audit: avoid umount hangs on missing mount

Don't fetch fcaps when umount2 is called with MNT_FORCE to avoid a process hang while it waits for the missing resource to (possibly never) re-appear.

Quoted 4 lines · Richard Guy Briggs

Don't fetch fcaps when umount2 is called with MNT_FORCE to avoid a process hang while it waits for the missing resource to (possibly never) re-appear.

The patch would be pretty good if the dependence on MNT_FORCE wasn't
added. As it is, it's buggy in more ways than one:

- It does the opposite of the above (i.e. skips fcaps *unless*
MNT_FORCE is set)

- sets LOOKUP_NO_REVAL from caller of path lookup, which is invalid
(LOOKUP_NO_REVAL is used only internally by path lookup)

- the fact that *_path_mountpoint_at() shouldn't touch the mount root
is independent of MNT_FORCE

I still don't quite understand what audit is trying to do here, but apparently it's okay to skip getxattr in the MNT_FORCE case. So why is it not okay to skip it in the non-MNT_FORCE case?

Thanks,
Miklos

The simple answer is that the audit PATH record format expects the four cap_f* fields to be there and a best effort is being attempted to fill in that information in an expected way with meaningful values. Perhaps better to accept that it is unreasonable to expect any fcaps on any umount operation and simply ignore those fields in the PATH record for umount syscall events.

Quoted 6 lines · Richard Guy Briggs

The simple answer is that the audit PATH record format expects the four cap_f* fields to be there and a best effort is being attempted to fill in that information in an expected way with meaningful values. Perhaps better to accept that it is unreasonable to expect any fcaps on any umount operation and simply ignore those fields in the PATH record for umount syscall events.

When there's a mount there are in fact two objects belonging to the exact same path, each having completely independent metadata: the mount point and the root of the mount. For example:

stat /mnt
umount /mnt
stat /mnt

The first stat will show the root of the mount, the second one will show the mount point.
Which one is the relevant for audit?

Not saying audit should be doing getxattr on any of them, just trying to see more clearly.

Thanks,
Miklos

Quoted 1 line · Miklos Szeredi

On Mon, Nov 19, 2018 at 11:59 PM Richard Guy Briggs <[email protected]>

Quoted 11 lines

Quoted 6 lines

The simple answer is that the audit PATH record format expects the four cap_f* fields to be there and a best effort is being attempted to fill in that information in an expected way with meaningful values. Perhaps better to accept that it is unreasonable to expect any fcaps on any umount operation and simply ignore those fields in the PATH record for umount syscall events.

When there's a mount there are in fact two objects belonging to the exact same path, each having completely independent metadata: the mount point and the root of the mount. For example:

Quoted 7 lines

stat /mnt
umount /mnt
stat /mnt

The first stat will show the root of the mount, the second one will show the mount point.
Which one is the relevant for audit?

It would be the root of the mount, the one that is visible to processes in that mount namespace.

Obviously, once that mount has been unmounted, it would be the mount point (no longer in use as such at that point) that is of interest.

On mounting, I'm guessing both would be of interest if the fcaps changed for that process-visible path in that mount namespace, so this provides an additional operation that would need recording aside from the case of a simple attribute change.

Quoted 5 lines

Not saying audit should be doing getxattr on any of them, just trying to see more clearly.

Thanks,
Miklos

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

On user and remote filesystems, a forced umount can still hang due to attemting to fetch the fcaps of a mounted filesystem that is no longer available.

These two patches take different approaches to address this, one by avoiding the lookup when the MNT_FORCE flag is included, the other by providing a method to filter out auditing specified types of filesystems.

This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.

Arguably the better way to address this issue is to disable auditing processes that touch removable filesystems.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100

Richard Guy Briggs (2):
audit: avoid fcaps on MNT_FORCE
audit: moar filter PATH records keyed on filesystem magic

 fs/namei.c            |  2 +-
 fs/namespace.c        |  3 +++
 include/linux/audit.h |  8 ++++++--
 kernel/audit.c        |  5 +++--
 kernel/audit.h        |  2 +-
 kernel/auditsc.c      | 29 ++++++++++++++++++++++++++---
 6 files changed, 40 insertions(+), 9 deletions(-)

Quoted 19 lines · Richard Guy Briggs

On user and remote filesystems, a forced umount can still hang due to attemting to fetch the fcaps of a mounted filesystem that is no longer available.

These two patches take different approaches to address this, one by avoiding the lookup when the MNT_FORCE flag is included, the other by providing a method to filter out auditing specified types of filesystems.

This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.

Arguably the better way to address this issue is to disable auditing processes that touch removable filesystems.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100

Richard Guy Briggs (2):
audit: avoid fcaps on MNT_FORCE
audit: moar filter PATH records keyed on filesystem magic

Quoted 7 lines

 fs/namei.c            |  2 +-
 fs/namespace.c        |  3 +++
 include/linux/audit.h |  8 ++++++--
 kernel/audit.c        |  5 +++--
 kernel/audit.h        |  2 +-
 kernel/auditsc.c      | 29 ++++++++++++++++++++++++++---
 6 files changed, 40 insertions(+), 9 deletions(-)

Just to get this out of the way, don't use "moar", spell it properly.

Beyond that, it's not clear to me from your cover letter if you are proposing these patches as an "or" or as an "and"; assuming the patch(es) are reasonable, do you want us to merge both of these patches, or only the one we like the most?

paul moore

Quoted 28 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

On user and remote filesystems, a forced umount can still hang due to attemting to fetch the fcaps of a mounted filesystem that is no longer available.

These two patches take different approaches to address this, one by avoiding the lookup when the MNT_FORCE flag is included, the other by providing a method to filter out auditing specified types of filesystems.

This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.

Arguably the better way to address this issue is to disable auditing processes that touch removable filesystems.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100

Richard Guy Briggs (2):
audit: avoid fcaps on MNT_FORCE
audit: moar filter PATH records keyed on filesystem magic

Quoted 7 lines

 fs/namei.c            |  2 +-
 fs/namespace.c        |  3 +++
 include/linux/audit.h |  8 ++++++--
 kernel/audit.c        |  5 +++--
 kernel/audit.h        |  2 +-
 kernel/auditsc.c      | 29 ++++++++++++++++++++++++++---
 6 files changed, 40 insertions(+), 9 deletions(-)

Quoted 6 lines

Just to get this out of the way, don't use "moar", spell it properly.

Beyond that, it's not clear to me from your cover letter if you are proposing these patches as an "or" or as an "and"; assuming the patch(es) are reasonable, do you want us to merge both of these patches, or only the one we like the most?

I would like each one to be considered on its own merits.

The second was discussed back when the logs were flooded with "(null)" PATH records due to debugfs and tracefs noise. Do you agree with the general concept or not?

The first is being picked apart (rightfully) due to assumptions and choices made long ago in the audit system. So while it is still in far more flux than the second patch, I think it has the potential to fix the problem more correctly and permanently but in the process may challenge our rules about the format and invariability of audit records. The basic premise is to prevent audit from trying to get information (fcaps) from a filesystem that is likely to be far more ephemeral than local on-disk kernel filesystems or to fail to do so more gracefully.

Quoted 36 lines · Richard Guy Briggs

Quoted 34 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

On user and remote filesystems, a forced umount can still hang due to attemting to fetch the fcaps of a mounted filesystem that is no longer available.

These two patches take different approaches to address this, one by avoiding the lookup when the MNT_FORCE flag is included, the other by providing a method to filter out auditing specified types of filesystems.

This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.

Arguably the better way to address this issue is to disable auditing processes that touch removable filesystems.
Please see the github issue tracker
https://github.com/linux-audit/audit-kernel/issues/100

Richard Guy Briggs (2):
audit: avoid fcaps on MNT_FORCE
audit: moar filter PATH records keyed on filesystem magic

Quoted 7 lines

 fs/namei.c            |  2 +-
 fs/namespace.c        |  3 +++
 include/linux/audit.h |  8 ++++++--
 kernel/audit.c        |  5 +++--
 kernel/audit.h        |  2 +-
 kernel/auditsc.c      | 29 ++++++++++++++++++++++++++---
 6 files changed, 40 insertions(+), 9 deletions(-)

Just to get this out of the way, don't use "moar", spell it properly.

Beyond that, it's not clear to me from your cover letter if you are proposing these patches as an "or" or as an "and"; assuming the patch(es) are reasonable, do you want us to merge both of these patches, or only the one we like the most?

Quoted 5 lines

I would like each one to be considered on its own merits.

The second was discussed back when the logs were flooded with "(null)" PATH records due to debugfs and tracefs noise. Do you agree with the general concept or not?

I believe I was in favor of this back then, and I think it is still a reasonable feature to add independent of the umount hang problem.

One possible enhancement might be to also support filesystem names and not just magic numbers. This could either be done in userspace or the kernel via AUDIT_FSNAME, or similar. If you do take the _FSNAME approach you should have the kernel convert that to a magic number when it translates the rule (performance reasons).

Quoted 8 lines

The first is being picked apart (rightfully) due to assumptions and choices made long ago in the audit system. So while it is still in far more flux than the second patch, I think it has the potential to fix the problem more correctly and permanently but in the process may challenge our rules about the format and invariability of audit records. The basic premise is to prevent audit from trying to get information (fcaps) from a filesystem that is likely to be far more ephemeral than local on-disk kernel filesystems or to fail to do so more gracefully.

There is one minor nit: use "flags" instead of "lflags" in the audit_inode parameter list, the local "flags" variable can be changed to something else; the parameters are what callers see, make them simple and familiar.

However, beyond that I think the general approach of not recording fcaps is reasonable if we can't reliably do it. What do the fcaps entries look like in this particular case, are they "0" or "?"? I would suggest "?" is the correct answer here.

paul moore