On Thu, Nov 30, 2023 at 5:47 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sat 18-11-23 20:30:18, Amir Goldstein wrote:
> > So far, fanotify returns -ENODEV or -EXDEV when trying to set a mark
> > on a filesystem with a "weak" fsid, namely, zero fsid (e.g. fuse), or
> > non-uniform fsid (e.g. btrfs non-root subvol).
> >
> > When group is watching inodes all from the same filesystem (or subvol),
> > allow adding inode marks with "weak" fsid, because there is no ambiguity
> > regarding which filesystem reports the event.
> >
> > The first mark added to a group determines if this group is single or
> > multi filesystem, depending on the fsid at the path of the added mark.
> >
> > If the first mark added has a "strong" fsid, marks with "weak" fsid
> > cannot be added and vice versa.
> >
> > If the first mark added has a "weak" fsid, following marks must have
> > the same "weak" fsid and the same sb as the first mark.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Looks mostly good to me, some small comments below.
>
> > @@ -1192,11 +1192,68 @@ static bool fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
> > return recalc;
> > }
> >
> > +struct fan_fsid {
> > + struct super_block *sb;
> > + __kernel_fsid_t id;
> > + bool weak;
> > +};
> > +
> > +static int fanotify_check_mark_fsid(struct fsnotify_group *group,
> > + struct fsnotify_mark *mark,
> > + struct fan_fsid *fsid)
>
> I'd call this fanotify_set_mark_fsid() because that's what it does and
> as part of that it also checks whether it can actually set the fsid...
ok.
>
> > @@ -1564,20 +1622,25 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
> > return fd;
> > }
> >
> > -static int fanotify_test_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
> > +static int fanotify_test_fsid(struct dentry *dentry, unsigned int flags,
> > + struct fan_fsid *fsid)
> > {
> > + unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
> > __kernel_fsid_t root_fsid;
> > int err;
> >
> > /*
> > * Make sure dentry is not of a filesystem with zero fsid (e.g. fuse).
> > */
> > - err = vfs_get_fsid(dentry, fsid);
> > + err = vfs_get_fsid(dentry, &fsid->id);
> > if (err)
> > return err;
> >
> > - if (!fsid->val[0] && !fsid->val[1])
> > - return -ENODEV;
> > + /* Allow weak fsid when marking inodes */
> > + fsid->sb = dentry->d_sb;
> > + fsid->weak = mark_type == FAN_MARK_INODE;
> > + if (!fsid->id.val[0] && !fsid->id.val[1])
> > + return fsid->weak ? 0 : -ENODEV;
> >
> > /*
> > * Make sure dentry is not of a filesystem subvolume (e.g. btrfs)
> > @@ -1587,10 +1650,10 @@ static int fanotify_test_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
> > if (err)
> > return err;
> >
> > - if (root_fsid.val[0] != fsid->val[0] ||
> > - root_fsid.val[1] != fsid->val[1])
> > - return -EXDEV;
> > + if (!fanotify_fsid_equal(&root_fsid, &fsid->id))
> > + return fsid->weak ? 0 : -EXDEV;
> >
> > + fsid->weak = false;
> > return 0;
> > }
>
> So the handling of 'weak' confuses me here as it combines determining
> whether fsid is weak with determining whether we accept it or not. Maybe my
> brain is just fried... Can we maybe simplify to something like:
>
> fsid->sb = dentry->d_sb;
> if (!fsid->id.val[0] && !fsid->id.val[1]) {
> fsid->weak = true;
> goto check;
> }
>
> ... fetch root_fsid ...
>
> if (!fanotify_fsid_equal(&root_fsid, &fsid->id))
> fsid->weak = true;
>
> check:
> /* Allow weak fsids only for inode marks... */
> if (fsid->weak && mark_type != FAN_MARK_INODE)
> return -EXDEV;
> return 0;
>
> This is how I understand the logic from your description but I'm not 100%
> sure this is equivalent to your code above ;).
Almost.
The return value for FUSE + sb mark is ENODEV.
The code below should work and be readable.
Thanks,
Amir.
fsid->sb = dentry->d_sb;
if (!fsid->id.val[0] && !fsid->id.val[1]) {
err = -ENODEV;
goto weak;
}
... fetch root_fsid ...
if (!fanotify_fsid_equal(&root_fsid, &fsid->id)) {
err = -EXDEV;
goto weak;
}
return 0;
weak:
/* Allow weak fsid when marking inodes */
fsid->weak = true;
return (mark_type == FAN_MARK_INODE) ? 0 : err;
}
