Re: [PATCH v5 2/3] fanotify: notify on mount attach and detach

[Christian Brauner <brauner@kernel.org>]

On Fri, Jan 31, 2025 at 09:28:31AM -0500, Paul Moore wrote:
> On Fri, Jan 31, 2025 at 5:53 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Thu, 30 Jan 2025 at 22:06, Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Jan 29, 2025 at 11:58 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > > >
> > > > Add notifications for attaching and detaching mounts.  The following new
> > > > event masks are added:
> > > >
> > > >   FAN_MNT_ATTACH  - Mount was attached
> > > >   FAN_MNT_DETACH  - Mount was detached
> > > >
> > > > If a mount is moved, then the event is reported with (FAN_MNT_ATTACH |
> > > > FAN_MNT_DETACH).
> > > >
> > > > These events add an info record of type FAN_EVENT_INFO_TYPE_MNT containing
> > > > these fields identifying the affected mounts:
> > > >
> > > >   __u64 mnt_id    - the ID of the mount (see statmount(2))
> > > >
> > > > FAN_REPORT_MNT must be supplied to fanotify_init() to receive these events
> > > > and no other type of event can be received with this report type.
> > > >
> > > > Marks are added with FAN_MARK_MNTNS, which records the mount namespace from
> > > > an nsfs file (e.g. /proc/self/ns/mnt).
> > > >
> > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > > > ---
> > > >  fs/mount.h                         |  2 +
> > > >  fs/namespace.c                     | 14 +++--
> > > >  fs/notify/fanotify/fanotify.c      | 38 +++++++++++--
> > > >  fs/notify/fanotify/fanotify.h      | 18 +++++++
> > > >  fs/notify/fanotify/fanotify_user.c | 87 +++++++++++++++++++++++++-----
> > > >  fs/notify/fdinfo.c                 |  5 ++
> > > >  include/linux/fanotify.h           | 12 +++--
> > > >  include/uapi/linux/fanotify.h      | 10 ++++
> > > >  security/selinux/hooks.c           |  4 ++
> > > >  9 files changed, 167 insertions(+), 23 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 7b867dfec88b..06d073eab53c 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -3395,6 +3395,10 @@ static int selinux_path_notify(const struct path *path, u64 mask,
> > > >         case FSNOTIFY_OBJ_TYPE_INODE:
> > > >                 perm = FILE__WATCH;
> > > >                 break;
> > > > +       case FSNOTIFY_OBJ_TYPE_MNTNS:
> > > > +               /* Maybe introduce FILE__WATCH_MOUNTNS? */
> > > > +               perm = FILE__WATCH_MOUNT;
> > > > +               break;
> > > >         default:
> > > >                 return -EINVAL;
> > > >         }
> > >
> > > Ignoring for a moment that this patch was merged without an explicit
> > > ACK for the SELinux changes, let's talk about these SELinux changes
> > > ...
> > >
> > > I understand that you went with the "simpler version" because you
> > > didn't believe the discussion was converging, which is fair, however,
> > > I believe Daniel's argument is convincing enough to warrant the new
> > > permission.
> >
> > Fine, I'll work on this.
> 
> Great, thanks.
> 
> > >  Yes, it has taken me approximately two days to find the
> > > time to revisit this topic and reply with some clarity, but personally
> > > I feel like that is not an unreasonable period of time, especially for
> > > a new feature discussion occurring during the merge window.
> >
> > Definitely not.
> >
> > Christian is definitely very responsive and quick to queue things up,
> > and that can have drawbacks.   In this he made it clear that he wants
> > to get this queued ASAP regardless of whether there's decision on the
> > SELinux side or not.
> 
> When one merges code that affects another subsystem without an
> explicit ACK from the affected subsystem when the maintainer has asked
> for others to clear the code change with an ACK, it's hard to see that
> as anything but bad behavior at its best and reckless behavior at its
> worst.  It is doubly troubling in cases like this where the code
> change is user visible.
> 
> > What I think might be a good thing if Christian could record
> > conditional NAKs such as this one from you, that need to be worked on
> > before sending a feature upstream.  That would prevent wrong code
> > being sent upstream due to lack of attention.
> 
> Christian's merge notification email already has this section:
> 
>   "Please report any outstanding bugs that were missed
>    during review in a new review to the original patch series
>    allowing us to drop it."
> 
> ... and to be fair, the vfs-6.15.mount branch mentioned in the
> notification does appear to be gone.

The branch is very much alive but it has never been public for very
obvious reasons: new patches don't appear in -next or elsewhere until
the merge window is closed. They will be pushed out today.