Re: [RFC PATCH ghak100 V1 0/2] audit: avoid umount hangs on missing mount

[Richard Guy Briggs]

On 2018-12-14 17:02, Paul Moore wrote:
> On Fri, Dec 14, 2018 at 11:27 AM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > On 2018-12-12 08:03, Paul Moore wrote:
> > > On Fri, Nov 16, 2018 at 12:34 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > > > 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(-)
> > >
> > > 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?
> 
> 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.

I wasn't so keen then, but see more use for it now.

> 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).

I had looked at filesystem names previously with Steve and I seem to
recall that was much better left to userspace to convert.  The biggest
challenge I see here is that in-kernel filsystems have all their magic
numbers listed in the kernel, whereas discovering the magic numbers for
fuse and other remote filesystems is a bit harder.  This was one of the
reasons I wanted to include the magic number in the name= field in patch
#1 for ghak8 so it was easy to figure out what type of filesystem was
involved.

> > 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.

Noted.

> 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.

I'd agree "?" would be the best option to make it clear it is not
available rather than just zero.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@xxxxxxxxxx>
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