[PATCH v2 0/3] fanotify support for btrfs sub-volumes

[PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Amir Goldstein]

Jan,

As agreed on the review of v1 [1], we do not need any vfs changes
to support fanotify on btrfs sub-volumes and we can enable setting
marks on btrfs sub-volumes simply by caching the fsid in the mark
object instead of the connector.

This is the would be man page update to clarify the meaning of fsid
as it is reflected in this patch set:

fsid

  This is a unique identifier of the filesystem containing the object
  associated with the event.  It is a structure of type __kernel_fsid_t
  and contains the same value reported in  f_fsid  when calling
  statfs(2) with the same pathname argument that was used for
  fanotify_mark(2).  Note that some filesystems (e.g., btrfs(5)) report
  non-uniform values of f_fsid on different objects of the same filesystem.
  In these cases, if fanotify_mark(2) is called several times with different
  pathname values, the fsid value reported in events will match f_fsid
  associated  with at least one of those pathname values.

Thanks,
Amir.

[1] https://lore.kernel.org/r/CAOQ4uxg9wjESoCFNDADbneF0-nW4xVHHV3Rhhp=gJwAs=S83dQ@mail.gmail.com/

Amir Goldstein (3):
  fanotify: store fsid in mark instead of in connector
  fanotify: report the most specific fsid for btrfs
  fanotify: support setting marks in btrfs sub-volumes

 fs/notify/fanotify/fanotify.c      | 21 ++++--------
 fs/notify/fanotify/fanotify.h      | 10 ++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++----------
 fs/notify/mark.c                   | 52 +++++-------------------------
 include/linux/fsnotify_backend.h   | 18 +++++------
 5 files changed, 47 insertions(+), 85 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] fanotify: store fsid in mark instead of in connector

[Amir Goldstein]

With btrfs, different inodes on the same sb can have different fsid.
We would like to report fsid in events that is the same as the fsid of
the object whose path was used to setup the mark.

That means that two different groups can report different fsid on
different sb marks on the same btrfs sb, so we need to store the fsid
in the marks of the group instead of in the shared conenctor.

This is needed to support fanotify marks in btrfs sub-volumes.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 19 +++--------
 fs/notify/fanotify/fanotify.h      | 10 ++++++
 fs/notify/fanotify/fanotify_user.c | 18 ++++++++---
 fs/notify/mark.c                   | 52 +++++-------------------------
 include/linux/fsnotify_backend.h   | 13 +++-----
 5 files changed, 42 insertions(+), 70 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 9dac7f6e72d2..aff1ab3c32aa 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -838,9 +838,8 @@ static struct fanotify_event *fanotify_alloc_event(
 }
 
 /*
- * Get cached fsid of the filesystem containing the object from any connector.
- * All connectors are supposed to have the same fsid, but we do not verify that
- * here.
+ * Get cached fsid of the filesystem containing the object from any mark.
+ * All marks are supposed to have the same fsid, but we do not verify that here.
  */
 static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 {
@@ -849,17 +848,9 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 	__kernel_fsid_t fsid = {};
 
 	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
-		struct fsnotify_mark_connector *conn;
-
-		conn = READ_ONCE(mark->connector);
-		/* Mark is just getting destroyed or created? */
-		if (!conn)
-			continue;
-		if (!(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID))
+		if (!(mark->flags & FSNOTIFY_MARK_FLAG_HAS_FSID))
 			continue;
-		/* Pairs with smp_wmb() in fsnotify_add_mark_list() */
-		smp_rmb();
-		fsid = conn->fsid;
+		fsid = FANOTIFY_MARK(mark)->fsid;
 		if (WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
 			continue;
 		return fsid;
@@ -1068,7 +1059,7 @@ static void fanotify_freeing_mark(struct fsnotify_mark *mark,
 
 static void fanotify_free_mark(struct fsnotify_mark *fsn_mark)
 {
-	kmem_cache_free(fanotify_mark_cache, fsn_mark);
+	kmem_cache_free(fanotify_mark_cache, FANOTIFY_MARK(fsn_mark));
 }
 
 const struct fsnotify_ops fanotify_fsnotify_ops = {
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index e8a3c28c5d12..88367e7b41dc 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -489,6 +489,16 @@ static inline unsigned int fanotify_event_hash_bucket(
 	return event->hash & FANOTIFY_HTABLE_MASK;
 }
 
+struct fanotify_mark {
+	struct fsnotify_mark fsn_mark;
+	__kernel_fsid_t fsid;
+};
+
+static inline struct fanotify_mark *FANOTIFY_MARK(struct fsnotify_mark *mark)
+{
+	return container_of(mark, struct fanotify_mark, fsn_mark);
+}
+
 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 0eb9622e8a9f..fdd39bf91806 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1199,6 +1199,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 						   __kernel_fsid_t *fsid)
 {
 	struct ucounts *ucounts = group->fanotify_data.ucounts;
+	struct fanotify_mark *fan_mark;
 	struct fsnotify_mark *mark;
 	int ret;
 
@@ -1211,17 +1212,26 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 	    !inc_ucount(ucounts->ns, ucounts->uid, UCOUNT_FANOTIFY_MARKS))
 		return ERR_PTR(-ENOSPC);
 
-	mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
-	if (!mark) {
+	fan_mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
+	if (!fan_mark) {
 		ret = -ENOMEM;
 		goto out_dec_ucounts;
 	}
 
+	mark = &fan_mark->fsn_mark;
 	fsnotify_init_mark(mark, group);
 	if (fan_flags & FAN_MARK_EVICTABLE)
 		mark->flags |= FSNOTIFY_MARK_FLAG_NO_IREF;
 
-	ret = fsnotify_add_mark_locked(mark, connp, obj_type, 0, fsid);
+	/* Cache fsid of filesystem containing the marked object */
+	if (fsid) {
+		fan_mark->fsid = *fsid;
+		mark->flags |= FSNOTIFY_MARK_FLAG_HAS_FSID;
+	} else {
+		fan_mark->fsid.val[0] = fan_mark->fsid.val[1] = 0;
+	}
+
+	ret = fsnotify_add_mark_locked(mark, connp, obj_type, 0);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		goto out_dec_ucounts;
@@ -1935,7 +1945,7 @@ static int __init fanotify_user_setup(void)
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 12);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 11);
 
-	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
+	fanotify_mark_cache = KMEM_CACHE(fanotify_mark,
 					 SLAB_PANIC|SLAB_ACCOUNT);
 	fanotify_fid_event_cachep = KMEM_CACHE(fanotify_fid_event,
 					       SLAB_PANIC);
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index c74ef947447d..d6944ff86ffa 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -537,8 +537,7 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 }
 
 static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
-					       unsigned int obj_type,
-					       __kernel_fsid_t *fsid)
+					       unsigned int obj_type)
 {
 	struct fsnotify_mark_connector *conn;
 
@@ -550,14 +549,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	conn->flags = 0;
 	conn->type = obj_type;
 	conn->obj = connp;
-	/* Cache fsid of filesystem containing the object */
-	if (fsid) {
-		conn->fsid = *fsid;
-		conn->flags = FSNOTIFY_CONN_FLAG_HAS_FSID;
-	} else {
-		conn->fsid.val[0] = conn->fsid.val[1] = 0;
-		conn->flags = 0;
-	}
+	conn->flags = 0;
 	fsnotify_get_sb_connectors(conn);
 
 	/*
@@ -608,8 +600,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
  */
 static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 				  fsnotify_connp_t *connp,
-				  unsigned int obj_type,
-				  int add_flags, __kernel_fsid_t *fsid)
+				  unsigned int obj_type, int add_flags)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
@@ -619,41 +610,15 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 	if (WARN_ON(!fsnotify_valid_obj_type(obj_type)))
 		return -EINVAL;
 
-	/* Backend is expected to check for zero fsid (e.g. tmpfs) */
-	if (fsid && WARN_ON_ONCE(!fsid->val[0] && !fsid->val[1]))
-		return -ENODEV;
-
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(connp);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(connp, obj_type,
-							  fsid);
+		err = fsnotify_attach_connector_to_object(connp, obj_type);
 		if (err)
 			return err;
 		goto restart;
-	} else if (fsid && !(conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID)) {
-		conn->fsid = *fsid;
-		/* Pairs with smp_rmb() in fanotify_get_fsid() */
-		smp_wmb();
-		conn->flags |= FSNOTIFY_CONN_FLAG_HAS_FSID;
-	} else if (fsid && (conn->flags & FSNOTIFY_CONN_FLAG_HAS_FSID) &&
-		   (fsid->val[0] != conn->fsid.val[0] ||
-		    fsid->val[1] != conn->fsid.val[1])) {
-		/*
-		 * Backend is expected to check for non uniform fsid
-		 * (e.g. btrfs), but maybe we missed something?
-		 * Only allow setting conn->fsid once to non zero fsid.
-		 * inotify and non-fid fanotify groups do not set nor test
-		 * conn->fsid.
-		 */
-		pr_warn_ratelimited("%s: fsid mismatch on object of type %u: "
-				    "%x.%x != %x.%x\n", __func__, conn->type,
-				    fsid->val[0], fsid->val[1],
-				    conn->fsid.val[0], conn->fsid.val[1]);
-		err = -EXDEV;
-		goto out_err;
 	}
 
 	/* is mark the first mark? */
@@ -703,7 +668,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
  */
 int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 			     fsnotify_connp_t *connp, unsigned int obj_type,
-			     int add_flags, __kernel_fsid_t *fsid)
+			     int add_flags)
 {
 	struct fsnotify_group *group = mark->group;
 	int ret = 0;
@@ -723,7 +688,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	fsnotify_get_mark(mark); /* for g_list */
 	spin_unlock(&mark->lock);
 
-	ret = fsnotify_add_mark_list(mark, connp, obj_type, add_flags, fsid);
+	ret = fsnotify_add_mark_list(mark, connp, obj_type, add_flags);
 	if (ret)
 		goto err;
 
@@ -742,14 +707,13 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 }
 
 int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp,
-		      unsigned int obj_type, int add_flags,
-		      __kernel_fsid_t *fsid)
+		      unsigned int obj_type, int add_flags)
 {
 	int ret;
 	struct fsnotify_group *group = mark->group;
 
 	fsnotify_group_lock(group);
-	ret = fsnotify_add_mark_locked(mark, connp, obj_type, add_flags, fsid);
+	ret = fsnotify_add_mark_locked(mark, connp, obj_type, add_flags);
 	fsnotify_group_unlock(group);
 	return ret;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index c0892d75ce33..a80b525ca653 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -472,10 +472,8 @@ typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
 struct fsnotify_mark_connector {
 	spinlock_t lock;
 	unsigned short type;	/* Type of object [lock] */
-#define FSNOTIFY_CONN_FLAG_HAS_FSID	0x01
 #define FSNOTIFY_CONN_FLAG_HAS_IREF	0x02
 	unsigned short flags;	/* flags [lock] */
-	__kernel_fsid_t fsid;	/* fsid of filesystem containing object */
 	union {
 		/* Object pointer [lock] */
 		fsnotify_connp_t *obj;
@@ -530,6 +528,7 @@ struct fsnotify_mark {
 #define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY	0x0100
 #define FSNOTIFY_MARK_FLAG_NO_IREF		0x0200
 #define FSNOTIFY_MARK_FLAG_HAS_IGNORE_FLAGS	0x0400
+#define FSNOTIFY_MARK_FLAG_HAS_FSID		0x0800
 	unsigned int flags;		/* flags [mark->lock] */
 };
 
@@ -763,11 +762,10 @@ extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp,
 /* attach the mark to the object */
 extern int fsnotify_add_mark(struct fsnotify_mark *mark,
 			     fsnotify_connp_t *connp, unsigned int obj_type,
-			     int add_flags, __kernel_fsid_t *fsid);
+			     int add_flags);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 				    fsnotify_connp_t *connp,
-				    unsigned int obj_type, int add_flags,
-				    __kernel_fsid_t *fsid);
+				    unsigned int obj_type, int add_flags);
 
 /* attach the mark to the inode */
 static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
@@ -775,15 +773,14 @@ static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
 					  int add_flags)
 {
 	return fsnotify_add_mark(mark, &inode->i_fsnotify_marks,
-				 FSNOTIFY_OBJ_TYPE_INODE, add_flags, NULL);
+				 FSNOTIFY_OBJ_TYPE_INODE, add_flags);
 }
 static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
 						 struct inode *inode,
 						 int add_flags)
 {
 	return fsnotify_add_mark_locked(mark, &inode->i_fsnotify_marks,
-					FSNOTIFY_OBJ_TYPE_INODE, add_flags,
-					NULL);
+					FSNOTIFY_OBJ_TYPE_INODE, add_flags);
 }
 
 /* given a group and a mark, flag mark to be freed when all references are dropped */
-- 
2.34.1

[PATCH v2 2/3] fanotify: report the most specific fsid for btrfs

[Amir Goldstein]

With btrfs, a marked inode and marked sb may have a different fsid.
If both inode and sb are marked with a specific event type in mask,
report the more specific fsid (i.e. of the inode) in the event.

This is needed to support fanotify marks in btrfs sub-volumes.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    | 2 ++
 include/linux/fsnotify_backend.h | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index aff1ab3c32aa..3053606d7ff5 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -840,6 +840,8 @@ static struct fanotify_event *fanotify_alloc_event(
 /*
  * Get cached fsid of the filesystem containing the object from any mark.
  * All marks are supposed to have the same fsid, but we do not verify that here.
+ * With btrfs, a marked inode and marked sb may have a different fsid.
+ * In this case, we will return the more specific fsid (i.e. of the inode).
  */
 static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 {
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a80b525ca653..f329375bef22 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -372,13 +372,14 @@ static inline struct fs_error_report *fsnotify_data_error_report(
  * the other way around, because an event can match different watched objects
  * of the same object type.
  * For example, both parent and child are watching an object of type inode.
+ * The order of iteration is from most specific (inode) to most general (sb).
  */
 enum fsnotify_iter_type {
 	FSNOTIFY_ITER_TYPE_INODE,
+	FSNOTIFY_ITER_TYPE_INODE2,
+	FSNOTIFY_ITER_TYPE_PARENT,
 	FSNOTIFY_ITER_TYPE_VFSMOUNT,
 	FSNOTIFY_ITER_TYPE_SB,
-	FSNOTIFY_ITER_TYPE_PARENT,
-	FSNOTIFY_ITER_TYPE_INODE2,
 	FSNOTIFY_ITER_TYPE_COUNT
 };
 
-- 
2.34.1

[PATCH v2 3/3] fanotify: support setting marks in btrfs sub-volumes

[Amir Goldstein]

Setting fanotify marks that report events with fid on btrfs sub-volumes
was not supported, because in the case of btrfs sub-volumes, there is no
uniform fsid that can be reported in events.

Now that we report the fsid on the object whose path was used to setup
the mark, we can allow support setting marks in btrfs sub-volumes.

Users that make multiple fanotify_mark(2) calls on the same filesystem
are expected to call statfs(2) on every path were a mark was setup and
can expect that any of those fsid could be reported in any of the events.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index fdd39bf91806..59a7a720fd08 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1566,7 +1566,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 static int fanotify_test_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
 {
-	__kernel_fsid_t root_fsid;
 	int err;
 
 	/*
@@ -1579,18 +1578,6 @@ static int fanotify_test_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
 	if (!fsid->val[0] && !fsid->val[1])
 		return -ENODEV;
 
-	/*
-	 * Make sure dentry is not of a filesystem subvolume (e.g. btrfs)
-	 * which uses a different fsid than sb root.
-	 */
-	err = vfs_get_fsid(dentry->d_sb->s_root, &root_fsid);
-	if (err)
-		return err;
-
-	if (root_fsid.val[0] != fsid->val[0] ||
-	    root_fsid.val[1] != fsid->val[1])
-		return -EXDEV;
-
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Jan Kara]

On Thu 26-10-23 18:52:21, Amir Goldstein wrote:
> As agreed on the review of v1 [1], we do not need any vfs changes
> to support fanotify on btrfs sub-volumes and we can enable setting
> marks on btrfs sub-volumes simply by caching the fsid in the mark
> object instead of the connector.
> 
> This is the would be man page update to clarify the meaning of fsid
> as it is reflected in this patch set:
> 
> fsid
> 
>   This is a unique identifier of the filesystem containing the object
>   associated with the event.  It is a structure of type __kernel_fsid_t
>   and contains the same value reported in  f_fsid  when calling
>   statfs(2) with the same pathname argument that was used for
>   fanotify_mark(2).  Note that some filesystems (e.g., btrfs(5)) report
>   non-uniform values of f_fsid on different objects of the same filesystem.
>   In these cases, if fanotify_mark(2) is called several times with different
>   pathname values, the fsid value reported in events will match f_fsid
>   associated  with at least one of those pathname values.

Thanks! The patchset looks good to me but I don't want to queue it now so
shortly before the merge window opens. So I'll queue it into my tree once
I'll push out changes for the merge window.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Christoph Hellwig]

As per the discussion in the last round:

Hard-NAKed-by: Christoph Hellwig <hch@lst.de>

We need to solve the whole btrfs subvolume st_dev thing out properly
and not leak these details in fanotify.

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Amir Goldstein]

On Fri, Oct 27, 2023 at 8:46 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> As per the discussion in the last round:
>
> Hard-NAKed-by: Christoph Hellwig <hch@lst.de>
>
> We need to solve the whole btrfs subvolume st_dev thing out properly
> and not leak these details in fanotify.
>

With all due respect, your NACK is uncalled for.

Did you look at the patches?
Did you actually study what they do?
Please point out a single line of code that leaks details to fanotify
as you claim.

The "details" that fanotify reports and has reported since circa v5.1
are the same details available to any unprivileged user via calls
to statfs(2) and name_to_handle_at(2).

The v2 patches do not change anything in that regard.
This is an internal fanotify detail (whether we allow setting a
watch on an inode inside a sub-volume), which does not expose
any new details to usersapce. It has nothing to do with your
campaign to fix the btrfs non-uniform st_dev/f_fsid issue.

Thanks,
Amir.

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 09:03:43AM +0300, Amir Goldstein wrote:
> With all due respect, your NACK is uncalled for.

It abosolute is not.  We must not spread the broken btrfs behavior.

> The "details" that fanotify reports and has reported since circa v5.1
> are the same details available to any unprivileged user via calls
> to statfs(2) and name_to_handle_at(2).

Yes, and that's very broken.  btrfs sneaked this in and we need to
fix it.

Again:

Extra-hard-Nacked-by: Christoph Hellwig <hch@lst.de>

and I'm a little sad that you're even arguing for sneaking such broken
thing in.

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Amir Goldstein]

On Fri, Oct 27, 2023 at 9:08 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Oct 27, 2023 at 09:03:43AM +0300, Amir Goldstein wrote:
> > With all due respect, your NACK is uncalled for.
>
> It abosolute is not.  We must not spread the broken btrfs behavior.
>
> > The "details" that fanotify reports and has reported since circa v5.1
> > are the same details available to any unprivileged user via calls
> > to statfs(2) and name_to_handle_at(2).
>
> Yes, and that's very broken.  btrfs sneaked this in and we need to
> fix it.
>
> Again:
>
> Extra-hard-Nacked-by: Christoph Hellwig <hch@lst.de>
>
> and I'm a little sad that you're even arguing for sneaking such broken
> thing in.

OK. You are blaming me for attempting to sneak in a broken feature
and I have blamed you for trying take my patches hostage to
promote your agenda.
I hope that we are both wrong.

Suppose that we follow up on your plan to fix btrfs, that it is
all done and everyone is happy.

You said it yourself, that we cannot risk regressions by changing
the existing st_dev/f_fsid without an opt-in mount/mkfs option.
Is that correct?

If that is the case, fanotify will need to continue reporting the fsid's
exactly as the user observes them on the legacy btrfs filesystems.
The v2 patches I posted are required to make that possible.

However, I think we can reach a compromise here.
My main goal with the recent f_fsid series is to allow fanotify
to be used as (unprivileged) inotify drop-in replacement.

To do that, there is no need to allow btrfs sb/mount watch
on sub-volumes, so I don't mind keeping the ban on sb/mount
watches on btrfs sub-volume and relaxing only inode watch
inside btrfs sub-volume.

IOW, on legacy btrfs sub-volumes, users could use fanotify
to watch changes/access to a file and get events with the same
fsid that the user observed with statfs(2) on that file.

Migrating to new btrfs sub-volumes (i.e. with unique sb/vfsmount)
would enable the feature of fanotify sb/mount watch, so there is
one more incentive for the community to fix btrfs fsid's and for
users to actually make this migration.

Will that address your concerns?

Thanks,
Amir.

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 09:33:19AM +0300, Amir Goldstein wrote:
> OK. You are blaming me for attempting to sneak in a broken feature
> and I have blamed you for trying take my patches hostage to
> promote your agenda.

I'm not blaming you for anything.  But I absolutely reject spreading
this broken behavior to core.  That's why there is hard NAK on this
patchs. 

> 
> If that is the case, fanotify will need to continue reporting the fsid's
> exactly as the user observes them on the legacy btrfs filesystems.
> The v2 patches I posted are required to make that possible.

The point is tht you simply can't use fanotify on a btrfs file system
with the broken behavior.  That's what btrfs gets for doing this
broken behavior to start with.

Re: [PATCH v2 0/3] fanotify support for btrfs sub-volumes

[Jan Kara]

On Fri 27-10-23 00:30:29, Christoph Hellwig wrote:
> On Fri, Oct 27, 2023 at 09:33:19AM +0300, Amir Goldstein wrote:
> > OK. You are blaming me for attempting to sneak in a broken feature
> > and I have blamed you for trying take my patches hostage to
> > promote your agenda.
> 
> I'm not blaming you for anything.  But I absolutely reject spreading
> this broken behavior to core.  That's why there is hard NAK on this
> patchs. 
> 
> > 
> > If that is the case, fanotify will need to continue reporting the fsid's
> > exactly as the user observes them on the legacy btrfs filesystems.
> > The v2 patches I posted are required to make that possible.
> 
> The point is tht you simply can't use fanotify on a btrfs file system
> with the broken behavior.  That's what btrfs gets for doing this
> broken behavior to start with.

Well, fanotify was never disabled on btrfs and presumably there are users.
What we blocked (exactly out of caution to not spread questionable
behavior) is placing of marks using a path whose fsid (from statfs(2)) is
different from filesystem root fsid when using FID-mode fanotify group
(i.e. groups where we report fsid + fhandle for each event instead of open
file descriptor). So effectively people currently cannot place marks on
non-root subvolume paths of btrfs for such fanotify groups.

Now given it is uncertain how exactly will filesystems end up presenting
subvolumes to VFS (and consequently to userspace) I agree it is probably
premature to allow placing superblock or mount marks on such paths because
the meaning could change when btrfs changes its presentation and we'd be
just adding ourselves more headaches with backward compatibility for no
great reasons. But so far I see no good reason to keep forbidding adding
inode marks on such paths as Amir suggests. I'll think about that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR