Merge tag 'fsnotify_for_v6.12-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs

torvalds · torvalds · commit e02f08e21716 · 2024-10-04T10:31:59.000-07:00
Pull fsnotify fixes from Jan Kara:
 "Fixes for an inotify deadlock and a data race in fsnotify"

* tag 'fsnotify_for_v6.12-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs:
  inotify: Fix possible deadlock in fsnotify_destroy_mark
  fsnotify: Avoid data race between fsnotify_recalc_mask() and fsnotify_object_watched()
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
@@ -792,7 +792,7 @@ nfsd_file_cache_init(void)
 	}
 
 	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
-							FSNOTIFY_GROUP_NOFS);
+							0);
 	if (IS_ERR(nfsd_file_fsnotify_group)) {
 		pr_err("nfsd: unable to create fsnotify group: %ld\n",
 			PTR_ERR(nfsd_file_fsnotify_group));
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
@@ -406,8 +406,7 @@ static int __init dnotify_init(void)
 					  SLAB_PANIC|SLAB_ACCOUNT);
 	dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC|SLAB_ACCOUNT);
 
-	dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops,
-					     FSNOTIFY_GROUP_NOFS);
+	dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops, 0);
 	if (IS_ERR(dnotify_group))
 		panic("unable to allocate fsnotify group for dnotify\n");
 	dnotify_sysctl_init();
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
@@ -1480,7 +1480,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 	/* fsnotify_alloc_group takes a ref.  Dropped in fanotify_release */
 	group = fsnotify_alloc_group(&fanotify_fsnotify_ops,
-				     FSNOTIFY_GROUP_USER | FSNOTIFY_GROUP_NOFS);
+				     FSNOTIFY_GROUP_USER);
 	if (IS_ERR(group)) {
 		return PTR_ERR(group);
 	}
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
@@ -183,8 +183,10 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask,
 	BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
 
 	/* Did either inode/sb/mount subscribe for events with parent/name? */
-	marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
-	marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
+	marks_mask |= fsnotify_parent_needed_mask(
+				READ_ONCE(inode->i_fsnotify_mask));
+	marks_mask |= fsnotify_parent_needed_mask(
+				READ_ONCE(inode->i_sb->s_fsnotify_mask));
 	marks_mask |= fsnotify_parent_needed_mask(mnt_mask);
 
 	/* Did they subscribe for this event with parent/name info? */
@@ -195,8 +197,8 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask,
 static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
 					   __u32 mask)
 {
-	__u32 marks_mask = inode->i_fsnotify_mask | mnt_mask |
-			   inode->i_sb->s_fsnotify_mask;
+	__u32 marks_mask = READ_ONCE(inode->i_fsnotify_mask) | mnt_mask |
+			   READ_ONCE(inode->i_sb->s_fsnotify_mask);
 
 	return mask & marks_mask & ALL_FSNOTIFY_EVENTS;
 }
@@ -213,7 +215,8 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 		      int data_type)
 {
 	const struct path *path = fsnotify_data_path(data, data_type);
-	__u32 mnt_mask = path ? real_mount(path->mnt)->mnt_fsnotify_mask : 0;
+	__u32 mnt_mask = path ?
+		READ_ONCE(real_mount(path->mnt)->mnt_fsnotify_mask) : 0;
 	struct inode *inode = d_inode(dentry);
 	struct dentry *parent;
 	bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
@@ -557,13 +560,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	    (!inode2 || !inode2->i_fsnotify_marks))
 		return 0;
 
-	marks_mask = sb->s_fsnotify_mask;
+	marks_mask = READ_ONCE(sb->s_fsnotify_mask);
 	if (mnt)
-		marks_mask |= mnt->mnt_fsnotify_mask;
+		marks_mask |= READ_ONCE(mnt->mnt_fsnotify_mask);
 	if (inode)
-		marks_mask |= inode->i_fsnotify_mask;
+		marks_mask |= READ_ONCE(inode->i_fsnotify_mask);
 	if (inode2)
-		marks_mask |= inode2->i_fsnotify_mask;
+		marks_mask |= READ_ONCE(inode2->i_fsnotify_mask);
 
 
 	/*
diff --git a/fs/notify/group.c b/fs/notify/group.c
@@ -115,7 +115,6 @@ static struct fsnotify_group *__fsnotify_alloc_group(
 				const struct fsnotify_ops *ops,
 				int flags, gfp_t gfp)
 {
-	static struct lock_class_key nofs_marks_lock;
 	struct fsnotify_group *group;
 
 	group = kzalloc(sizeof(struct fsnotify_group), gfp);
@@ -136,16 +135,6 @@ static struct fsnotify_group *__fsnotify_alloc_group(
 
 	group->ops = ops;
 	group->flags = flags;
-	/*
-	 * For most backends, eviction of inode with a mark is not expected,
-	 * because marks hold a refcount on the inode against eviction.
-	 *
-	 * Use a different lockdep class for groups that support evictable
-	 * inode marks, because with evictable marks, mark_mutex is NOT
-	 * fs-reclaim safe - the mutex is taken when evicting inodes.
-	 */
-	if (flags & FSNOTIFY_GROUP_NOFS)
-		lockdep_set_class(&group->mark_mutex, &nofs_marks_lock);
 
 	return group;
 }
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
@@ -569,7 +569,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 		/* more bits in old than in new? */
 		int dropped = (old_mask & ~new_mask);
 		/* more bits in this fsn_mark than the inode's mask? */
-		int do_inode = (new_mask & ~inode->i_fsnotify_mask);
+		int do_inode = (new_mask & ~READ_ONCE(inode->i_fsnotify_mask));
 
 		/* update the inode with this new fsn_mark */
 		if (dropped || do_inode)
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
@@ -128,7 +128,7 @@ __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn)
 	if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))
 		return 0;
 
-	return *fsnotify_conn_mask_p(conn);
+	return READ_ONCE(*fsnotify_conn_mask_p(conn));
 }
 
 static void fsnotify_get_sb_watched_objects(struct super_block *sb)
@@ -245,7 +245,11 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		    !(mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF))
 			want_iref = true;
 	}
-	*fsnotify_conn_mask_p(conn) = new_mask;
+	/*
+	 * We use WRITE_ONCE() to prevent silly compiler optimizations from
+	 * confusing readers not holding conn->lock with partial updates.
+	 */
+	WRITE_ONCE(*fsnotify_conn_mask_p(conn), new_mask);
 
 	return fsnotify_update_iref(conn, want_iref);
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
@@ -217,7 +217,6 @@ struct fsnotify_group {
 
 #define FSNOTIFY_GROUP_USER	0x01 /* user allocated group */
 #define FSNOTIFY_GROUP_DUPS	0x02 /* allow multiple marks per object */
-#define FSNOTIFY_GROUP_NOFS	0x04 /* group lock is not direct reclaim safe */
 	int flags;
 	unsigned int owner_flags;	/* stored flags of mark_mutex owner */
 
@@ -268,22 +267,19 @@ struct fsnotify_group {
 static inline void fsnotify_group_lock(struct fsnotify_group *group)
 {
 	mutex_lock(&group->mark_mutex);
-	if (group->flags & FSNOTIFY_GROUP_NOFS)
-		group->owner_flags = memalloc_nofs_save();
+	group->owner_flags = memalloc_nofs_save();
 }
 
 static inline void fsnotify_group_unlock(struct fsnotify_group *group)
 {
-	if (group->flags & FSNOTIFY_GROUP_NOFS)
-		memalloc_nofs_restore(group->owner_flags);
+	memalloc_nofs_restore(group->owner_flags);
 	mutex_unlock(&group->mark_mutex);
 }
 
 static inline void fsnotify_group_assert_locked(struct fsnotify_group *group)
 {
 	WARN_ON_ONCE(!mutex_is_locked(&group->mark_mutex));
-	if (group->flags & FSNOTIFY_GROUP_NOFS)
-		WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));
+	WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));
 }
 
 /* When calling fsnotify tell it if the data is a path or inode */

Original file line number	Diff line number	Diff line change
`@@ -792,7 +792,7 @@ nfsd_file_cache_init(void)`
`792`	`792`	`}`
`793`	`793`
`794`	`794`	`nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,`
`795`		`- FSNOTIFY_GROUP_NOFS);`
	`795`	`+ 0);`
`796`	`796`	`if (IS_ERR(nfsd_file_fsnotify_group)) {`
`797`	`797`	`pr_err("nfsd: unable to create fsnotify group: %ld\n",`
`798`	`798`	`PTR_ERR(nfsd_file_fsnotify_group));`
Original file line number	Diff line number	Diff line change
`@@ -1480,7 +1480,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)`
`1480`	`1480`
`1481`	`1481`	`/* fsnotify_alloc_group takes a ref. Dropped in fanotify_release */`
`1482`	`1482`	`group = fsnotify_alloc_group(&fanotify_fsnotify_ops,`
`1483`		`- FSNOTIFY_GROUP_USER \| FSNOTIFY_GROUP_NOFS);`
	`1483`	`+ FSNOTIFY_GROUP_USER);`
`1484`	`1484`	`if (IS_ERR(group)) {`
`1485`	`1485`	`return PTR_ERR(group);`
`1486`	`1486`	`}`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn)`
`128`	`128`	`if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))`
`129`	`129`	`return 0;`
`130`	`130`
`131`		`- return *fsnotify_conn_mask_p(conn);`
	`131`	`+ return READ_ONCE(*fsnotify_conn_mask_p(conn));`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`static void fsnotify_get_sb_watched_objects(struct super_block *sb)`
`@@ -245,7 +245,11 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector conn)`
`245`	`245`	`!(mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF))`
`246`	`246`	`want_iref = true;`
`247`	`247`	`}`
`248`		`- *fsnotify_conn_mask_p(conn) = new_mask;`
	`248`	`+ /*`
	`249`	`+ * We use WRITE_ONCE() to prevent silly compiler optimizations from`
	`250`	`+ * confusing readers not holding conn->lock with partial updates.`
	`251`	`+ */`
	`252`	`+ WRITE_ONCE(*fsnotify_conn_mask_p(conn), new_mask);`
`249`	`253`
`250`	`254`	`return fsnotify_update_iref(conn, want_iref);`
`251`	`255`	`}`
Original file line number	Diff line number	Diff line change
`@@ -217,7 +217,6 @@ struct fsnotify_group {`
`217`	`217`
`218`	`218`	`#define FSNOTIFY_GROUP_USER 0x01 /* user allocated group */`
`219`	`219`	`#define FSNOTIFY_GROUP_DUPS 0x02 /* allow multiple marks per object */`
`220`		`-#define FSNOTIFY_GROUP_NOFS 0x04 /* group lock is not direct reclaim safe */`
`221`	`220`	`int flags;`
`222`	`221`	`unsigned int owner_flags; /* stored flags of mark_mutex owner */`
`223`	`222`
`@@ -268,22 +267,19 @@ struct fsnotify_group {`
`268`	`267`	`static inline void fsnotify_group_lock(struct fsnotify_group *group)`
`269`	`268`	`{`
`270`	`269`	`mutex_lock(&group->mark_mutex);`
`271`		`- if (group->flags & FSNOTIFY_GROUP_NOFS)`
`272`		`- group->owner_flags = memalloc_nofs_save();`
	`270`	`+ group->owner_flags = memalloc_nofs_save();`
`273`	`271`	`}`
`274`	`272`
`275`	`273`	`static inline void fsnotify_group_unlock(struct fsnotify_group *group)`
`276`	`274`	`{`
`277`		`- if (group->flags & FSNOTIFY_GROUP_NOFS)`
`278`		`- memalloc_nofs_restore(group->owner_flags);`
	`275`	`+ memalloc_nofs_restore(group->owner_flags);`
`279`	`276`	`mutex_unlock(&group->mark_mutex);`
`280`	`277`	`}`
`281`	`278`
`282`	`279`	`static inline void fsnotify_group_assert_locked(struct fsnotify_group *group)`
`283`	`280`	`{`
`284`	`281`	`WARN_ON_ONCE(!mutex_is_locked(&group->mark_mutex));`
`285`		`- if (group->flags & FSNOTIFY_GROUP_NOFS)`
`286`		`- WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));`
	`282`	`+ WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));`
`287`	`283`	`}`
`288`	`284`
`289`	`285`	`/* When calling fsnotify tell it if the data is a path or inode */`