Re: [RFC PATCH ghak100 V1 0/2] audit: avoid umount hangs on missing mount
On 2018-12-14 17:02, Paul Moore wrote: > On Fri, Dec 14, 2018 at 11:27 AM Richard Guy Briggs wrote: > > On 2018-12-12 08:03, Paul Moore wrote: > > > On Fri, Nov 16, 2018 at 12:34 PM Richard Guy Briggs > > > 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 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak100 V1 0/2] audit: avoid umount hangs on missing mount
On Fri, Dec 14, 2018 at 11:27 AM Richard Guy Briggs wrote: > On 2018-12-12 08:03, Paul Moore wrote: > > On Fri, Nov 16, 2018 at 12:34 PM Richard Guy Briggs 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. 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). > 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 www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak100 V1 0/2] audit: avoid umount hangs on missing mount
On 2018-12-12 08:03, Paul Moore wrote: > On Fri, Nov 16, 2018 at 12:34 PM Richard Guy Briggs 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? 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. > paul moore - RGB -- Richard Guy Briggs 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak100 V1 0/2] audit: avoid umount hangs on missing mount
On Fri, Nov 16, 2018 at 12:34 PM Richard Guy Briggs 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? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
