File system wide monitoring

Split out the final bits of struct fsnotify_mark->flags for use by a backend.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

Changes since v1:
- turn consts into defines (jan)

---
 include/linux/fsnotify_backend.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

include/linux/fsnotify_backend.h +14 -0

index 1ce66748a2d2..ae1bd9f06808 100644
@@ -363,6 +363,20 @@ struct fsnotify_mark_connector {
        struct hlist_head list;
 };
 
+enum fsnotify_mark_bits {
+       FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY,
+       FSN_MARK_FL_BIT_ALIVE,
+       FSN_MARK_FL_BIT_ATTACHED,
+       FSN_MARK_PRIVATE_FLAGS,
+};
+
+#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY \
+       (1 << FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY)
+#define FSNOTIFY_MARK_FLAG_ALIVE \
+       (1 << FSN_MARK_FL_BIT_ALIVE)
+#define FSNOTIFY_MARK_FLAG_ATTACHED \
+       (1 << FSN_MARK_FL_BIT_ATTACHED)
+
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events

Quoted 39 lines · Gabriel Krisman Bertazi

Split out the final bits of struct fsnotify_mark->flags for use by a backend.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

Changes since v1:
- turn consts into defines (jan)

---
 include/linux/fsnotify_backend.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

include/linux/fsnotify_backend.h +14 -0

index 1ce66748a2d2..ae1bd9f06808 100644
@@ -363,6 +363,20 @@ struct fsnotify_mark_connector {
        struct hlist_head list;
 };
 
+enum fsnotify_mark_bits {
+       FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY,
+       FSN_MARK_FL_BIT_ALIVE,
+       FSN_MARK_FL_BIT_ATTACHED,
+       FSN_MARK_PRIVATE_FLAGS,
+};
+
+#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY \
+       (1 << FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY)
+#define FSNOTIFY_MARK_FLAG_ALIVE \
+       (1 << FSN_MARK_FL_BIT_ALIVE)
+#define FSNOTIFY_MARK_FLAG_ATTACHED \
+       (1 << FSN_MARK_FL_BIT_ATTACHED)
+
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events

@@ -398,9 +412,7 @@ struct fsnotify_mark {
        struct fsnotify_mark_connector *connector;

include/linux/fsnotify_backend.h struct fsnotify_mark { · line 415

Quoted 6 lines

include/linux/fsnotify_backend.h +1 -3

        /* Events types to ignore [mark->lock, group->mark_mutex] */
        __u32 ignored_mask;
-#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY 0x01
-#define FSNOTIFY_MARK_FLAG_ALIVE               0x02
-#define FSNOTIFY_MARK_FLAG_ATTACHED            0x04
+       /* Upper bits [31:PRIVATE_FLAGS] are reserved for backend usage */

I don't understand what [31:PRIVATE_FLAGS] means

Quoted 41 lines · Amir Goldstein

Quoted 39 lines · Gabriel Krisman Bertazi

Split out the final bits of struct fsnotify_mark->flags for use by a backend.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

Changes since v1:
- turn consts into defines (jan)

---
 include/linux/fsnotify_backend.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

include/linux/fsnotify_backend.h +14 -0

index 1ce66748a2d2..ae1bd9f06808 100644
@@ -363,6 +363,20 @@ struct fsnotify_mark_connector {
        struct hlist_head list;
 };
 
+enum fsnotify_mark_bits {
+       FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY,
+       FSN_MARK_FL_BIT_ALIVE,
+       FSN_MARK_FL_BIT_ATTACHED,
+       FSN_MARK_PRIVATE_FLAGS,
+};
+
+#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY \
+       (1 << FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY)
+#define FSNOTIFY_MARK_FLAG_ALIVE \
+       (1 << FSN_MARK_FL_BIT_ALIVE)
+#define FSNOTIFY_MARK_FLAG_ATTACHED \
+       (1 << FSN_MARK_FL_BIT_ATTACHED)
+
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events

@@ -398,9 +412,7 @@ struct fsnotify_mark {
        struct fsnotify_mark_connector *connector;

Quoted 8 lines

include/linux/fsnotify_backend.h struct fsnotify_mark { · line 415

Quoted 6 lines

include/linux/fsnotify_backend.h +1 -3

        /* Events types to ignore [mark->lock, group->mark_mutex] */
        __u32 ignored_mask;
-#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY 0x01
-#define FSNOTIFY_MARK_FLAG_ALIVE               0x02
-#define FSNOTIFY_MARK_FLAG_ATTACHED            0x04
+       /* Upper bits [31:PRIVATE_FLAGS] are reserved for backend usage */

I don't understand what [31:PRIVATE_FLAGS] means

I think it should be [FSN_MARK_PRIVATE_FLAGS:31] (identifying a range of bits). I'd maybe write just "Bits starting from FSN_MARK_PRIVATE_FLAGS are reserved for backend usage". With this fixed feel free to add:

FAN_FS_ERROR will require an error structure to be stored per mark. But, since FAN_FS_ERROR doesn't apply to inode/mount marks, it should suffice to only expose this information for superblock marks. Therefore, wrap this kind of marks into a container and plumb it for the future.

Reviewed-by Amir Goldstein <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- turn the flag bits into defines (jan)
- don't use zalloc for consistency (jan)
Changes since v2:
- Move mark initialization to fanotify_alloc_mark (Amir)

Changes since v1:
- Only extend superblock marks (Amir)

---
 fs/notify/fanotify/fanotify.c      | 10 ++++++--
 fs/notify/fanotify/fanotify.h      | 20 ++++++++++++++++
 fs/notify/fanotify/fanotify_user.c | 38 ++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 4 deletions(-)

fs/notify/fanotify/fanotify.c +8 -2

index 310246f8d3f1..c3eefe3f6494 100644
@@ -869,9 +869,15 @@ static void fanotify_freeing_mark(struct fsnotify_mark *mark,
 		dec_ucount(group->fanotify_data.ucounts, UCOUNT_FANOTIFY_MARKS);
 }
 
-static void fanotify_free_mark(struct fsnotify_mark *fsn_mark)
+static void fanotify_free_mark(struct fsnotify_mark *mark)
 {
-	kmem_cache_free(fanotify_mark_cache, fsn_mark);
+	if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
+		struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);
+
+		kmem_cache_free(fanotify_sb_mark_cache, fa_mark);
+	} else {
+		kmem_cache_free(fanotify_mark_cache, mark);
+	}
 }
 
 const struct fsnotify_ops fanotify_fsnotify_ops = {

fs/notify/fanotify/fanotify.h +20 -0

index 4a5e555dc3d2..3b11dd03df59 100644
@@ -6,6 +6,7 @@
 #include <linux/hashtable.h>
 
 extern struct kmem_cache *fanotify_mark_cache;
+extern struct kmem_cache *fanotify_sb_mark_cache;
 extern struct kmem_cache *fanotify_fid_event_cachep;
 extern struct kmem_cache *fanotify_path_event_cachep;
 extern struct kmem_cache *fanotify_perm_event_cachep;

@@ -129,6 +130,25 @@ static inline void fanotify_info_copy_name(struct fanotify_info *info,
 	       name->name);
 }
 
+enum fanotify_mark_bits {
+	FANOTIFY_MARK_FLAG_BIT_SB_MARK = FSN_MARK_PRIVATE_FLAGS,
+};
+
+#define FANOTIFY_MARK_FLAG_SB_MARK \
+	(1 << FANOTIFY_MARK_FLAG_BIT_SB_MARK)
+
+struct fanotify_sb_mark {
+	struct fsnotify_mark fsn_mark;
+};
+
+static inline
+struct fanotify_sb_mark *FANOTIFY_SB_MARK(struct fsnotify_mark *mark)
+{
+	WARN_ON(!(mark->flags & FANOTIFY_MARK_FLAG_SB_MARK));
+
+	return container_of(mark, struct fanotify_sb_mark, fsn_mark);
+}
+
 /*
  * Common structure for fanotify events. Concrete structs are allocated in
  * fanotify_handle_event() and freed when the information is retrieved by

fs/notify/fanotify/fanotify_user.c +36 -2

index 67b18dfe0025..c47a5a45c0d3 100644
@@ -99,6 +99,7 @@ struct ctl_table fanotify_table[] = {
 extern const struct fsnotify_ops fanotify_fsnotify_ops;
 
 struct kmem_cache *fanotify_mark_cache __read_mostly;
+struct kmem_cache *fanotify_sb_mark_cache __read_mostly;
 struct kmem_cache *fanotify_fid_event_cachep __read_mostly;
 struct kmem_cache *fanotify_path_event_cachep __read_mostly;
 struct kmem_cache *fanotify_perm_event_cachep __read_mostly;

@@ -915,6 +916,38 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 	return mask & ~oldmask;
 }
 
+static struct fsnotify_mark *fanotify_alloc_mark(struct fsnotify_group *group,
+						 unsigned int type)
+{
+	struct fanotify_sb_mark *sb_mark;
+	struct fsnotify_mark *mark;
+
+	switch (type) {
+	case FSNOTIFY_OBJ_TYPE_SB:
+		sb_mark = kmem_cache_alloc(fanotify_sb_mark_cache, GFP_KERNEL);
+		if (!sb_mark)
+			return NULL;
+		mark = &sb_mark->fsn_mark;
+		break;
+
+	case FSNOTIFY_OBJ_TYPE_INODE:
+	case FSNOTIFY_OBJ_TYPE_PARENT:
+	case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+		mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
+		break;
+	default:
+		WARN_ON(1);
+		return NULL;
+	}
+
+	fsnotify_init_mark(mark, group);
+
+	if (type == FSNOTIFY_OBJ_TYPE_SB)
+		mark->flags |= FANOTIFY_MARK_FLAG_SB_MARK;
+
+	return mark;
+}
+
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 						   fsnotify_connp_t *connp,
 						   unsigned int type,

@@ -933,13 +966,12 @@ 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);
+	mark = fanotify_alloc_mark(group, type);
 	if (!mark) {
 		ret = -ENOMEM;
 		goto out_dec_ucounts;
 	}
 
-	fsnotify_init_mark(mark, group);
 	ret = fsnotify_add_mark_locked(mark, connp, type, 0, fsid);
 	if (ret) {
 		fsnotify_put_mark(mark);

@@ -1497,6 +1529,8 @@ static int __init fanotify_user_setup(void)
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
 					 SLAB_PANIC|SLAB_ACCOUNT);
+	fanotify_sb_mark_cache = KMEM_CACHE(fanotify_sb_mark,
+					    SLAB_PANIC|SLAB_ACCOUNT);
 	fanotify_fid_event_cachep = KMEM_CACHE(fanotify_fid_event,
 					       SLAB_PANIC);
 	fanotify_path_event_cachep = KMEM_CACHE(fanotify_path_event,

Signature · 1 line

2.32.0

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

Quoted 36 lines · Gabriel Krisman Bertazi

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

fs/notify/fsnotify.c static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) · line 467

Quoted 7 lines

fs/notify/fsnotify.c +2 -2

@@ -455,8 +463,10 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *             @file_name is relative to
  * @file_name: optional file name associated with event
  * @inode:     optional inode associated with event -
- *             either @dir or @inode must be non-NULL.
- *             if both are non-NULL event may be reported to both.
+ *             If @dir and @inode are NULL, @data must have a type that
+ *             allows retrieving the file system associated with this

Irrelevant comment. sb must always be available from @data.

fs/notify/fsnotify.c int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, · line 496

Quoted 11 lines

fs/notify/fsnotify.c +3 -1

+ *             event.  if both are non-NULL event may be reported to
+ *             both.
  * @cookie:    inotify rename cookie
  */
 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,

@@ -483,7 +493,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
                 */
                parent = dir;
        }
-       sb = inode->i_sb;
+       sb = inode ? inode->i_sb : fsnotify_data_sb(data, data_type);

        const struct path *path = fsnotify_data_path(data, data_type);

+ const struct super_block *sb = fsnotify_data_sb(data, data_type);

All the games with @data @inode and @dir args are irrelevant to this. sb should always be available from @data and it does not matter if fsnotify_data_inode() is the same as @inode, @dir or neither. All those inodes are anyway on the same sb.

Quoted 61 lines · Amir Goldstein

Quoted 36 lines · Gabriel Krisman Bertazi

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

fs/notify/fsnotify.c static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) · line 467

Quoted 7 lines

fs/notify/fsnotify.c +2 -2

@@ -455,8 +463,10 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *             @file_name is relative to
  * @file_name: optional file name associated with event
  * @inode:     optional inode associated with event -
- *             either @dir or @inode must be non-NULL.
- *             if both are non-NULL event may be reported to both.
+ *             If @dir and @inode are NULL, @data must have a type that
+ *             allows retrieving the file system associated with this

Irrelevant comment. sb must always be available from @data.

fs/notify/fsnotify.c int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, · line 496

Quoted 11 lines

fs/notify/fsnotify.c +3 -1

+ *             event.  if both are non-NULL event may be reported to
+ *             both.
  * @cookie:    inotify rename cookie
  */
 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,

@@ -483,7 +493,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
                 */
                parent = dir;
        }
-       sb = inode->i_sb;
+       sb = inode ? inode->i_sb : fsnotify_data_sb(data, data_type);

        const struct path *path = fsnotify_data_path(data, data_type);

Quoted 6 lines

+ const struct super_block *sb = fsnotify_data_sb(data, data_type);

All the games with @data @inode and @dir args are irrelevant to this. sb should always be available from @data and it does not matter if fsnotify_data_inode() is the same as @inode, @dir or neither. All those inodes are anyway on the same sb.

Hi Amir,

I think this is actually necessary. I could identify at least one event (FS_CREATE | FS_ISDIR) where fsnotify is invoked with a NULL data field. In that case, fsnotify_dirent is called with a negative dentry from vfs_mkdir(). I'm not sure why exactly the dentry is negative after the

mkdir, but it would be possible we are racing with the file removal, I

guess? It might be a bug in fsnotify that this case even happen, but I'm not sure yet.

The easiest way around it is what I proposed: just use inode->i_sb if data is NULL. Since, as you said, data, inode and dir are all on the same superblock, it should work, I think.

Quoted 73 lines · Gabriel Krisman Bertazi

Quoted 61 lines · Amir Goldstein

Quoted 36 lines · Gabriel Krisman Bertazi

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

fs/notify/fsnotify.c static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) · line 467

Quoted 7 lines

fs/notify/fsnotify.c +2 -2

@@ -455,8 +463,10 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *             @file_name is relative to
  * @file_name: optional file name associated with event
  * @inode:     optional inode associated with event -
- *             either @dir or @inode must be non-NULL.
- *             if both are non-NULL event may be reported to both.
+ *             If @dir and @inode are NULL, @data must have a type that
+ *             allows retrieving the file system associated with this

Irrelevant comment. sb must always be available from @data.

fs/notify/fsnotify.c int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, · line 496

Quoted 11 lines

fs/notify/fsnotify.c +3 -1

+ *             event.  if both are non-NULL event may be reported to
+ *             both.
  * @cookie:    inotify rename cookie
  */
 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,

@@ -483,7 +493,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
                 */
                parent = dir;
        }
-       sb = inode->i_sb;
+       sb = inode ? inode->i_sb : fsnotify_data_sb(data, data_type);

        const struct path *path = fsnotify_data_path(data, data_type);

Quoted 6 lines

+ const struct super_block *sb = fsnotify_data_sb(data, data_type);

All the games with @data @inode and @dir args are irrelevant to this. sb should always be available from @data and it does not matter if fsnotify_data_inode() is the same as @inode, @dir or neither. All those inodes are anyway on the same sb.

Hi Amir,

Quoted 4 lines

I think this is actually necessary. I could identify at least one event (FS_CREATE | FS_ISDIR) where fsnotify is invoked with a NULL data field. In that case, fsnotify_dirent is called with a negative dentry from vfs_mkdir(). I'm not sure why exactly the dentry is negative after the

That doesn't sound right at all.
Are you sure about this?
Which filesystem was this mkdir called on?

Quoted 2 lines

mkdir, but it would be possible we are racing with the file removal, I

No. Both vfs_mkdir() and vfs_rmdir() hold the dir inode lock (on parent).

Quoted 2 lines

guess? It might be a bug in fsnotify that this case even happen, but I'm not sure yet.

fsnotify_data_inode() should not be NULL. fsnotify_handle_inode_event() passes fsnotify_data_inode() to callbacks like audit_watch_handle_event() that checks WARN_ON_ONCE(!inode)

Quoted 5 lines

The easiest way around it is what I proposed: just use inode->i_sb if data is NULL. Since, as you said, data, inode and dir are all on the same superblock, it should work, I think.

It would be papering over another issue.
I don't mind if we use inode->i_sb as long as we understand the reason for what you are reporting.

Please provide more information.

Quoted 75 lines · Amir Goldstein

Quoted 73 lines · Gabriel Krisman Bertazi

Quoted 61 lines · Amir Goldstein

Quoted 36 lines · Gabriel Krisman Bertazi

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

fs/notify/fsnotify.c static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) · line 467

Quoted 7 lines

fs/notify/fsnotify.c +2 -2

@@ -455,8 +463,10 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *             @file_name is relative to
  * @file_name: optional file name associated with event
  * @inode:     optional inode associated with event -
- *             either @dir or @inode must be non-NULL.
- *             if both are non-NULL event may be reported to both.
+ *             If @dir and @inode are NULL, @data must have a type that
+ *             allows retrieving the file system associated with this

Irrelevant comment. sb must always be available from @data.

fs/notify/fsnotify.c int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, · line 496

Quoted 11 lines

fs/notify/fsnotify.c +3 -1

+ *             event.  if both are non-NULL event may be reported to
+ *             both.
  * @cookie:    inotify rename cookie
  */
 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,

@@ -483,7 +493,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
                 */
                parent = dir;
        }
-       sb = inode->i_sb;
+       sb = inode ? inode->i_sb : fsnotify_data_sb(data, data_type);

        const struct path *path = fsnotify_data_path(data, data_type);

Quoted 6 lines

+ const struct super_block *sb = fsnotify_data_sb(data, data_type);

All the games with @data @inode and @dir args are irrelevant to this. sb should always be available from @data and it does not matter if fsnotify_data_inode() is the same as @inode, @dir or neither. All those inodes are anyway on the same sb.

Hi Amir,

Quoted 8 lines

Quoted 4 lines

I think this is actually necessary. I could identify at least one event (FS_CREATE | FS_ISDIR) where fsnotify is invoked with a NULL data field. In that case, fsnotify_dirent is called with a negative dentry from vfs_mkdir(). I'm not sure why exactly the dentry is negative after the

That doesn't sound right at all.
Are you sure about this?
Which filesystem was this mkdir called on?

You should be able to reproduce it on top of mainline if you pick only this patch and do the change you suggested:

- sb = inode->i_sb;
+ sb = fsnotify_data_sb(data, data_type);

And then boot a Debian stable with systemd. The notification happens on the cgroup pseudo-filesystem (/sys/fs/cgroup), which is being monitored by systemd itself. The event that arrives with a NULL data is telling the directory /sys/fs/cgroup/*/ about the creation of directory init.scope.

The change above triggers the following null dereference of struct super_block, since data is NULL.

I will keep looking but you might be able to answer it immediately...

Quoted 99 lines · Gabriel Krisman Bertazi

Quoted 75 lines · Amir Goldstein

Quoted 73 lines · Gabriel Krisman Bertazi

Quoted 61 lines · Amir Goldstein

Quoted 36 lines · Gabriel Krisman Bertazi

Some file system events (i.e. FS_ERROR) might not be associated with an inode. For these, it makes sense to associate them directly with the super block of the file system they apply to. This patch allows the event to be reported with a NULL inode, by recovering the superblock directly from the data field, if needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)

---
 fs/notify/fsnotify.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/notify/fsnotify.c +8 -0

index 30d422b8c0fc..536db02cb26e 100644
@@ -98,6 +98,14 @@ void fsnotify_sb_delete(struct super_block *sb)
        fsnotify_clear_marks_by_sb(sb);
 }
 
+static struct super_block *fsnotify_data_sb(const void *data, int data_type)
+{
+       struct inode *inode = fsnotify_data_inode(data, data_type);
+       struct super_block *sb = inode ? inode->i_sb : NULL;
+
+       return sb;
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event

fs/notify/fsnotify.c static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) · line 467

Quoted 7 lines

fs/notify/fsnotify.c +2 -2

@@ -455,8 +463,10 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *             @file_name is relative to
  * @file_name: optional file name associated with event
  * @inode:     optional inode associated with event -
- *             either @dir or @inode must be non-NULL.
- *             if both are non-NULL event may be reported to both.
+ *             If @dir and @inode are NULL, @data must have a type that
+ *             allows retrieving the file system associated with this

Irrelevant comment. sb must always be available from @data.

fs/notify/fsnotify.c int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, · line 496

Quoted 11 lines

fs/notify/fsnotify.c +3 -1

+ *             event.  if both are non-NULL event may be reported to
+ *             both.
  * @cookie:    inotify rename cookie
  */
 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,

@@ -483,7 +493,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
                 */
                parent = dir;
        }
-       sb = inode->i_sb;
+       sb = inode ? inode->i_sb : fsnotify_data_sb(data, data_type);

        const struct path *path = fsnotify_data_path(data, data_type);

Quoted 6 lines

+ const struct super_block *sb = fsnotify_data_sb(data, data_type);

All the games with @data @inode and @dir args are irrelevant to this. sb should always be available from @data and it does not matter if fsnotify_data_inode() is the same as @inode, @dir or neither. All those inodes are anyway on the same sb.

Hi Amir,

Quoted 8 lines

Quoted 4 lines

I think this is actually necessary. I could identify at least one event (FS_CREATE | FS_ISDIR) where fsnotify is invoked with a NULL data field. In that case, fsnotify_dirent is called with a negative dentry from vfs_mkdir(). I'm not sure why exactly the dentry is negative after the

That doesn't sound right at all.
Are you sure about this?
Which filesystem was this mkdir called on?

You should be able to reproduce it on top of mainline if you pick only this patch and do the change you suggested:

- sb = inode->i_sb;
+ sb = fsnotify_data_sb(data, data_type);

And then boot a Debian stable with systemd. The notification happens on the cgroup pseudo-filesystem (/sys/fs/cgroup), which is being monitored by systemd itself. The event that arrives with a NULL data is telling the directory /sys/fs/cgroup/*/ about the creation of directory init.scope.

Quoted 4 lines

The change above triggers the following null dereference of struct super_block, since data is NULL.

I will keep looking but you might be able to answer it immediately...

Yes, I see what is going on.

cgroupfs is a sort of kernfs and kernfs_iop_mkdir() does not instantiate the negative dentry. Instead, kernfs_dop_revalidate() always invalidates negative dentries to force re-lookup to find the inode.

Documentation/filesystems/vfs.rst says on create() and friends:
"...you will probably call d_instantiate() with the dentry and the
newly created inode..."

So this behavior seems legit.
Meaning that we have made a wrong assumption in fsnotify_create() and fsnotify_mkdir().
Please note the comment above fsnotify_link() which anticipates negative dentries.

I've audited the fsnotify backends and it seems that the WARN_ON(!inode) in kernel/audit_* is the only immediate implication of negative dentry with FS_CREATE.
I am the one who added these WARN_ON(), so I will remove them. I think that missing inode in an FS_CREATE event really breaks audit on kernfs, but not sure if that is a valid use case (Paul?).

Quoted 44 lines

Quoted 23 lines

Quoted 10 lines

Quoted 6 lines

Hi Amir,

I think this is actually necessary. I could identify at least one event (FS_CREATE | FS_ISDIR) where fsnotify is invoked with a NULL data field. In that case, fsnotify_dirent is called with a negative dentry from vfs_mkdir(). I'm not sure why exactly the dentry is negative after the

That doesn't sound right at all.
Are you sure about this?
Which filesystem was this mkdir called on?

You should be able to reproduce it on top of mainline if you pick only this patch and do the change you suggested:

- sb = inode->i_sb;
+ sb = fsnotify_data_sb(data, data_type);

And then boot a Debian stable with systemd. The notification happens on the cgroup pseudo-filesystem (/sys/fs/cgroup), which is being monitored by systemd itself. The event that arrives with a NULL data is telling the directory /sys/fs/cgroup/*/ about the creation of directory init.scope.

Quoted 4 lines

The change above triggers the following null dereference of struct super_block, since data is NULL.

I will keep looking but you might be able to answer it immediately...

Yes, I see what is going on.

cgroupfs is a sort of kernfs and kernfs_iop_mkdir() does not instantiate the negative dentry. Instead, kernfs_dop_revalidate() always invalidates negative dentries to force re-lookup to find the inode.

Documentation/filesystems/vfs.rst says on create() and friends:
"...you will probably call d_instantiate() with the dentry and the
newly created inode..."

So this behavior seems legit.
Meaning that we have made a wrong assumption in fsnotify_create() and fsnotify_mkdir().
Please note the comment above fsnotify_link() which anticipates negative dentries.

Quoted 6 lines

I've audited the fsnotify backends and it seems that the WARN_ON(!inode) in kernel/audit_* is the only immediate implication of negative dentry with FS_CREATE.
I am the one who added these WARN_ON(), so I will remove them. I think that missing inode in an FS_CREATE event really breaks audit on kernfs, but not sure if that is a valid use case (Paul?).

While it is tempting to ignore kernfs from an audit filesystem watch perspective, I can see admins potentially wanting to watch kernfs/cgroupfs/other-config-pseudofs to detect who is potentially playing with the system config. Arguably the most important config changes would already be audited if they were security relevant, but I could also see an admin wanting to watch for *any* changes so it's probably best not to rule out a kernfs based watch right now.

I'm sure I'm missing some details, but from what I gather from the portion of the thread that I'm seeing, it looks like the audit issue lies in audit_mark_handle_event() and audit_watch_handle_event(). In both cases it looks like the functions are at least safe with a NULL inode pointer, even with the WARN_ON() removed; the problem being that

the mark and watch will not be updated with the device and inode number which means the audit filters based on those marks/watches will not trigger. Is that about right or did I read the thread and code a bit too quickly?

Working under the assumption that the above is close enough to correct, that is a bit of a problem as it means audit can't effectively watch kernfs based filesystems, although it sounds like it wasn't really working properly to begin with, yes? Before I start

thinking too hard about this, does anyone already have a great idea to fix this that they want to share?

I did not check if it is possible or easy to d_instantiate() in kernfs mkdir() etc like other filesystems do and I do not know if it would be possible to enforce that as a strict vfs API rather than a recommendation.

Expose a new type of fsnotify event for filesystems to report errors for userspace monitoring tools. fanotify will send this type of notification for FAN_FS_ERROR events. This also introduce a helper for generating the new event.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!

Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)

Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)

---
 fs/notify/fsnotify.c             |  3 +++
 include/linux/fsnotify.h         | 13 +++++++++++++
 include/linux/fsnotify_backend.h | 18 +++++++++++++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

fs/notify/fsnotify.c +3 -0

index 536db02cb26e..6d3b3de4f8ee 100644
@@ -103,6 +103,9 @@ static struct super_block *fsnotify_data_sb(const void *data, int data_type)
        struct inode *inode = fsnotify_data_inode(data, data_type);
        struct super_block *sb = inode ? inode->i_sb : NULL;
 
+       if (!sb && data_type == FSNOTIFY_EVENT_ERROR)
+               sb = ((struct fs_error_report *) data)->sb;
+
        return sb;
 }
 

include/linux/fsnotify.h +13 -0

index f8acddcf54fb..521234af1827 100644
@@ -317,4 +317,17 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
                fsnotify_dentry(dentry, mask);
 }
 
+static inline int fsnotify_sb_error(struct super_block *sb, struct inode *inode,
+                                   int error)
+{
+       struct fs_error_report report = {
+               .error = error,
+               .inode = inode,
+               .sb = sb,
+       };
+
+       return fsnotify(FS_ERROR, &report, FSNOTIFY_EVENT_ERROR,
+                       NULL, NULL, NULL, 0);
+}
+
 #endif /* _LINUX_FS_NOTIFY_H */

include/linux/fsnotify_backend.h +17 -2

index e027af3cd8dd..277b6f3e0998 100644
@@ -42,6 +42,12 @@
 
 #define FS_UNMOUNT             0x00002000      /* inode on umount fs */
 #define FS_Q_OVERFLOW          0x00004000      /* Event queued overflowed */
+#define FS_ERROR               0x00008000      /* Filesystem Error (fanotify) */
+
+/*
+ * FS_IN_IGNORED overloads FS_ERROR.  It is only used internally by inotify
+ * which does not support FS_ERROR.
+ */
 #define FS_IN_IGNORED          0x00008000      /* last inotify event here */
 
 #define FS_OPEN_PERM           0x00010000      /* open event in an permission hook */

@@ -95,7 +101,8 @@
 #define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
                             FS_EVENTS_POSS_ON_CHILD | \
                             FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \
-                            FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED)
+                            FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
+                            FS_ERROR)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \

@@ -248,6 +255,13 @@ enum fsnotify_data_type {
        FSNOTIFY_EVENT_NONE,
        FSNOTIFY_EVENT_PATH,
        FSNOTIFY_EVENT_INODE,
+       FSNOTIFY_EVENT_ERROR,
+};
+
+struct fs_error_report {
+       int error;
+       struct inode *inode;
+       struct super_block *sb;
 };
 
 static inline struct inode *fsnotify_data_inode(const void *data, int data_type)

@@ -257,6 +271,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
                return (struct inode *)data;
        case FSNOTIFY_EVENT_PATH:
                return d_inode(((const struct path *)data)->dentry);
+       case FSNOTIFY_EVENT_ERROR:
+               return ((struct fs_error_report *)data)->inode;
        default:
                return NULL;
        }
--

2.32.0

Expose a new type of fsnotify event for filesystems to report errors for userspace monitoring tools. fanotify will send this type of notification for FAN_FS_ERROR events. This also introduce a helper for generating the new event.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!

Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)

Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)

---
 fs/notify/fsnotify.c             |  3 +++
 include/linux/fsnotify.h         | 13 +++++++++++++
 include/linux/fsnotify_backend.h | 18 +++++++++++++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

fs/notify/fsnotify.c +3 -0

index 536db02cb26e..6d3b3de4f8ee 100644
@@ -103,6 +103,9 @@ static struct super_block *fsnotify_data_sb(const void *data, int data_type)
 	struct inode *inode = fsnotify_data_inode(data, data_type);
 	struct super_block *sb = inode ? inode->i_sb : NULL;
 
+	if (!sb && data_type == FSNOTIFY_EVENT_ERROR)
+		sb = ((struct fs_error_report *) data)->sb;
+
 	return sb;
 }
 

include/linux/fsnotify.h +13 -0

index f8acddcf54fb..521234af1827 100644
@@ -317,4 +317,17 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
 		fsnotify_dentry(dentry, mask);
 }
 
+static inline int fsnotify_sb_error(struct super_block *sb, struct inode *inode,
+				    int error)
+{
+	struct fs_error_report report = {
+		.error = error,
+		.inode = inode,
+		.sb = sb,
+	};
+
+	return fsnotify(FS_ERROR, &report, FSNOTIFY_EVENT_ERROR,
+			NULL, NULL, NULL, 0);
+}
+
 #endif	/* _LINUX_FS_NOTIFY_H */

include/linux/fsnotify_backend.h +17 -1

index e027af3cd8dd..277b6f3e0998 100644
@@ -42,6 +42,12 @@
 
 #define FS_UNMOUNT		0x00002000	/* inode on umount fs */
 #define FS_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
+#define FS_ERROR		0x00008000	/* Filesystem Error (fanotify) */
+
+/*
+ * FS_IN_IGNORED overloads FS_ERROR.  It is only used internally by inotify
+ * which does not support FS_ERROR.
+ */
 #define FS_IN_IGNORED		0x00008000	/* last inotify event here */
 
 #define FS_OPEN_PERM		0x00010000	/* open event in an permission hook */

@@ -95,7 +101,8 @@
 #define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
 			     FS_EVENTS_POSS_ON_CHILD | \
 			     FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \
-			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED)
+			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
+			     FS_ERROR)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \

@@ -248,6 +255,13 @@ enum fsnotify_data_type {
 	FSNOTIFY_EVENT_NONE,
 	FSNOTIFY_EVENT_PATH,
 	FSNOTIFY_EVENT_INODE,
+	FSNOTIFY_EVENT_ERROR,
+};
+
+struct fs_error_report {
+	int error;
+	struct inode *inode;
+	struct super_block *sb;
 };
 
 static inline struct inode *fsnotify_data_inode(const void *data, int data_type)

@@ -257,6 +271,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
 		return (struct inode *)data;
 	case FSNOTIFY_EVENT_PATH:
 		return d_inode(((const struct path *)data)->dentry);
+	case FSNOTIFY_EVENT_ERROR:
+		return ((struct fs_error_report *)data)->inode;
 	default:
 		return NULL;
 	}

Signature · 1 line

2.32.0

FAN_FS_ERROR will report a file handle, but it is an unhashed event. Allow passing a NULL hash to fanotify_encode_fh and avoid calculating the hash if not needed.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

Reviewed-by Jan Kara <[email protected]>

---
 fs/notify/fanotify/fanotify.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +6 -3

index acf78c0ed219..50fce4fec0d6 100644
@@ -403,8 +403,12 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        fh->type = type;
        fh->len = fh_len;
 
-       /* Mix fh into event merge key */
-       *hash ^= fanotify_hash_fh(fh);
+       /*
+        * Mix fh into event merge key.  Hash might be NULL in case of
+        * unhashed FID events (i.e. FAN_FS_ERROR).
+        */
+       if (hash)
+               *hash ^= fanotify_hash_fh(fh);
 
        return FANOTIFY_FH_HDR_LEN + fh_len;
 
--

2.32.0

Instead of failing, encode an invalid file handle in fanotify_encode_fh if no inode is provided. This bogus file handle will be reported by FAN_FS_ERROR for non-inode errors.

When being reported to userspace, the length information is actually reset and the handle cleaned up, such that userspace don't have the visibility of the internal kernel representation of this null handle.

Also adjust the single caller that might rely on failure after passing an empty inode.

Suggested-by Amir Goldstein <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic

---
 fs/notify/fanotify/fanotify.c      | 27 ++++++++++++++++++-----
 fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
 2 files changed, 41 insertions(+), 21 deletions(-)

fs/notify/fanotify/fanotify.c +6 -5

index 50fce4fec0d6..2b1ab031fbe5 100644
@@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
        return test_mask & user_mask;
 }
 
+#define FANOTIFY_NULL_FH_LEN   4
+
 /*
  * Check size needed to encode fanotify_fh.
  *

@@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
        int dwords = 0;
 
        if (!inode)
-               return 0;
+               return FANOTIFY_NULL_FH_LEN;
 
        exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
 

@@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        void *buf = fh->buf;
        int err;
 
-       fh->type = FILEID_ROOT;
-       fh->len = 0;
+       BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 ||
+                    FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN);
+
        fh->flags = 0;
-       if (!inode)
-               return 0;

fs/notify/fanotify/fanotify.c +5 -1

+
+               goto success;
+       }
 
        /*
         * !gpf means preallocated variable size fh, but fh_len could

@@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
                goto out_err;
 
+success:
        fh->type = type;
        fh->len = fh_len;
 

@@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
        struct fanotify_info *info;
        struct fanotify_fh *dfh, *ffh;
        unsigned int dir_fh_len = fanotify_encode_fh_len(id);
-       unsigned int child_fh_len = fanotify_encode_fh_len(child);
+       unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0;
        unsigned int size;
 
        size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;

fs/notify/fanotify/fanotify_user.c +0 -0

index c47a5a45c0d3..4cacea5fcaca 100644

fs/notify/fanotify/fanotify_user.c +0 -0

        if (copy_to_user(buf, &handle, sizeof(handle)))
                return -EFAULT;
 

@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        if (WARN_ON_ONCE(len < fh_len))
                return -EFAULT;

fs/notify/fanotify/fanotify_user.c +14 -5

+               /*
+                * For an inline fh and inline file name, copy through
+                * stack to exclude the copy from usercopy hardening
+                * protections.
+                */
+               fh_buf = fanotify_fh_buf(fh);
+               if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+                       memcpy(bounce, fh_buf, fh_len);
+                       fh_buf = bounce;
+               }
+               if (copy_to_user(buf, fh_buf, fh_len))
+                       return -EFAULT;
+               buf += fh_len;
+               len -= fh_len;
        }
-       if (copy_to_user(buf, fh_buf, fh_len))
-               return -EFAULT;
-
-       buf += fh_len;
-       len -= fh_len;
 
        if (name_len) {
                /* Copy the filename with terminating null */

Quoted 61 lines · Gabriel Krisman Bertazi

Instead of failing, encode an invalid file handle in fanotify_encode_fh if no inode is provided. This bogus file handle will be reported by FAN_FS_ERROR for non-inode errors.

When being reported to userspace, the length information is actually reset and the handle cleaned up, such that userspace don't have the visibility of the internal kernel representation of this null handle.

Also adjust the single caller that might rely on failure after passing an empty inode.

Suggested-by Amir Goldstein <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic

---
 fs/notify/fanotify/fanotify.c      | 27 ++++++++++++++++++-----
 fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
 2 files changed, 41 insertions(+), 21 deletions(-)

fs/notify/fanotify/fanotify.c +6 -5

index 50fce4fec0d6..2b1ab031fbe5 100644
@@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
        return test_mask & user_mask;
 }
 
+#define FANOTIFY_NULL_FH_LEN   4
+
 /*
  * Check size needed to encode fanotify_fh.
  *

@@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
        int dwords = 0;
 
        if (!inode)
-               return 0;
+               return FANOTIFY_NULL_FH_LEN;
 
        exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
 

@@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        void *buf = fh->buf;
        int err;
 
-       fh->type = FILEID_ROOT;
-       fh->len = 0;
+       BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 ||
+                    FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN);
+
        fh->flags = 0;
-       if (!inode)
-               return 0;

fs/notify/fanotify/fanotify.c static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, · line 385

Quoted 10 lines

fs/notify/fanotify/fanotify.c +10 -0

+
+       if (!inode) {
+               /*
+                * Invalid FHs are used on FAN_FS_ERROR for errors not
+                * linked to any inode. The f_handle won't be reported
+                * back to userspace.  The extra bytes are cleared prior
+                * to reporting.
+                */
+               type = FILEID_INVALID;
+               fh_len = FANOTIFY_NULL_FH_LEN;

Please memset() the NULL_FH buffer to zero.

Quoted 28 lines

fs/notify/fanotify/fanotify.c +5 -1

+
+               goto success;
+       }
 
        /*
         * !gpf means preallocated variable size fh, but fh_len could

@@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
                goto out_err;
 
+success:
        fh->type = type;
        fh->len = fh_len;
 

@@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
        struct fanotify_info *info;
        struct fanotify_fh *dfh, *ffh;
        unsigned int dir_fh_len = fanotify_encode_fh_len(id);
-       unsigned int child_fh_len = fanotify_encode_fh_len(child);
+       unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0;
        unsigned int size;
 
        size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;

fs/notify/fanotify/fanotify_user.c +0 -0

index c47a5a45c0d3..4cacea5fcaca 100644

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 366

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c +4 -1

@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
                return -EFAULT;
 
        handle.handle_type = fh->type;
-       handle.handle_bytes = fh_len;
+
+       /* FILEID_INVALID handle type is reported without its f_handle. */
+       if (fh->type != FILEID_INVALID)
+               handle.handle_bytes = fh_len;

I know I suggested those exact lines, but looking at the patch, I think it would be better to do:

(quoted diff) +2 -0

+       if (fh->type != FILEID_INVALID)
+               fh_len = 0;
         handle.handle_bytes = fh_len;

Quoted 7 lines

fs/notify/fanotify/fanotify_user.c +0 -0

        if (copy_to_user(buf, &handle, sizeof(handle)))
                return -EFAULT;
 

@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        if (WARN_ON_ONCE(len < fh_len))
                return -EFAULT;

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 375

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +1 -8

 
-       /*
-        * For an inline fh and inline file name, copy through stack to exclude
-        * the copy from usercopy hardening protections.
-        */
-       fh_buf = fanotify_fh_buf(fh);
-       if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
-               memcpy(bounce, fh_buf, fh_len);
-               fh_buf = bounce;
+       if (fh->type != FILEID_INVALID) {

... and here: if (fh_len) {

Quoted 24 lines

fs/notify/fanotify/fanotify_user.c +14 -5

+               /*
+                * For an inline fh and inline file name, copy through
+                * stack to exclude the copy from usercopy hardening
+                * protections.
+                */
+               fh_buf = fanotify_fh_buf(fh);
+               if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+                       memcpy(bounce, fh_buf, fh_len);
+                       fh_buf = bounce;
+               }
+               if (copy_to_user(buf, fh_buf, fh_len))
+                       return -EFAULT;
+               buf += fh_len;
+               len -= fh_len;
        }
-       if (copy_to_user(buf, fh_buf, fh_len))
-               return -EFAULT;
-
-       buf += fh_len;
-       len -= fh_len;
 
        if (name_len) {
                /* Copy the filename with terminating null */

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 406

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +1 -1

@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        }
 
        /* Pad with 0's */
-       WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);
+       WARN_ON_ONCE(len < 0);

According to my calculations, FAN_FS_ERROR event with NULL_FH is expected to get here with len == 4, so you can change this to:

         WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN);

But first, I would like to get Jan's feedback on this concept of keeping unneeded 4 bytes zero padding in reported event in case of NULL_FH in order to keep the FID reporting code simpler.

Quoted 170 lines · Amir Goldstein

Quoted 61 lines · Gabriel Krisman Bertazi

Instead of failing, encode an invalid file handle in fanotify_encode_fh if no inode is provided. This bogus file handle will be reported by FAN_FS_ERROR for non-inode errors.

When being reported to userspace, the length information is actually reset and the handle cleaned up, such that userspace don't have the visibility of the internal kernel representation of this null handle.

Also adjust the single caller that might rely on failure after passing an empty inode.

Suggested-by Amir Goldstein <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic

---
 fs/notify/fanotify/fanotify.c      | 27 ++++++++++++++++++-----
 fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
 2 files changed, 41 insertions(+), 21 deletions(-)

fs/notify/fanotify/fanotify.c +6 -5

index 50fce4fec0d6..2b1ab031fbe5 100644
@@ -334,6 +334,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
        return test_mask & user_mask;
 }
 
+#define FANOTIFY_NULL_FH_LEN   4
+
 /*
  * Check size needed to encode fanotify_fh.
  *

@@ -345,7 +347,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
        int dwords = 0;
 
        if (!inode)
-               return 0;
+               return FANOTIFY_NULL_FH_LEN;
 
        exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
 

@@ -367,11 +369,23 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        void *buf = fh->buf;
        int err;
 
-       fh->type = FILEID_ROOT;
-       fh->len = 0;
+       BUILD_BUG_ON(FANOTIFY_NULL_FH_LEN < 4 ||
+                    FANOTIFY_NULL_FH_LEN > FANOTIFY_INLINE_FH_LEN);
+
        fh->flags = 0;
-       if (!inode)
-               return 0;

fs/notify/fanotify/fanotify.c static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, · line 385

Quoted 10 lines

fs/notify/fanotify/fanotify.c +10 -0

+
+       if (!inode) {
+               /*
+                * Invalid FHs are used on FAN_FS_ERROR for errors not
+                * linked to any inode. The f_handle won't be reported
+                * back to userspace.  The extra bytes are cleared prior
+                * to reporting.
+                */
+               type = FILEID_INVALID;
+               fh_len = FANOTIFY_NULL_FH_LEN;

Please memset() the NULL_FH buffer to zero.

Quoted 28 lines

fs/notify/fanotify/fanotify.c +5 -1

+
+               goto success;
+       }
 
        /*
         * !gpf means preallocated variable size fh, but fh_len could

@@ -400,6 +414,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
        if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
                goto out_err;
 
+success:
        fh->type = type;
        fh->len = fh_len;
 

@@ -529,7 +544,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
        struct fanotify_info *info;
        struct fanotify_fh *dfh, *ffh;
        unsigned int dir_fh_len = fanotify_encode_fh_len(id);
-       unsigned int child_fh_len = fanotify_encode_fh_len(child);
+       unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0;
        unsigned int size;
 
        size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;

fs/notify/fanotify/fanotify_user.c +0 -0

index c47a5a45c0d3..4cacea5fcaca 100644

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 366

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c +4 -1

@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
                return -EFAULT;
 
        handle.handle_type = fh->type;
-       handle.handle_bytes = fh_len;
+
+       /* FILEID_INVALID handle type is reported without its f_handle. */
+       if (fh->type != FILEID_INVALID)
+               handle.handle_bytes = fh_len;

I know I suggested those exact lines, but looking at the patch, I think it would be better to do:

(quoted diff) +2 -0

+       if (fh->type != FILEID_INVALID)
+               fh_len = 0;
         handle.handle_bytes = fh_len;

Quoted 7 lines

fs/notify/fanotify/fanotify_user.c +0 -0

        if (copy_to_user(buf, &handle, sizeof(handle)))
                return -EFAULT;
 

@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        if (WARN_ON_ONCE(len < fh_len))
                return -EFAULT;

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 375

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +1 -8

 
-       /*
-        * For an inline fh and inline file name, copy through stack to exclude
-        * the copy from usercopy hardening protections.
-        */
-       fh_buf = fanotify_fh_buf(fh);
-       if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
-               memcpy(bounce, fh_buf, fh_len);
-               fh_buf = bounce;
+       if (fh->type != FILEID_INVALID) {

... and here: if (fh_len) {

Quoted 24 lines

fs/notify/fanotify/fanotify_user.c +14 -5

+               /*
+                * For an inline fh and inline file name, copy through
+                * stack to exclude the copy from usercopy hardening
+                * protections.
+                */
+               fh_buf = fanotify_fh_buf(fh);
+               if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+                       memcpy(bounce, fh_buf, fh_len);
+                       fh_buf = bounce;
+               }
+               if (copy_to_user(buf, fh_buf, fh_len))
+                       return -EFAULT;
+               buf += fh_len;
+               len -= fh_len;
        }
-       if (copy_to_user(buf, fh_buf, fh_len))
-               return -EFAULT;
-
-       buf += fh_len;
-       len -= fh_len;
 
        if (name_len) {
                /* Copy the filename with terminating null */

fs/notify/fanotify/fanotify_user.c static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 406

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +1 -1

@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        }
 
        /* Pad with 0's */
-       WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);
+       WARN_ON_ONCE(len < 0);

According to my calculations, FAN_FS_ERROR event with NULL_FH is expected to get here with len == 4, so you can change this to:

Quoted 5 lines

         WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN);

But first, I would like to get Jan's feedback on this concept of keeping unneeded 4 bytes zero padding in reported event in case of NULL_FH in order to keep the FID reporting code simpler.

Dunno, it still seems like quite some complications (simple ones but non-trivial amount of them) for what is rather a corner case. What if we *internally* propagated the information that there's no inode info with FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(), fanotify_encode_fh(), or fanotify_alloc_name_event(). In copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's all. No useless padding, no specialcasing of copying etc. Am I missing something?

Quoted 8 lines · Jan Kara

Dunno, it still seems like quite some complications (simple ones but non-trivial amount of them) for what is rather a corner case. What if we *internally* propagated the information that there's no inode info with FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(), fanotify_encode_fh(), or fanotify_alloc_name_event(). In copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's all. No useless padding, no specialcasing of copying etc. Am I missing something?

I am perfectly fine with encoding "no inode" with FILEID_ROOT internally. It's already the value used by fanotify_encode_fh() in upstream.

However, if we use zero len internally, we need to pass fh_type to fanotify_fid_info_len() and special case FILEID_ROOT in order to take FANOTIFY_FID_INFO_HDR_LEN into account.

And special case fanotify_event_object_fh_len() in
fanotify_event_info_len() and in copy_info_records_to_user().

Like inode events, FAN_FS_ERROR will require fid mode. Therefore, convert the verification during fanotify_mark(2) to require fid for any non-fd event. This means fid_mode will not only be required for inode events, but for any event that doesn't provide a descriptor.

Suggested-by Amir Goldstein <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
changes since v5:
- Fix condition to include FANOTIFY_EVENT_FLAGS. (me)
- Fix comment identation (jan)

---
 fs/notify/fanotify/fanotify_user.c | 12 ++++++------
 include/linux/fanotify.h           |  3 +++
 2 files changed, 9 insertions(+), 6 deletions(-)

fs/notify/fanotify/fanotify_user.c +6 -6

index 4cacea5fcaca..54107f1533d5 100644
@@ -1387,14 +1387,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
                goto fput_and_out;
 
        /*
-        * Events with data type inode do not carry enough information to report
-        * event->fd, so we do not allow setting a mask for inode events unless
-        * group supports reporting fid.
-        * inode events are not supported on a mount mark, because they do not
-        * carry enough information (i.e. path) to be filtered by mount point.
+        * Events that do not carry enough information to report
+        * event->fd require a group that supports reporting fid.  Those
+        * events are not supported on a mount mark, because they do not
+        * carry enough information (i.e. path) to be filtered by mount
+        * point.
         */
        fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
-       if (mask & FANOTIFY_INODE_EVENTS &&
+       if (mask & ~(FANOTIFY_FD_EVENTS|FANOTIFY_EVENT_FLAGS) &&
            (!fid_mode || mark_type == FAN_MARK_MOUNT))
                goto fput_and_out;
 

include/linux/fanotify.h +3 -1

index a16dbeced152..c05d45bde8b8 100644
@@ -81,6 +81,9 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
  */
 #define FANOTIFY_DIRENT_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE)
 
+/* Events that can be reported with event->fd */
+#define FANOTIFY_FD_EVENTS (FANOTIFY_PATH_EVENTS | FANOTIFY_PERM_EVENTS)
+
 /* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */
 #define FANOTIFY_INODE_EVENTS  (FANOTIFY_DIRENT_EVENTS | \
                                 FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF)
--

2.32.0

FAN_FS_ERROR allows reporting of event type FS_ERROR to userspace, which a mechanism to report file system wide problems via fanotify. This commit preallocate userspace visible bits to match the FS_ERROR event.

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
 fs/notify/fanotify/fanotify.c | 1 +
 include/uapi/linux/fanotify.h | 1 +
 2 files changed, 2 insertions(+)

fs/notify/fanotify/fanotify.c +1 -0

index 2b1ab031fbe5..ebb6c557cea1 100644
@@ -760,6 +760,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
        BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
        BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
        BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
+       BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
 
        BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
 

include/uapi/linux/fanotify.h +1 -1

index fbf9c5c7dd59..16402037fc7a 100644
@@ -20,6 +20,7 @@
 #define FAN_OPEN_EXEC          0x00001000      /* File was opened for exec */
 
 #define FAN_Q_OVERFLOW         0x00004000      /* Event queued overflowed */
+#define FAN_FS_ERROR           0x00008000      /* Filesystem error */
 
 #define FAN_OPEN_PERM          0x00010000      /* File open in perm check */
 #define FAN_ACCESS_PERM                0x00020000      /* File accessed in perm check */
--

2.32.0

Error reporting needs to be done in an atomic context. This patch introduces a single error slot for superblock marks that report the FAN_FS_ERROR event, to be used during event submission.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)

---
 fs/notify/fanotify/fanotify.c      | 12 ++++++++++++
 fs/notify/fanotify/fanotify.h      | 13 +++++++++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +12 -0

index ebb6c557cea1..3bf6fd85c634 100644
@@ -855,6 +855,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
        kfree(FANOTIFY_NE(event));
 }
 
+static void fanotify_free_error_event(struct fanotify_event *event)
+{
+       /*
+        * The actual event is tied to a mark, and is released on mark
+        * removal
+        */
+}
+
 static void fanotify_free_event(struct fsnotify_event *fsn_event)
 {
        struct fanotify_event *event;

@@ -877,6 +885,9 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
        case FANOTIFY_EVENT_TYPE_OVERFLOW:
                kfree(event);
                break;
+       case FANOTIFY_EVENT_TYPE_FS_ERROR:
+               fanotify_free_error_event(event);
+               break;
        default:
                WARN_ON_ONCE(1);
        }

@@ -894,6 +905,7 @@ static void fanotify_free_mark(struct fsnotify_mark *mark)
        if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
                struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);
 
+               kfree(fa_mark->fee_slot);
                kmem_cache_free(fanotify_sb_mark_cache, fa_mark);
        } else {
                kmem_cache_free(fanotify_mark_cache, mark);

fs/notify/fanotify/fanotify.h +13 -0

index b3ab620822c2..3f03333df32f 100644
@@ -139,6 +139,7 @@ enum fanotify_mark_bits {
 
 struct fanotify_sb_mark {
        struct fsnotify_mark fsn_mark;
+       struct fanotify_error_event *fee_slot;
 };
 
 static inline

@@ -161,6 +162,7 @@ enum fanotify_event_type {
        FANOTIFY_EVENT_TYPE_PATH,
        FANOTIFY_EVENT_TYPE_PATH_PERM,
        FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
+       FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */
        __FANOTIFY_EVENT_TYPE_NUM
 };
 

@@ -216,6 +218,17 @@ FANOTIFY_NE(struct fanotify_event *event)
        return container_of(event, struct fanotify_name_event, fae);
 }
 
+struct fanotify_error_event {
+       struct fanotify_event fae;
+       struct fanotify_sb_mark *sb_mark; /* Back reference to the mark. */
+};
+
+static inline struct fanotify_error_event *
+FANOTIFY_EE(struct fanotify_event *event)
+{
+       return container_of(event, struct fanotify_error_event, fae);
+}
+
 static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
 {
        if (event->type == FANOTIFY_EVENT_TYPE_FID)

fs/notify/fanotify/fanotify_user.c +12 -1

index 54107f1533d5..b77030386d7f 100644
@@ -947,8 +947,10 @@ static struct fsnotify_mark *fanotify_alloc_mark(struct fsnotify_group *group,
 
        fsnotify_init_mark(mark, group);
 
-       if (type == FSNOTIFY_OBJ_TYPE_SB)
+       if (type == FSNOTIFY_OBJ_TYPE_SB) {
                mark->flags |= FANOTIFY_MARK_FLAG_SB_MARK;
+               sb_mark->fee_slot = NULL;
+       }
 
        return mark;
 }

@@ -999,6 +1001,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 {
        struct fsnotify_mark *fsn_mark;
        __u32 added;
+       int ret = 0;
 
        mutex_lock(&group->mark_mutex);
        fsn_mark = fsnotify_find_mark(connp, group);

@@ -1009,13 +1012,37 @@ static int fanotify_add_mark(struct fsnotify_group *group,
                        return PTR_ERR(fsn_mark);
                }
        }
+
+       /*
+        * Error events are allocated per super-block mark only if
+        * strictly needed (i.e. FAN_FS_ERROR was requested).
+        */
+       if (type == FSNOTIFY_OBJ_TYPE_SB && !(flags & FAN_MARK_IGNORED_MASK) &&
+           (mask & FAN_FS_ERROR)) {
+               struct fanotify_sb_mark *sb_mark = FANOTIFY_SB_MARK(fsn_mark);

Error reporting needs to be done in an atomic context. This patch introduces a single error slot for superblock marks that report the FAN_FS_ERROR event, to be used during event submission.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)

---
 fs/notify/fanotify/fanotify.c      | 12 ++++++++++++
 fs/notify/fanotify/fanotify.h      | 13 +++++++++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +0 -0

index ebb6c557cea1..3bf6fd85c634 100644
@@ -855,6 +855,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 	kfree(FANOTIFY_NE(event));

fs/notify/fanotify/fanotify.c +2 -0

@@ -1009,13 +1012,37 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			return PTR_ERR(fsn_mark);
 		}
 	}
+
+	/*

Quoted 23 lines · Gabriel Krisman Bertazi

Error reporting needs to be done in an atomic context. This patch introduces a single error slot for superblock marks that report the FAN_FS_ERROR event, to be used during event submission.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)

---
 fs/notify/fanotify/fanotify.c      | 12 ++++++++++++
 fs/notify/fanotify/fanotify.h      | 13 +++++++++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +0 -0

index ebb6c557cea1..3bf6fd85c634 100644
@@ -855,6 +855,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 	kfree(FANOTIFY_NE(event));

fs/notify/fanotify/fanotify.c static void fanotify_free_name_event(struct fanotify_event *event) · line 865

Quoted 10 lines

fs/notify/fanotify/fanotify.c +8 -0

 }
 
+static void fanotify_free_error_event(struct fanotify_event *event)
+{
+	/*
+	 * The actual event is tied to a mark, and is released on mark
+	 * removal
+	 */
+}
+

I was pondering about the lifetime rules some more. This is also related to patch 16/21 but I'll comment here. When we hold mark ref from queued event, we introduce a subtle race into group destruction logic. There we first evict all marks, wait for them to be destroyed by worker thread after SRCU period expires, and then we remove queued events. When we hold mark reference from an event we break this as mark will exist until the event is dequeued and then group can get freed before we actually free the mark and so mark freeing can hit use-after-free issues.

So we'll have to do this a bit differently. I have two options:

1) Instead of preallocating events explicitely like this, we could setup a mempool to allocate error events from for each notification group. We would resize the mempool when adding error mark so that it has as many reserved events as error marks. Upside is error events will be much less special - no special lifetime rules. We'd just need to setup & resize the mempool. We would also have to provide proper merge function for error events (to merge events from the same sb). Also there will be limitation of number of error marks per group because mempools use kmalloc() for an array tracking reserved events. But we could certainly manage 512, likely 1024 error marks per notification group.

2) We would keep attaching event to mark as currently. As far as I have checked the event doesn't actually need a back-ref to sb_mark. It is really only used for mark reference taking (and then to get to sb from fanotify_handle_error_event() but we can certainly get to sb by easier means there). So I would just remove that. What we still need to know in fanotify_free_error_event() though is whether the sb_mark is still alive or not. If it is alive, we leave the event alone, otherwise we need to free it. So we need a mark_alive flag in the error event and then do in ->freeing_mark callback something like:

	if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
		struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);

### /* Maybe we could use mark->lock for this? */

		spin_lock(&group->notification_lock);
		if (fa_mark->fee_slot) {
			if (list_empty(&fa_mark->fee_slot->fae.fse.list)) {
				kfree(fa_mark->fee_slot);
				fa_mark->fee_slot = NULL;
			} else {
				fa_mark->fee_slot->mark_alive = 0;
			}
		}
		spin_unlock(&group->notification_lock);
	}

And then when queueing and dequeueing event we would have to carefully check what is the mark & event state under appropriate lock (because ->handle_event() callbacks can see marks on the way to be destroyed as they are protected just by SRCU).

Quoted 83 lines · Jan Kara

Quoted 23 lines · Gabriel Krisman Bertazi

Error reporting needs to be done in an atomic context. This patch introduces a single error slot for superblock marks that report the FAN_FS_ERROR event, to be used during event submission.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)

---
 fs/notify/fanotify/fanotify.c      | 12 ++++++++++++
 fs/notify/fanotify/fanotify.h      | 13 +++++++++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +0 -0

index ebb6c557cea1..3bf6fd85c634 100644
@@ -855,6 +855,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 	kfree(FANOTIFY_NE(event));

fs/notify/fanotify/fanotify.c static void fanotify_free_name_event(struct fanotify_event *event) · line 865

Quoted 10 lines

fs/notify/fanotify/fanotify.c +8 -0

 }
 
+static void fanotify_free_error_event(struct fanotify_event *event)
+{
+	/*
+	 * The actual event is tied to a mark, and is released on mark
+	 * removal
+	 */
+}
+

I was pondering about the lifetime rules some more. This is also related to patch 16/21 but I'll comment here. When we hold mark ref from queued event, we introduce a subtle race into group destruction logic. There we first evict all marks, wait for them to be destroyed by worker thread after SRCU period expires, and then we remove queued events. When we hold mark reference from an event we break this as mark will exist until the event is dequeued and then group can get freed before we actually free the mark and so mark freeing can hit use-after-free issues.

So we'll have to do this a bit differently. I have two options:

1) Instead of preallocating events explicitely like this, we could setup a mempool to allocate error events from for each notification group. We would resize the mempool when adding error mark so that it has as many reserved events as error marks. Upside is error events will be much less special - no special lifetime rules. We'd just need to setup & resize the mempool. We would also have to provide proper merge function for error events (to merge events from the same sb). Also there will be limitation of number of error marks per group because mempools use kmalloc() for an array tracking reserved events. But we could certainly manage 512, likely 1024 error marks per notification group.

2) We would keep attaching event to mark as currently. As far as I have checked the event doesn't actually need a back-ref to sb_mark. It is really only used for mark reference taking (and then to get to sb from fanotify_handle_error_event() but we can certainly get to sb by easier means there). So I would just remove that. What we still need to know in fanotify_free_error_event() though is whether the sb_mark is still alive or not. If it is alive, we leave the event alone, otherwise we need to free it. So we need a mark_alive flag in the error event and then do in ->freeing_mark callback something like:

	if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
		struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);

### /* Maybe we could use mark->lock for this? */

		spin_lock(&group->notification_lock);
		if (fa_mark->fee_slot) {
			if (list_empty(&fa_mark->fee_slot->fae.fse.list)) {
				kfree(fa_mark->fee_slot);
				fa_mark->fee_slot = NULL;
			} else {
				fa_mark->fee_slot->mark_alive = 0;
			}
		}
		spin_unlock(&group->notification_lock);
	}

Quoted 4 lines

And then when queueing and dequeueing event we would have to carefully check what is the mark & event state under appropriate lock (because ->handle_event() callbacks can see marks on the way to be destroyed as they are protected just by SRCU).

Thanks for the review. That is indeed a subtle race that I hadn't noticed.

Option 2 is much more straightforward. And considering the uABI won't be changed if we decide to change to option 1 later, I gave that a try and should be able to prepare a new version that leaves the error event with a weak association to the mark, without the back reference, and allowing it to be deleted by the latest between dequeue and ->freeing_mark, as you suggested.

Quoted 85 lines · Gabriel Krisman Bertazi

Quoted 83 lines · Jan Kara

Quoted 23 lines · Gabriel Krisman Bertazi

Error reporting needs to be done in an atomic context. This patch introduces a single error slot for superblock marks that report the FAN_FS_ERROR event, to be used during event submission.

Signed-off-by Gabriel Krisman Bertazi <[email protected]>

---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)

---
 fs/notify/fanotify/fanotify.c      | 12 ++++++++++++
 fs/notify/fanotify/fanotify.h      | 13 +++++++++++++
 fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify.c +0 -0

index ebb6c557cea1..3bf6fd85c634 100644
@@ -855,6 +855,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
    kfree(FANOTIFY_NE(event));

fs/notify/fanotify/fanotify.c static void fanotify_free_name_event(struct fanotify_event *event) · line 865

Quoted 10 lines

fs/notify/fanotify/fanotify.c +8 -0

 }
 
+static void fanotify_free_error_event(struct fanotify_event *event)
+{
+   /*
+    * The actual event is tied to a mark, and is released on mark
+    * removal
+    */
+}
+

I was pondering about the lifetime rules some more. This is also related to patch 16/21 but I'll comment here. When we hold mark ref from queued event, we introduce a subtle race into group destruction logic. There we first evict all marks, wait for them to be destroyed by worker thread after SRCU period expires, and then we remove queued events. When we hold mark reference from an event we break this as mark will exist until the event is dequeued and then group can get freed before we actually free the mark and so mark freeing can hit use-after-free issues.

So we'll have to do this a bit differently. I have two options:

1) Instead of preallocating events explicitely like this, we could setup a mempool to allocate error events from for each notification group. We would resize the mempool when adding error mark so that it has as many reserved events as error marks. Upside is error events will be much less special - no special lifetime rules. We'd just need to setup & resize the mempool. We would also have to provide proper merge function for error events (to merge events from the same sb). Also there will be limitation of number of error marks per group because mempools use kmalloc() for an array tracking reserved events. But we could certainly manage 512, likely 1024 error marks per notification group.

2) We would keep attaching event to mark as currently. As far as I have checked the event doesn't actually need a back-ref to sb_mark. It is really only used for mark reference taking (and then to get to sb from fanotify_handle_error_event() but we can certainly get to sb by easier means there). So I would just remove that. What we still need to know in fanotify_free_error_event() though is whether the sb_mark is still alive or not. If it is alive, we leave the event alone, otherwise we need to free it. So we need a mark_alive flag in the error event and then do in ->freeing_mark callback something like:

     if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
             struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);

### /* Maybe we could use mark->lock for this? */

             spin_lock(&group->notification_lock);
             if (fa_mark->fee_slot) {
                     if (list_empty(&fa_mark->fee_slot->fae.fse.list)) {
                             kfree(fa_mark->fee_slot);
                             fa_mark->fee_slot = NULL;
                     } else {
                             fa_mark->fee_slot->mark_alive = 0;
                     }
             }
             spin_unlock(&group->notification_lock);
     }

Quoted 1 line

Quoted 1 line

And then when queueing and dequeueing event we would have to carefully

Quoted 7 lines

Quoted 3 lines

check what is the mark & event state under appropriate lock (because ->handle_event() callbacks can see marks on the way to be destroyed as they are protected just by SRCU).

Thanks for the review. That is indeed a subtle race that I hadn't noticed.

Quoted 6 lines

Option 2 is much more straightforward. And considering the uABI won't be changed if we decide to change to option 1 later, I gave that a try and should be able to prepare a new version that leaves the error event with a weak association to the mark, without the back reference, and allowing it to be deleted by the latest between dequeue and ->freeing_mark, as you suggested.

Actually, I don't think this will work for insertion unless we keep a bounce buffer for the file_handle, because we need to keep the group->notification_lock to ensure the fee doesn't go away with the mark (since it is not yet enqueued) but, as discussed before, we don't want to hold that lock when generating the FH.

I think the correct way is to have some sort of refcount of the error event slot. We could use err_count for that and change the suggestion above to:

if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
        struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);

        spin_lock(&group->notification_lock);
        if (fa_mark->fee_slot) {
                if (!fee->err_count) {
                        kfree(fa_mark->fee_slot);
                        fa_mark->fee_slot = NULL;
                } else {
                        fa_mark->fee_slot->mark_alive = 0;
                }
        }
        spin_unlock(&group->notification_lock);
}

And insertion would look like this:

static int fanotify_handle_error_event(....) {

        spin_lock(&group->notification_lock);

        if (!mark->fee || (mark->fee->err_count++) {
                spin_unlock(&group->notification_lock);
                return 0;
        }

        spin_unlock(&group->notification_lock);

        mark->fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;

        /* ... Write report data to error event ... */

        fanotify_encode_fh(&fee->object_fh, fanotify_encode_fh_len(inode),
                           NULL, 0);

        fsnotify_add_event(group, &fee->fae.fse, NULL);

}

Unless you think this is too hack-ish.

To be fair, I think it is hack-ish.

I would add a proper refcount_t
to the error event, and let the mark own a reference to it, which is dropped when the mark goes away. Enqueue and Dequeue will acquire and drop references, respectively. In this case, err_count is not overloaded.

Will it work?

Quoted 46 lines · Gabriel Krisman Bertazi

Actually, I don't think this will work for insertion unless we keep a bounce buffer for the file_handle, because we need to keep the group->notification_lock to ensure the fee doesn't go away with the mark (since it is not yet enqueued) but, as discussed before, we don't want to hold that lock when generating the FH.

I think the correct way is to have some sort of refcount of the error event slot. We could use err_count for that and change the suggestion above to:

if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
        struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);

        spin_lock(&group->notification_lock);
        if (fa_mark->fee_slot) {
                if (!fee->err_count) {
                        kfree(fa_mark->fee_slot);
                        fa_mark->fee_slot = NULL;
                } else {
                        fa_mark->fee_slot->mark_alive = 0;
                }
        }
        spin_unlock(&group->notification_lock);
}

And insertion would look like this:

static int fanotify_handle_error_event(....) {

        spin_lock(&group->notification_lock);

        if (!mark->fee || (mark->fee->err_count++) {
                spin_unlock(&group->notification_lock);
                return 0;
        }

        spin_unlock(&group->notification_lock);

        mark->fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;

        /* ... Write report data to error event ... */

        fanotify_encode_fh(&fee->object_fh, fanotify_encode_fh_len(inode),
                           NULL, 0);

        fsnotify_add_event(group, &fee->fae.fse, NULL);

Quoted 5 lines

}

Unless you think this is too hack-ish.

To be fair, I think it is hack-ish.

Actually, I wouldn't mind the hack-ish-ness if it would simplify things, but I do not see how this is the case here.
I still cannot wrap my head around the semantics, which is a big red light. First of all a suggestion should start with the lifetime rules:
- Possible states
- State transition rules