Skip to contentSkip to conversation

binfmt_misc: use RCU for the handler lookup

PATCH Superseded Christian Brauner 2 participants 5 messages Jul 8 – Jul 9

Subsystems (3)
Memory management VFS exec and binfmt

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Christian Brauner

author maintainer
#0 lore raw

cover binfmt_misc: use RCU for the handler lookup

Once binfmt_misc is loaded load_misc_binary() runs for every execve() on the system since binfmt_misc registers at the head of the formats list. Every exec therefore performs read_lock() and read_unlock() on the entries_lock of the relevant binfmt_misc instance, i.e., two atomic read-modify-writes on a shared cacheline, in the common case just to conclude that the binary matches no handler. User namespaces without their own binfmt_misc mount fall back to an ancestor's instance so on container-heavy systems every exec on the machine typically ends up hammering the cacheline of init_binfmt_misc. On PREEMPT_RT the rwlock additionally turns the handler lookup into a sleeping lock on the exec fast path.

The lock protects very little. Entries are immutable after publication except for the Enabled bit which is already toggled locklessly via set_bit()/clear_bit() and entry lifetime is already handled by the users refcount. The read lock's only remaining job is to make "the entry is still linked" and "take a reference" atomic with respect to the unlink sites.

So make the lookup an RCU walk that acquires a reference via refcount_inc_not_zero() and free entries via kfree_rcu(). The removal paths need to detect whether an entry has already been unlinked and rely on list_del_init() reinitialization for that today, but reinitializing the forward pointer of a removed entry would make a concurrent lockless walker standing on it loop indefinitely. hlists support exactly this pattern: hlist_del_init_rcu() keeps the forward pointer of a removed entry intact for concurrent walkers and only zeroes ->pprev with hlist_unhashed() serving as the linked test. Hence the first patch converts the entry list to an hlist so the RCU conversion in the second is a pure locking change.

Writers remain serialized by the inode lock of the root dentry with one exception: bm_evict_inode() unlinks entries during umount without holding it. A spinlock stays around the unlink sites to make that exclusion explicit instead of relying on superblock lifetime rules to provide it implicitly.

Handler removal semantics are unchanged. An exec that acquired a reference just before its handler was unregistered already completes with the removed handler today. The read lock never protected against that, it only made the window smaller.

With this an exec that matches no binfmt_misc entry no longer writes to any shared cacheline at all. The last patch annotates the long-standing lockless ->enabled accesses for KCSAN while at it.

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
Christian Brauner (3):
binfmt_misc: convert entry list to an hlist
binfmt_misc: use RCU for the handler lookup
binfmt_misc: annotate racy accesses to ->enabled

 fs/binfmt_misc.c        | 86 ++++++++++++++++++++++++++++---------------------
 include/linux/binfmts.h |  4 +--
 kernel/user.c           |  4 +--
 3 files changed, 53 insertions(+), 41 deletions(-)

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260708-work-binfmt_misc-locking-15347df12ec8

author maintainer
#1 lore raw

PATCH 1/3 binfmt_misc: convert entry list to an hlist

The upcoming conversion of the handler lookup to RCU walks cannot use list_del_init(): reinitializing the forward pointer of a removed entry would make a concurrent lockless walker standing on that entry loop back onto it indefinitely. The removal paths do rely on reinitialization though because bm_{entry,status}_write() and bm_evict_inode() need to detect whether an entry has already been unlinked.

hlists support exactly this pattern: hlist_del_init_rcu() keeps the forward pointer of the removed entry intact for concurrent walkers and only zeroes ->pprev with hlist_unhashed() serving as the linked test.

Convert the entry list to an hlist now while keeping the rwlock so the subsequent RCU conversion is a pure locking change. hlist_add_head() inserts at the head just as list_add() did so lookup precedence between registered handlers is unchanged.

Signed-off-by Christian Brauner (Amutable) <[email protected]>
---
 fs/binfmt_misc.c        | 25 +++++++++++++------------
 include/linux/binfmts.h |  2 +-
 kernel/user.c           |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)
fs/binfmt_misc.c +13 -12
index 84349fcb93f1..34de1b01e9c2 100644
@@ -48,7 +48,7 @@ enum {Enabled, Magic};
 #define MISC_FMT_OPEN_FILE (1UL << 28)
 
 typedef struct {
-	struct list_head list;
+	struct hlist_node node;
 	unsigned long flags;		/* type, status, etc. */
 	int offset;			/* offset of magic */
 	int size;			/* size of magic/mask */

@@ -95,7 +95,7 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,
 	Node *e;
 
 	/* Walk all the registered handlers. */
-	list_for_each_entry(e, &misc->entries, list) {
+	hlist_for_each_entry(e, &misc->entries, node) {
 		char *s;
 		int j;
 

@@ -654,8 +654,8 @@ static struct binfmt_misc *i_binfmt_misc(struct inode *inode)
  *
  * If the ->evict call was not caused by a super block shutdown but by a write
  * to remove the entry or all entries via bm_{entry,status}_write() the entry
- * will have already been removed from the list. We keep the list_empty() check
- * to make that explicit.
+ * will have already been removed from the list. We keep the hlist_unhashed()
+ * check to make that explicit.
 */
 static void bm_evict_inode(struct inode *inode)
 {

@@ -668,8 +668,8 @@ static void bm_evict_inode(struct inode *inode)
 
 		misc = i_binfmt_misc(inode);
 		write_lock(&misc->entries_lock);
-		if (!list_empty(&e->list))
-			list_del_init(&e->list);
+		if (!hlist_unhashed(&e->node))
+			hlist_del_init(&e->node);
 		write_unlock(&misc->entries_lock);
 		put_binfmt_handler(e);
 	}

@@ -690,7 +690,7 @@ static void bm_evict_inode(struct inode *inode)
 static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e)
 {
 	write_lock(&misc->entries_lock);
-	list_del_init(&e->list);
+	hlist_del_init(&e->node);
 	write_unlock(&misc->entries_lock);
 	locked_recursive_removal(e->dentry, NULL);
 }

@@ -746,7 +746,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
 		 * read-only. So we only need to take the write lock when we
 		 * actually remove the entry from the list.
 		 */
-		if (!list_empty(&e->list))
+		if (!hlist_unhashed(&e->node))
 			remove_binfmt_handler(i_binfmt_misc(inode), e);
 
 		inode_unlock(inode);

@@ -790,7 +790,7 @@ static int add_entry(Node *e, struct super_block *sb)
 	d_make_persistent(dentry, inode);
 	misc = i_binfmt_misc(inode);
 	write_lock(&misc->entries_lock);
-	list_add(&e->list, &misc->entries);
+	hlist_add_head(&e->node, &misc->entries);
 	write_unlock(&misc->entries_lock);
 	simple_done_creating(dentry);
 	return 0;

@@ -863,8 +863,9 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 {
 	struct binfmt_misc *misc;
 	int res = parse_command(buffer, count);
-	Node *e, *next;
+	struct hlist_node *next;
 	struct inode *inode;
+	Node *e;
 
 	misc = i_binfmt_misc(file_inode(file));
 	switch (res) {

@@ -890,7 +891,7 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 		 * read-only. So we only need to take the write lock when we
 		 * actually remove the entry from the list.
 		 */
-		list_for_each_entry_safe(e, next, &misc->entries, list)
+		hlist_for_each_entry_safe(e, next, &misc->entries, node)
 			remove_binfmt_handler(misc, e);
 
 		inode_unlock(inode);

@@ -965,7 +966,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
 		if (!misc)
 			return -ENOMEM;
 
-		INIT_LIST_HEAD(&misc->entries);
+		INIT_HLIST_HEAD(&misc->entries);
 		rwlock_init(&misc->entries_lock);
 
 		/* Pairs with smp_load_acquire() in load_binfmt_misc(). */
include/linux/binfmts.h +1 -1
index 2c77e383e737..071da63f2b48 100644
@@ -101,7 +101,7 @@ struct linux_binfmt {
 
 #if IS_ENABLED(CONFIG_BINFMT_MISC)
 struct binfmt_misc {
-	struct list_head entries;
+	struct hlist_head entries;
 	rwlock_t entries_lock;
 	bool enabled;
 } __randomize_layout;
kernel/user.c +1 -1
index 7aef4e679a6a..c6a2bfb4d918 100644
@@ -23,7 +23,7 @@
 
 #if IS_ENABLED(CONFIG_BINFMT_MISC)
 struct binfmt_misc init_binfmt_misc = {
-	.entries = LIST_HEAD_INIT(init_binfmt_misc.entries),
+	.entries = HLIST_HEAD_INIT,
 	.enabled = true,
 	.entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_misc.entries_lock),
 };
Signature · 1 line
2.53.0
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] binfmt_misc: convert entry list to an hlist

Quoted 47 lines · Christian Brauner

The upcoming conversion of the handler lookup to RCU walks cannot use list_del_init(): reinitializing the forward pointer of a removed entry would make a concurrent lockless walker standing on that entry loop back onto it indefinitely. The removal paths do rely on reinitialization though because bm_{entry,status}_write() and bm_evict_inode() need to detect whether an entry has already been unlinked.

hlists support exactly this pattern: hlist_del_init_rcu() keeps the forward pointer of the removed entry intact for concurrent walkers and only zeroes ->pprev with hlist_unhashed() serving as the linked test.

Convert the entry list to an hlist now while keeping the rwlock so the subsequent RCU conversion is a pure locking change. hlist_add_head() inserts at the head just as list_add() did so lookup precedence between registered handlers is unchanged.

Signed-off-by Christian Brauner (Amutable) <[email protected]>
---
 fs/binfmt_misc.c        | 25 +++++++++++++------------
 include/linux/binfmts.h |  2 +-
 kernel/user.c           |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)
fs/binfmt_misc.c +2 -2
index 84349fcb93f1..34de1b01e9c2 100644
@@ -48,7 +48,7 @@ enum {Enabled, Magic};
 #define MISC_FMT_OPEN_FILE (1UL << 28)
 
 typedef struct {
-	struct list_head list;
+	struct hlist_node node;
 	unsigned long flags;		/* type, status, etc. */
 	int offset;			/* offset of magic */
 	int size;			/* size of magic/mask */

@@ -95,7 +95,7 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,
 	Node *e;
 
 	/* Walk all the registered handlers. */
-	list_for_each_entry(e, &misc->entries, list) {
+	hlist_for_each_entry(e, &misc->entries, node) {
 		char *s;
 		int j;

fs/binfmt_misc.c static struct binfmt_misc *i_binfmt_misc(struct inode *inode) · line 658

Quoted 7 lines
fs/binfmt_misc.c +2 -2
@@ -654,8 +654,8 @@ static struct binfmt_misc *i_binfmt_misc(struct inode *inode)
  *
  * If the ->evict call was not caused by a super block shutdown but by a write
  * to remove the entry or all entries via bm_{entry,status}_write() the entry
- * will have already been removed from the list. We keep the list_empty() check
- * to make that explicit.
+ * will have already been removed from the list. We keep the hlist_unhashed()
+ * check to make that explicit.

Right. That's a good idea. The code reads a bit confusing because of the two ways we may end up in bm_evict_inode(), but that isn't an issue of this patch specifically.

Quoted 100 lines
fs/binfmt_misc.c +9 -8
 */
 static void bm_evict_inode(struct inode *inode)
 {

@@ -668,8 +668,8 @@ static void bm_evict_inode(struct inode *inode)
 
 		misc = i_binfmt_misc(inode);
 		write_lock(&misc->entries_lock);
-		if (!list_empty(&e->list))
-			list_del_init(&e->list);
+		if (!hlist_unhashed(&e->node))
+			hlist_del_init(&e->node);
 		write_unlock(&misc->entries_lock);
 		put_binfmt_handler(e);
 	}

@@ -690,7 +690,7 @@ static void bm_evict_inode(struct inode *inode)
 static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e)
 {
 	write_lock(&misc->entries_lock);
-	list_del_init(&e->list);
+	hlist_del_init(&e->node);
 	write_unlock(&misc->entries_lock);
 	locked_recursive_removal(e->dentry, NULL);
 }

@@ -746,7 +746,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
 		 * read-only. So we only need to take the write lock when we
 		 * actually remove the entry from the list.
 		 */
-		if (!list_empty(&e->list))
+		if (!hlist_unhashed(&e->node))
 			remove_binfmt_handler(i_binfmt_misc(inode), e);
 
 		inode_unlock(inode);

@@ -790,7 +790,7 @@ static int add_entry(Node *e, struct super_block *sb)
 	d_make_persistent(dentry, inode);
 	misc = i_binfmt_misc(inode);
 	write_lock(&misc->entries_lock);
-	list_add(&e->list, &misc->entries);
+	hlist_add_head(&e->node, &misc->entries);
 	write_unlock(&misc->entries_lock);
 	simple_done_creating(dentry);
 	return 0;

@@ -863,8 +863,9 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 {
 	struct binfmt_misc *misc;
 	int res = parse_command(buffer, count);
-	Node *e, *next;
+	struct hlist_node *next;
 	struct inode *inode;
+	Node *e;
 
 	misc = i_binfmt_misc(file_inode(file));
 	switch (res) {

@@ -890,7 +891,7 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 		 * read-only. So we only need to take the write lock when we
 		 * actually remove the entry from the list.
 		 */
-		list_for_each_entry_safe(e, next, &misc->entries, list)
+		hlist_for_each_entry_safe(e, next, &misc->entries, node)
 			remove_binfmt_handler(misc, e);
 
 		inode_unlock(inode);

@@ -965,7 +966,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
 		if (!misc)
 			return -ENOMEM;
 
-		INIT_LIST_HEAD(&misc->entries);
+		INIT_HLIST_HEAD(&misc->entries);
 		rwlock_init(&misc->entries_lock);
 
 		/* Pairs with smp_load_acquire() in load_binfmt_misc(). */
include/linux/binfmts.h +1 -1
index 2c77e383e737..071da63f2b48 100644
@@ -101,7 +101,7 @@ struct linux_binfmt {
 
 #if IS_ENABLED(CONFIG_BINFMT_MISC)
 struct binfmt_misc {
-	struct list_head entries;
+	struct hlist_head entries;
 	rwlock_t entries_lock;
 	bool enabled;
 } __randomize_layout;
kernel/user.c +1 -1
index 7aef4e679a6a..c6a2bfb4d918 100644
@@ -23,7 +23,7 @@
 
 #if IS_ENABLED(CONFIG_BINFMT_MISC)
 struct binfmt_misc init_binfmt_misc = {
-	.entries = LIST_HEAD_INIT(init_binfmt_misc.entries),
+	.entries = HLIST_HEAD_INIT,
 	.enabled = true,
 	.entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_misc.entries_lock),
 };
Signature · 2 lines
-- 
2.53.0

I can't really comment on whether the cache bouncing is really as significant a problem to warrant moving to RCU, but your argument makes sense to me.

At any rate I see no issues here or with the RCU change. For this one, feel free to add:

Reviewed-by Jori Koolstra <[email protected]>

Will look a bit more carefully at the RCU change later today/tomorrow.

author maintainer
#3 lore raw

PATCH 2/3 binfmt_misc: use RCU for the handler lookup

In reply to Christian Brauner

Once binfmt_misc is loaded load_misc_binary() runs for every execve() on the system since binfmt_misc registers at the head of the formats list. Every exec therefore performs read_lock() and read_unlock() on the entries_lock of the relevant binfmt_misc instance, i.e., two atomic read-modify-writes on a shared cacheline. User namespaces without their own binfmt_misc mount fall back to an ancestor's instance so on container-heavy systems every exec on the machine typically ends up hammering the cacheline of init_binfmt_misc. On PREEMPT_RT the rwlock additionally turns the handler lookup into a sleeping lock on the exec fast path.

The lock protects very little. Entries are immutable after publication except for the Enabled bit which is already toggled locklessly via set_bit()/clear_bit() and entry lifetime is already handled by the users refcount via get_binfmt_handler()/put_binfmt_handler(). The read lock's only remaining job is to make "the entry is still linked" and "take a reference" atomic with respect to the unlink sites.

Switch the lookup to an RCU walk:

* Lookup walks the entry list under rcu_read_lock() and acquires a reference via refcount_inc_not_zero(). The refcount can only drop to zero after an entry has been unlinked so a failed increment means the walk raced with an unlink. Restarting the search is bounded because an unlinked entry cannot be found again.

* The unlink sites use hlist_del_init_rcu() which keeps the forward pointer intact for concurrent walkers and preserves hlist_unhashed() as the protection against double removal.

* The final put frees the entry via kfree_rcu() as a concurrent walker may still dereference its flags, magic, mask, and inline strings. They all live in the entry allocation itself and thus stay valid until a grace period has elapsed. Closing the interpreter file stays synchronous. It is only used with a reference already held and all final puts run in process context. Using kfree_rcu() instead of call_rcu() with a custom callback also guarantees that no rcu callback ever runs module code so module unload doesn't need an rcu_barrier().

* Writers remain serialized by the inode lock of the root dentry with one exception: bm_evict_inode() called from generic_shutdown_super() during umount unlinks entries without holding it. Keep a spinlock around the unlink sites instead of relying on superblock lifetime rules to make that exclusion implicit.

Handler removal semantics are unchanged. An exec that acquired a reference just before its handler was unregistered already completes with the removed handler today. The read lock never protected against that, it only made the window smaller.

With this an exec that matches no binfmt_misc entry, the common case, no longer writes to any shared cacheline at all.

Signed-off-by Christian Brauner (Amutable) <[email protected]>
---
 fs/binfmt_misc.c        | 59 +++++++++++++++++++++++++++++--------------------
 include/linux/binfmts.h |  2 +-
 kernel/user.c           |  2 +-
 3 files changed, 37 insertions(+), 26 deletions(-)
fs/binfmt_misc.c +35 -24
index 34de1b01e9c2..9d1039dcd3f7 100644
@@ -24,6 +24,7 @@
 #include <linux/pagemap.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
+#include <linux/rculist.h>
 #include <linux/fs_context.h>
 #include <linux/syscalls.h>
 #include <linux/fs.h>

@@ -59,6 +60,7 @@ typedef struct {
 	struct dentry *dentry;
 	struct file *interp_file;
 	refcount_t users;		/* sync removal with load_misc_binary() */
+	struct rcu_head rcu;
 } Node;
 
 static struct file_system_type bm_fs_type;

@@ -86,6 +88,8 @@ static struct file_system_type bm_fs_type;
  * Search for a binary type handler for @bprm in the list of registered binary
  * type handlers.
  *
+ * The caller must hold the RCU read lock.
+ *
  * Return: binary type list entry on success, NULL on failure
  */
 static Node *search_binfmt_handler(struct binfmt_misc *misc,

@@ -95,7 +99,7 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,
 	Node *e;
 
 	/* Walk all the registered handlers. */
-	hlist_for_each_entry(e, &misc->entries, node) {
+	hlist_for_each_entry_rcu(e, &misc->entries, node) {
 		char *s;
 		int j;
 

@@ -134,7 +138,10 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,
  * @bprm: binary for which we are looking for a handler
  *
  * Try to find a binfmt handler for the binary type. If one is found take a
- * reference to protect against removal via bm_{entry,status}_write().
+ * reference to protect against removal via bm_{entry,status}_write(). The
+ * refcount of an entry can only drop to zero once it has been unlinked and
+ * a restarted search cannot find an unlinked entry again so the retry loop
+ * is bounded.
  *
  * Return: binary type list entry on success, NULL on failure
  */

@@ -143,11 +150,10 @@ static Node *get_binfmt_handler(struct binfmt_misc *misc,
 {
 	Node *e;
 
-	read_lock(&misc->entries_lock);
-	e = search_binfmt_handler(misc, bprm);
-	if (e)
-		refcount_inc(&e->users);
-	read_unlock(&misc->entries_lock);
+	guard(rcu)();
+	do {
+		e = search_binfmt_handler(misc, bprm);
+	} while (e && !refcount_inc_not_zero(&e->users));
 	return e;
 }
 

@@ -164,7 +170,8 @@ static void put_binfmt_handler(Node *e)
 	if (refcount_dec_and_test(&e->users)) {
 		if (e->flags & MISC_FMT_OPEN_FILE)
 			filp_close(e->interp_file, NULL);
-		kfree(e);
+		/* Lockless walkers may still dereference this entry. */
+		kfree_rcu(e, rcu);
 	}
 }
 

@@ -667,10 +674,10 @@ static void bm_evict_inode(struct inode *inode)
 		struct binfmt_misc *misc;
 
 		misc = i_binfmt_misc(inode);
-		write_lock(&misc->entries_lock);
+		spin_lock(&misc->entries_lock);
 		if (!hlist_unhashed(&e->node))
-			hlist_del_init(&e->node);
-		write_unlock(&misc->entries_lock);
+			hlist_del_init_rcu(&e->node);
+		spin_unlock(&misc->entries_lock);
 		put_binfmt_handler(e);
 	}
 }

@@ -689,9 +696,9 @@ static void bm_evict_inode(struct inode *inode)
  */
 static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e)
 {
-	write_lock(&misc->entries_lock);
-	hlist_del_init(&e->node);
-	write_unlock(&misc->entries_lock);
+	spin_lock(&misc->entries_lock);
+	hlist_del_init_rcu(&e->node);
+	spin_unlock(&misc->entries_lock);
 	locked_recursive_removal(e->dentry, NULL);
 }
 

@@ -742,9 +749,11 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
 		 * via bm_{entry,register,status}_write() inode_lock() on the
 		 * root inode must be held.
 		 * The lock is exclusive ensuring that the list can't be
-		 * modified. Only load_misc_binary() can access but does so
-		 * read-only. So we only need to take the write lock when we
-		 * actually remove the entry from the list.
+		 * modified. Only load_misc_binary() can access the list
+		 * concurrently and it does so under RCU. So entries_lock only
+		 * needs to be held when an entry is actually unlinked to
+		 * serialize against bm_evict_inode() during umount which
+		 * unlinks without holding inode_lock.
 		 */
 		if (!hlist_unhashed(&e->node))
 			remove_binfmt_handler(i_binfmt_misc(inode), e);

@@ -789,9 +798,9 @@ static int add_entry(Node *e, struct super_block *sb)
 
 	d_make_persistent(dentry, inode);
 	misc = i_binfmt_misc(inode);
-	write_lock(&misc->entries_lock);
-	hlist_add_head(&e->node, &misc->entries);
-	write_unlock(&misc->entries_lock);
+	spin_lock(&misc->entries_lock);
+	hlist_add_head_rcu(&e->node, &misc->entries);
+	spin_unlock(&misc->entries_lock);
 	simple_done_creating(dentry);
 	return 0;
 }

@@ -887,9 +896,11 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 		 * via bm_{entry,register,status}_write() inode_lock() on the
 		 * root inode must be held.
 		 * The lock is exclusive ensuring that the list can't be
-		 * modified. Only load_misc_binary() can access but does so
-		 * read-only. So we only need to take the write lock when we
-		 * actually remove the entry from the list.
+		 * modified. Only load_misc_binary() can access the list
+		 * concurrently and it does so under RCU. So entries_lock only
+		 * needs to be held when an entry is actually unlinked to
+		 * serialize against bm_evict_inode() during umount which
+		 * unlinks without holding inode_lock.
 		 */
 		hlist_for_each_entry_safe(e, next, &misc->entries, node)
 			remove_binfmt_handler(misc, e);

@@ -967,7 +978,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
 			return -ENOMEM;
 
 		INIT_HLIST_HEAD(&misc->entries);
-		rwlock_init(&misc->entries_lock);
+		spin_lock_init(&misc->entries_lock);
 
 		/* Pairs with smp_load_acquire() in load_binfmt_misc(). */
 		smp_store_release(&user_ns->binfmt_misc, misc);
include/linux/binfmts.h +1 -1
index 071da63f2b48..7e7333b7bb0f 100644
@@ -102,7 +102,7 @@ struct linux_binfmt {
 #if IS_ENABLED(CONFIG_BINFMT_MISC)
 struct binfmt_misc {
 	struct hlist_head entries;
-	rwlock_t entries_lock;
+	spinlock_t entries_lock;
 	bool enabled;
 } __randomize_layout;
kernel/user.c +1 -1
index c6a2bfb4d918..21bafdc11379 100644
@@ -25,7 +25,7 @@
 struct binfmt_misc init_binfmt_misc = {
 	.entries = HLIST_HEAD_INIT,
 	.enabled = true,
-	.entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_misc.entries_lock),
+	.entries_lock = __SPIN_LOCK_UNLOCKED(init_binfmt_misc.entries_lock),
 };
 EXPORT_SYMBOL_GPL(init_binfmt_misc);
 #endif
Signature · 1 line
2.53.0
author maintainer
#4 lore raw

PATCH 3/3 binfmt_misc: annotate racy accesses to ->enabled

In reply to Christian Brauner

->enabled has always been read and written locklessly: every exec reads it in load_misc_binary() while bm_status_write() or a concurrent remount via bm_fill_super() may flip it. That is fine as it is an independent boolean toggle but the accesses should be marked accordingly for KCSAN. Annotate them with READ_ONCE()/WRITE_ONCE().

Signed-off-by Christian Brauner (Amutable) <[email protected]>
---
 fs/binfmt_misc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/binfmt_misc.c +5 -5
index 9d1039dcd3f7..0f04709a0a39 100644
@@ -215,7 +215,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
 	struct binfmt_misc *misc;
 
 	misc = load_binfmt_misc();
-	if (!misc->enabled)
+	if (!READ_ONCE(misc->enabled))
 		return retval;
 
 	fmt = get_binfmt_handler(misc, bprm);

@@ -863,7 +863,7 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 	char *s;
 
 	misc = i_binfmt_misc(file_inode(file));
-	s = misc->enabled ? "enabled\n" : "disabled\n";
+	s = READ_ONCE(misc->enabled) ? "enabled\n" : "disabled\n";
 	return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
 }
 

@@ -880,11 +880,11 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 	switch (res) {
 	case 1:
 		/* Disable all handlers. */
-		misc->enabled = false;
+		WRITE_ONCE(misc->enabled, false);
 		break;
 	case 2:
 		/* Enable all handlers. */
-		misc->enabled = true;
+		WRITE_ONCE(misc->enabled, true);
 		break;
 	case 3:
 		/* Delete all handlers. */

@@ -995,7 +995,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
 	 * someone mounts binfmt_misc for the first time or again we simply
 	 * reset ->enabled to true.
 	 */
-	misc->enabled = true;
+	WRITE_ONCE(misc->enabled, true);
 
 	err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
 	if (!err)
Signature · 1 line
2.53.0