Thread (19 messages) 19 messages, 6 authors, 2025-02-13

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

From: Christian Brauner <brauner@kernel.org>
Date: 2025-01-31 12:09:56
Also in: linux-fsdevel, selinux

On Thu, Jan 30, 2025 at 04:05:53PM -0500, Paul Moore wrote:
On Wed, Jan 29, 2025 at 11:58 AM Miklos Szeredi [off-list ref] wrote:
quoted
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 <redacted>
---
 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(-)
...
quoted
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.  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.

If you need an example on how to add a new SELinux permission, you can
look at commit ed5d44d42c95 ("selinux: Implement userns_create hook")
for a fairly simple example.  In the watch_mountns case things are
slightly different due to the existence of the COMMON_FILE_PERMS
macro, but you basically want to add "watch_mountns" to the end of the
COMMON_FILE_PERMS macro in security/selinux/include/classmap.h.  Of
course if you aren't sure about something, let me know.  As a FYI, my
network access will be spotty starting tonight and extending through
the weekend, but I will have network/mail access at least once a day.

Now back to the merge into the VFS tree ... I was very surprised to
open this patchset and see that Christian had merged v5 after less
than 24 hours (at least according to the email timestamps that I see)
and without an explicit ACK for the SELinux changes.  I've mentioned
this to you before Christian, please do not merge any SELinux, LSM
framework, or audit related patches without an explicit ACK.  I
Things go into the tree for testing when the VFS side is ready for
testing. We're at v5 and the patchset has gone through four iterations
over multiple months. It will go into linux-next and fs-next now for as
much expsure as possible.

I'm not sure what the confusion between merging things into a tree and
sending things upstream is. I have explained this to you before. The
application message is also pretty clear about that.
recognize that sometimes there are highly critical security issues
that need immediate attention, but that is clearly not the case here,
and there are other procedures to help deal with those emergency
scenarios.

-- 
paul-moore.com
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help