On Tue, Nov 21, 2023 at 3:33 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Mon, Nov 20, 2023 at 9:42 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Sat, Nov 18, 2023 at 8:30 PM Amir Goldstein <amir73il@xxxxxxxxx> 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>
> > > ---
> > > fs/notify/fanotify/fanotify.c | 15 +----
> > > fs/notify/fanotify/fanotify.h | 6 ++
> > > fs/notify/fanotify/fanotify_user.c | 97 ++++++++++++++++++++++++------
> > > include/linux/fsnotify_backend.h | 1 +
> > > 4 files changed, 90 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index aff1ab3c32aa..1e4def21811e 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -29,12 +29,6 @@ static unsigned int fanotify_hash_path(const struct path *path)
> > > hash_ptr(path->mnt, FANOTIFY_EVENT_HASH_BITS);
> > > }
> > >
> > > -static inline bool fanotify_fsid_equal(__kernel_fsid_t *fsid1,
> > > - __kernel_fsid_t *fsid2)
> > > -{
> > > - return fsid1->val[0] == fsid2->val[0] && fsid1->val[1] == fsid2->val[1];
> > > -}
> > > -
> > > static unsigned int fanotify_hash_fsid(__kernel_fsid_t *fsid)
> > > {
> > > return hash_32(fsid->val[0], FANOTIFY_EVENT_HASH_BITS) ^
> > > @@ -851,7 +845,8 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
> > > if (!(mark->flags & FSNOTIFY_MARK_FLAG_HAS_FSID))
> > > continue;
> > > fsid = FANOTIFY_MARK(mark)->fsid;
> > > - if (WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
> > > + if (!(mark->flags & FSNOTIFY_MARK_FLAG_WEAK_FSID) &&
> > > + WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
> > > continue;
> > > return fsid;
> > > }
> > > @@ -933,12 +928,8 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
> > > return 0;
> > > }
> > >
> > > - if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS)) {
> > > + if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS))
> > > fsid = fanotify_get_fsid(iter_info);
> > > - /* Racing with mark destruction or creation? */
> > > - if (!fsid.val[0] && !fsid.val[1])
> > > - return 0;
> > > - }
> > >
> > > event = fanotify_alloc_event(group, mask, data, data_type, dir,
> > > file_name, &fsid, match_mask);
> > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > > index 2847fa564298..9c97ae638ac5 100644
> > > --- a/fs/notify/fanotify/fanotify.h
> > > +++ b/fs/notify/fanotify/fanotify.h
> > > @@ -504,6 +504,12 @@ static inline __kernel_fsid_t *fanotify_mark_fsid(struct fsnotify_mark *mark)
> > > return &FANOTIFY_MARK(mark)->fsid;
> > > }
> > >
> > > +static inline bool fanotify_fsid_equal(__kernel_fsid_t *fsid1,
> > > + __kernel_fsid_t *fsid2)
> > > +{
> > > + return fsid1->val[0] == fsid2->val[0] && fsid1->val[1] == fsid2->val[1];
> > > +}
> > > +
> > > static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark)
> > > {
> > > unsigned int mflags = 0;
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index e3d836d4d156..1cc68ea56c17 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -23,7 +23,7 @@
> > >
> > > #include <asm/ioctls.h>
> > >
> > > -#include "../../mount.h"
> > > +#include "../fsnotify.h"
> > > #include "../fdinfo.h"
> > > #include "fanotify.h"
> > >
> > > @@ -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)
> > > +{
> > > + struct fsnotify_mark_connector *conn;
> > > + struct fsnotify_mark *old;
> > > + struct super_block *old_sb = NULL;
> > > +
> > > + *fanotify_mark_fsid(mark) = fsid->id;
> > > + mark->flags |= FSNOTIFY_MARK_FLAG_HAS_FSID;
> > > + if (fsid->weak)
> > > + mark->flags |= FSNOTIFY_MARK_FLAG_WEAK_FSID;
> > > +
> > > + /* First mark added will determine if group is single or multi fsid */
> > > + if (list_empty(&group->marks_list))
> > > + return 0;
> > > +
> > > + /* Find sb of an existing mark */
> > > + list_for_each_entry(old, &group->marks_list, g_list) {
> > > + conn = READ_ONCE(old->connector);
> > > + if (!conn)
> > > + continue;
> > > + old_sb = fsnotify_connector_sb(conn);
> > > + if (old_sb)
> > > + break;
> > > + }
> > > +
> > > + /* Only detached marks left? */
> > > + if (!old_sb)
> > > + return 0;
> > > +
> > > + /* Do not allow mixing of marks with weak and strong fsid */
> > > + if ((mark->flags ^ old->flags) & FSNOTIFY_MARK_FLAG_WEAK_FSID)
> > > + return -EXDEV;
> > > +
> > > + /* Allow mixing of marks with strong fsid from different fs */
> > > + if (!fsid->weak)
> > > + return 0;
> > > +
> > > + /* Do not allow mixing marks with weak fsid from different fs */
> > > + if (old_sb != fsid->sb)
> > > + return -EXDEV;
> > > +
> > > + /* Do not allow mixing marks from different btrfs sub-volumes */
> > > + if (!fanotify_fsid_equal(fanotify_mark_fsid(old),
> > > + fanotify_mark_fsid(mark)))
> > > + return -EXDEV;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
> > > fsnotify_connp_t *connp,
> > > unsigned int obj_type,
> > > unsigned int fan_flags,
> > > - __kernel_fsid_t *fsid)
> > > + struct fan_fsid *fsid)
> > > {
> > > struct ucounts *ucounts = group->fanotify_data.ucounts;
> > > struct fanotify_mark *fan_mark;
> > > @@ -1225,8 +1282,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
> > >
> > > /* Cache fsid of filesystem containing the marked object */
> > > if (fsid) {
> > > - fan_mark->fsid = *fsid;
> > > - mark->flags |= FSNOTIFY_MARK_FLAG_HAS_FSID;
> > > + ret = fanotify_check_mark_fsid(group, mark, fsid);
> > > + if (ret)
> >
> > OOPS, missed fsnotify_put_mark(mark);
> > better add a goto target out_put_mark as this is the second case now.
>
> FYI, I pushed the fix to:
> https://github.com/amir73il/linux/commits/fanotify_fsid
>
> Let me know if you want me to post v2 for this.
>
Hi Jan,
Ping.
Reminder, I have LTP tests for testing fs with "weak" fsid [2].
After your LTP patches are merged, I will rebase them.
The way that I tested fs with "weak" fsid is that LTP tests
the ntfs-3g FUSE filesystem with .all_filesystems = 1.
With this work, the tests started to run on FUSE and skip
the test cases of sb/mount marks.
Thanks,
Amir.
[2] https://github.com/amir73il/tlp/commits/fanotify_fsid
