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

[Paul Moore]

On Fri, Jan 31, 2025 at 5:53 AM Miklos Szeredi <miklos at szeredi.hu> wrote:
> On Thu, 30 Jan 2025 at 22:06, Paul Moore <paul at paul-moore.com> wrote:
> > On Wed, Jan 29, 2025 at 11:58 AM Miklos Szeredi <mszeredi at 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 at 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.

-- 
paul-moore.com