On Thu, Apr 20, 2023 at 4:12 PM Jan Kara [off-list ref] wrote:

On Sun 16-04-23 09:07:22, Amir Goldstein wrote:

quoted

An unprivileged user is allowed to create an fanotify group and add
inode marks, but not filesystem and mount marks.

Add limited support for setting up filesystem and mount marks by an
unprivileged user under the following conditions:

1.   User has CAP_SYS_ADMIN in the user ns where the group was created
2.a. User has CAP_SYS_ADMIN in the user ns where the filesystem was
     mounted (implies FS_USERNS_MOUNT)
  OR (in case setting up a mark mount)
2.b. User has CAP_SYS_ADMIN in the user ns attached to an idmapped mount

Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good to me. Just two comments below.

quoted

Christian,

You can find this patch, along with FAN_UNMOUNT patches on my github [3].
Please confirm that this meets your needs for watching container mounts.

[3] https://github.com/amir73il/linux/commits/fan_unmount

Yeah, it would be good to get ack from Christian that the model you propose
works for him.

quoted

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index db3b79b8e901..2c3e123aee14 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c

@@ -1238,6 +1238,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
       * A group with FAN_UNLIMITED_MARKS does not contribute to mark count
       * in the limited groups account.
       */
+     BUILD_BUG_ON(!(FANOTIFY_ADMIN_INIT_FLAGS & FAN_UNLIMITED_MARKS));
      if (!FAN_GROUP_FLAG(group, FAN_UNLIMITED_MARKS) &&
          !inc_ucount(ucounts->ns, ucounts->uid, UCOUNT_FANOTIFY_MARKS))
              return ERR_PTR(-ENOSPC);

...

quoted

@@ -1557,21 +1559,13 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
              goto out_destroy_group;
      }

+     BUILD_BUG_ON(!(FANOTIFY_ADMIN_INIT_FLAGS & FAN_UNLIMITED_QUEUE));
      if (flags & FAN_UNLIMITED_QUEUE) {
-             fd = -EPERM;
-             if (!capable(CAP_SYS_ADMIN))
-                     goto out_destroy_group;
              group->max_events = UINT_MAX;
      } else {
              group->max_events = fanotify_max_queued_events;
      }

-     if (flags & FAN_UNLIMITED_MARKS) {
-             fd = -EPERM;
-             if (!capable(CAP_SYS_ADMIN))
-                     goto out_destroy_group;
-     }
-

Perhaps this hunk (plus the BUILD_BUG_ON hunk above) should go into a
separate patch with a proper changelog?  I was scratching my head over it
for a while until I've realized it's unrelated cleanup of dead code.

Sure. It took me a while to understand it myself, when I re-read it
after two years...

Thanks,
Amir.