On Fri, May 16, 2025 at 3:22 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Sat, 19 Apr 2025 at 12:07, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > @@ -1987,12 +1988,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> > obj = inode;
> > } else if (obj_type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
> > obj = path.mnt;
> > + user_ns = real_mount(obj)->mnt_ns->user_ns;
> > } else if (obj_type == FSNOTIFY_OBJ_TYPE_SB) {
> > obj = path.mnt->mnt_sb;
> > + user_ns = path.mnt->mnt_sb->s_user_ns;
>
> The patch header notes that user_ns != &init_user_ns implies
> FS_USERNS_MOUNT, but it'd be nice to document this with a WARN_ON() in
> the code as well.
>
Can't do that because the commit message is wrong...
An sb *can* have non-init s_user_ns without FS_USERNS_MOUNT
if the mounter was running in non-init user ns, but had CAP_SYS_ADMIN
in init_user_ns.
Maybe not a very likely use case, but still cannot be asserted,
so we better remove the (*FS_USERNS_MOUNT*) remark
from comment and commit message.
> > } else if (obj_type == FSNOTIFY_OBJ_TYPE_MNTNS) {
> > obj = mnt_ns_from_dentry(path.dentry);
> > + user_ns = ((struct mnt_namespace *)obj)->user_ns;
>
> It would be much more elegant if the type wasn't lost before this assignment.
True.
We can make it:
struct mnt_namespace *mntns = mnt_ns_from_dentry(path.dentry);
user_ns = mntns->user_ns;
obj = mntns;
>
> Otherwise looks good:
>
> Reviewed-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
>
Thanks for the review!
Amir.
