binfmt_misc: write access fixes, RCU handler lookup and cleanups
Subsystems (5)
Christian Brauner
maintainerv2 cover binfmt_misc: write access fixes, RCU handler lookup and cleanups
The first two patches fix two i_writecount imbalances on MISC_FMT_OPEN_FILE interpreter files that turned up while auditing the file for the rework below and are marked for stable: removing an entry never restored the write access denied by open_exec() at registration, leaving the interpreter unwritable until its inode gets evicted, and the write denial taken on the interpreter clone during exec is not paired with the FMODE_FSNOTIFY_HSM aware release the exec machinery uses, so pre-content watches make execs leak write denials.
The rest reworks the locking and tidies the file up.
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 third patch converts the entry list to an hlist so the RCU conversion in the fourth 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 fifth patch annotates the long-standing lockless ->enabled accesses for KCSAN and the three patches after it make the entry flags proper enums and give struct binfmt_misc_entry a name that isn't Node.
The remaining patches are a cleanup pass over the whole file: remove the VERBOSE_STATUS and USE_DEBUG compile-time toggles, convert the entry file to seq_file, factor out entry matching, entry removal and the register string field parsing, make the entry/register string allocation a flexible array member, give the parse_command() results names, let cleanup.h unwind the entry registration and exec error paths and prune the include list down to what is used. Aside from seq_lseek() now bounding seeks on entry files and the ETXTBSY propagation in the second patch the cleanups have no user-visible effect.
The last patch adds what the comment in remove_binfmt_handler() had been suggesting for years: entries can now be removed via unlink(2) in addition to the -1 write. The status and register control files refuse removal.
---
Changes in v2:
- Allow removing entries via unlink(2).
- Add two stable-marked fixes restoring i_writecount balance for
MISC_FMT_OPEN_FILE interpreter files, first so they apply to
mainline directly.
- Turn the entry bit numbers and behavior flags into proper enums and
rename Node to struct binfmt_misc_entry.
- Add a cleanup pass over the whole file: remove the VERBOSE_STATUS
and USE_DEBUG toggles, convert the entry file to seq_file, factor
out entry matching, entry removal and register string parsing, use
a flexible array member for the register string, name the
parse_command() results, use cleanup.h for the entry error
unwinding in registration and exec and prune the include list.
- Link to v1: https://patch.msgid.link/[email protected]
---
Christian Brauner (23):
binfmt_misc: restore write access when removing an entry
binfmt_misc: use exe_file_deny_write_access() for the interpreter clone
binfmt_misc: convert entry list to an hlist
binfmt_misc: use RCU for the handler lookup
binfmt_misc: annotate racy accesses to ->enabled
binfmt_misc: turn the entry bit numbers into a proper enum
binfmt_misc: turn the entry behavior flags into an enum
binfmt_misc: rename Node to struct binfmt_misc_entry
binfmt_misc: remove the VERBOSE_STATUS toggle
binfmt_misc: use print_hex_dump_debug() for the register debug output
binfmt_misc: convert the entry file to seq_file
binfmt_misc: factor out the entry matching
binfmt_misc: rename load_binfmt_misc() to current_binfmt_misc()
binfmt_misc: return errors directly in load_misc_binary()
binfmt_misc: give the parse_command() results names
binfmt_misc: factor out the entry removal
binfmt_misc: simplify check_special_flags()
binfmt_misc: use a flexible array member for the register string
binfmt_misc: split the field parsing out of create_entry()
binfmt_misc: use __free(kfree) in bm_register_write()
binfmt_misc: assorted small cleanups
binfmt_misc: include what is used
binfmt_misc: allow removing entries via unlink(2)
Documentation/admin-guide/binfmt-misc.rst | 3 +- fs/binfmt_misc.c | 835 +++++++++++++++--------------- include/linux/binfmts.h | 4 +- kernel/user.c | 4 +- 4 files changed, 429 insertions(+), 417 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260708-work-binfmt_misc-locking-15347df12ec8
Christian Brauner
maintainerPATCH v2 1/23 binfmt_misc: restore write access when removing an entry
Registering an entry with the MISC_FMT_OPEN_FILE flag opens the interpreter via open_exec() which denies write access to it for as long as the entry exists. Removing the entry closes the interpreter file via filp_close() but never restores write access, leaving the inode's i_writecount permanently negative. Opening the interpreter for writing keeps failing with ETXTBSY long after the entry is gone until the inode is evicted from the inode cache.
Commit 90f601b497d7 ("binfmt_misc: restore write access before closing files opened by open_exec()") fixed the same imbalance in the error path of bm_register_write() but the actual removal path has been leaking the write denial since the introduction of the flag.
Restore write access in put_binfmt_handler() before closing the interpreter file.
--- fs/binfmt_misc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/binfmt_misc.c +3 -1
@@ -162,8 +162,10 @@ static Node *get_binfmt_handler(struct binfmt_misc *misc,static void put_binfmt_handler (Node *e) {if (refcount_dec_and_test (&e->users)) { -if (e->flags &MISC_FMT_OPEN_FILE ) +if (e->flags &MISC_FMT_OPEN_FILE ) { +exe_file_allow_write_access (e->interp_file);filp_close (e->interp_file,NULL ); + }kfree (e); } }
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 2/23 binfmt_misc: use exe_file_deny_write_access() for the interpreter clone
In reply to Christian BraunerFor MISC_FMT_OPEN_FILE entries load_misc_binary() clones the registered interpreter file and denies write access to the clone via plain deny_write_access(). The clone is installed as bprm->interpreter and later released by the exec machinery through exe_file_allow_write_access() which skips the i_writecount increment for files with FMODE_FSNOTIFY_HSM set.
The deny and allow side can therefore come to different conclusions when pre-content watches are in play: if a pre-content watch is added to the interpreter after registration every subsequent exec through that entry takes a write denial on the clone that is never paired with a write allowance, driving the interpreter inode's i_writecount further down with each exec and leaving the interpreter unwritable even after the entry and all its users are gone.
Take the write denial via exe_file_deny_write_access() so both sides of the pairing base their decision on the same file mode, and propagate failure instead of silently ignoring it: an interpreter that is concurrently open for writing now fails the exec with ETXTBSY, exactly like an interpreter freshly opened via open_exec() would.
--- fs/binfmt_misc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
fs/binfmt_misc.c +8 -2
@@ -252,8 +252,14 @@ static int load_misc_binary(struct linux_binprm *bprm)if (fmt->flags &MISC_FMT_OPEN_FILE ) { interp_file =file_clone_open (fmt->interp_file); -if (!IS_ERR (interp_file)) -deny_write_access (interp_file); +if (!IS_ERR (interp_file)) { +int err =exe_file_deny_write_access (interp_file); + +if (err) { +fput (interp_file); + interp_file =ERR_PTR (err); + } + } }else { interp_file =open_exec (fmt->interpreter); }
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 3/23 binfmt_misc: convert entry list to an hlist
In reply to Christian BraunerThe 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.
--- 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
@@ -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;
@@ -662,8 +662,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) {
@@ -676,8 +676,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); }
@@ -698,7 +698,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 ); }
@@ -754,7 +754,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);
@@ -798,7 +798,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 ;
@@ -871,8 +871,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) {
@@ -898,7 +899,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);
@@ -973,7 +974,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
@@ -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
@@ -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
Christian Brauner
maintainerPATCH v2 4/23 binfmt_misc: use RCU for the handler lookup
In reply to Christian BraunerOnce 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.
* 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.
--- 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
@@ -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; }
@@ -166,7 +172,8 @@ static void put_binfmt_handler(Node *e)exe_file_allow_write_access (e->interp_file);filp_close (e->interp_file,NULL ); } -kfree (e); +/* Lockless walkers may still dereference this entry. */ +kfree_rcu (e, rcu); } }
@@ -675,10 +682,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); } }
@@ -697,9 +704,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 ); }
@@ -750,9 +757,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);
@@ -797,9 +806,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 ; }
@@ -895,9 +904,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);
@@ -975,7 +986,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
@@ -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
@@ -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
Re: [PATCH v2 04/23] binfmt_misc: use RCU for the handler lookup
Quoted 97 lines · Christian Brauner
Once
binfmt_miscis loadedload_misc_binary()runs for everyexecve()on the system sincebinfmt_miscregisters at the head of the formats list. Every exec therefore performsread_lock()andread_unlock()on theentries_lockof the relevantbinfmt_miscinstance, i.e., two atomic read-modify-writes on a shared cacheline. User namespaces without their ownbinfmt_miscmount fall back to an ancestor's instance so on container-heavy systems every exec on the machine typically ends up hammering the cacheline ofinit_binfmt_misc. OnPREEMPT_RTthe 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 viarefcount_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 preserveshlist_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.* Writers remain serialized by the inode lock of the root dentry with one exception.
bm_evict_inode()called fromgeneric_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_miscentry, 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 +5
-1@@ -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;
fs/binfmt_misc.c static Node *search_binfmt_handler(struct binfmt_misc *misc, · line 144
Quoted 8 lines
fs/binfmt_misc.c +4
-1@@ -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.
Right, unlink happens before refcount decrease everywhere.
Quoted 58 lines
fs/binfmt_misc.c +12
-12* * 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; }@@ -166,7 +172,8 @@ static void put_binfmt_handler(Node *e)exe_file_allow_write_access (e->interp_file);filp_close (e->interp_file,NULL ); } -kfree (e); +/* Lockless walkers may still dereference this entry. */ +kfree_rcu (e, rcu); } }@@ -675,10 +682,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); } }@@ -697,9 +704,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 ); }@@ -750,9 +757,11 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,* via bm_{entry,register,status}_write() inode_lock() on the
fs/binfmt_misc.c static ssize_t bm_entry_write(struct file *file, const char __user *buffer, · line 764
Quoted 10 lines
fs/binfmt_misc.c +5
-3* 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.
Why do we have to serialize with umount here? bm_entry_write has a live file as argument, so its inode cannot be evicted afaik, and therefore there cannot be a race in this case with bm_evict_inode(). And when evicting after calling locked_recursive_removal(), we have already unhashed and
write_lock(&misc->entries_lock); if (!list_empty(&e->list)) list_del_init(&e->list); write_unlock(&misc->entries_lock);
becomes a noop.
Quoted 70 lines
fs/binfmt_misc.c +9
-7*/ if (!hlist_unhashed (&e->node))remove_binfmt_handler (i_binfmt_misc (inode), e);@@ -797,9 +806,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 ; }@@ -895,9 +904,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);@@ -975,7 +986,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@@ -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@@ -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 · 2 lines
-- 2.53.0
Do you want me to readd the reviewed-by tags to this v2, or wait for v3?
Best,
Jori.
Christian Brauner
maintainerPATCH v2 5/23 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().
--- fs/binfmt_misc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
fs/binfmt_misc.c +5 -5
@@ -217,7 +217,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);
@@ -871,7 +871,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)); }
@@ -888,11 +888,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. */
@@ -1003,7 +1003,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
Christian Brauner
maintainerPATCH v2 6/23 binfmt_misc: turn the entry bit numbers into a proper enum
In reply to Christian BraunerEnabled and Magic are bit numbers in the flags word of an entry but are declared as bare, unprefixed enumerators with implicit values in a style that predates the git history. Give the enum a name, explicit bit numbers and namespaced names and use BIT() instead of open-coding the shifts when building the initial flags word in create_entry().
No functional change.
--- fs/binfmt_misc.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
fs/binfmt_misc.c +14 -10
@@ -42,7 +42,11 @@ enum { VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */ }; -enum {Enabled, Magic}; +/* Entry status and match type bit numbers. */ +enum binfmt_misc_entry_bits { +MISC_FMT_ENABLED_BIT =0 , +MISC_FMT_MAGIC_BIT =1 , +};#define MISC_FMT_PRESERVE_ARGV0 (1UL << 31)#define MISC_FMT_OPEN_BINARY (1UL << 30)#define MISC_FMT_CREDENTIALS (1UL << 29)
@@ -104,11 +108,11 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,int j;/* Make sure this one is currently enabled. */ -if (!test_bit (Enabled, &e->flags)) +if (!test_bit (MISC_FMT_ENABLED_BIT , &e->flags))continue ;/* Do matching based on extension if applicable. */ -if (!test_bit (Magic, &e->flags)) { +if (!test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) {if (p && !strcmp (e->magic, p +1 ))return e;continue ;
@@ -413,11 +417,11 @@ static Node *create_entry(const char __user *buffer, size_t count)switch (*p++) {case 'E' :pr_debug ("register: type: E (extension)\n" ); - e->flags =1 << Enabled; + e->flags =BIT (MISC_FMT_ENABLED_BIT );break ;case 'M' :pr_debug ("register: type: M (magic)\n" ); - e->flags = (1 << Enabled) | (1 << Magic); + e->flags =BIT (MISC_FMT_ENABLED_BIT ) |BIT (MISC_FMT_MAGIC_BIT );break ;default : goto einval;
@@ -425,7 +429,7 @@ static Node *create_entry(const char __user *buffer, size_t count)if (*p++ != del) goto einval; -if (test_bit (Magic, &e->flags)) { +if (test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) {/* Handle the 'M' (magic) format. */ char *s;
@@ -595,7 +599,7 @@ static void entry_status(Node *e, char *page)char *dp = page;const char *status ="disabled" ; -if (test_bit (Enabled, &e->flags)) +if (test_bit (MISC_FMT_ENABLED_BIT , &e->flags)) status ="enabled" ;if (!VERBOSE_STATUS ) {
@@ -617,7 +621,7 @@ static void entry_status(Node *e, char *page) *dp++ ='F' ; *dp++ ='\n' ; -if (!test_bit (Magic, &e->flags)) { +if (!test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) {sprintf (dp,"extension .%s\n" , e->magic); }else { dp +=sprintf (dp,"offset %i\nmagic " , e->offset);
@@ -741,11 +745,11 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,switch (res) {case 1 :/* Disable this handler. */ -clear_bit (Enabled, &e->flags); +clear_bit (MISC_FMT_ENABLED_BIT , &e->flags);break ;case 2 :/* Enable this handler. */ -set_bit (Enabled, &e->flags); +set_bit (MISC_FMT_ENABLED_BIT , &e->flags);break ;case 3 :/* Delete this handler. */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 7/23 binfmt_misc: turn the entry behavior flags into an enum
In reply to Christian BraunerThe MISC_FMT_* behavior flags are macros using unsigned long literals while the entry bit numbers right above them are now a proper enum. Move the flags into an enum as well so every flags word constant is declared in one form and shows up in debuginfo. (1U << N) keeps the enumerators within unsigned int range which is well-defined for enum constants and the values are unchanged when promoted to the unsigned long flags word.
No functional change.
--- fs/binfmt_misc.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
fs/binfmt_misc.c +8 -4
@@ -47,10 +47,14 @@ enum binfmt_misc_entry_bits {MISC_FMT_ENABLED_BIT =0 , MISC_FMT_MAGIC_BIT = 1, }; -#define MISC_FMT_PRESERVE_ARGV0 (1UL << 31) -#define MISC_FMT_OPEN_BINARY (1UL << 30) -#define MISC_FMT_CREDENTIALS (1UL << 29) -#define MISC_FMT_OPEN_FILE (1UL << 28) + +/* Entry behavior flags, fixed at registration time. */ +enum binfmt_misc_entry_flags { +MISC_FMT_PRESERVE_ARGV0 = (1U <<31 ), +MISC_FMT_OPEN_BINARY = (1U <<30 ), +MISC_FMT_CREDENTIALS = (1U <<29 ), +MISC_FMT_OPEN_FILE = (1U <<28 ), +}; typedef struct { struct hlist_node node;
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 8/23 binfmt_misc: rename Node to struct binfmt_misc_entry
In reply to Christian BraunerThe CamelCase Node typedef is a 1997 leftover and hides that this is a plain struct. Call it what it is: struct binfmt_misc_entry, matching struct binfmt_misc that it hangs off of and the entry bit and flag enums. Drop the typedef, switch the size computations in create_entry() to sizeof(*e) and adjust the comments that still referred to the old name.
No functional change.
--- fs/binfmt_misc.c | 60 +++++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 29 deletions(-)
fs/binfmt_misc.c +31 -29
@@ -56,7 +56,7 @@ enum binfmt_misc_entry_flags { MISC_FMT_OPEN_FILE = (1U << 28), }; -typedef struct { +struct binfmt_misc_entry {struct hlist_node node;unsigned long flags;/* type, status, etc. */ int offset;/* offset of magic */
@@ -69,7 +69,7 @@ typedef struct {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;
@@ -84,7 +84,7 @@ static struct file_system_type bm_fs_type;* - interp: ~50 bytes * - flags: 5 bytes * Round that up a bit, and then back off to hold the internal data -* (like struct Node). +* (like struct binfmt_misc_entry). */ #define MAX_REGISTER_LENGTH 1920
@@ -100,11 +100,11 @@ static struct file_system_type bm_fs_type;* * Return: binary type list entry on success, NULL on failure */ -static Node *search_binfmt_handler (struct binfmt_misc *misc, -struct linux_binprm *bprm) +static struct binfmt_misc_entry * +search_binfmt_handler (struct binfmt_misc *misc,struct linux_binprm *bprm) {char *p =strrchr (bprm->interp,'.' ); -Node *e; +struct binfmt_misc_entry *e;/* Walk all the registered handlers. */ hlist_for_each_entry_rcu (e, &misc->entries, node) {
@@ -153,10 +153,10 @@ static Node *search_binfmt_handler(struct binfmt_misc *misc,* * Return: binary type list entry on success, NULL on failure */ -static Node *get_binfmt_handler (struct binfmt_misc *misc, -struct linux_binprm *bprm) +static struct binfmt_misc_entry *get_binfmt_handler (struct binfmt_misc *misc, +struct linux_binprm *bprm) { -Node *e; +struct binfmt_misc_entry *e;guard (rcu)(); do {
@@ -166,14 +166,14 @@ static Node *get_binfmt_handler(struct binfmt_misc *misc, }/** -* put_binfmt_handler - put binary handler node -* @e: node to put +* put_binfmt_handler - put binary handler entry +* @e: entry to put * -* Free node syncing with load_misc_binary() and defer final free to +* Free entry syncing with load_misc_binary() and defer final free to * load_misc_binary() in case it is using the binary type handler we were * requested to remove. */ -static void put_binfmt_handler (Node *e) +static void put_binfmt_handler (struct binfmt_misc_entry *e) {if (refcount_dec_and_test (&e->users)) {if (e->flags &MISC_FMT_OPEN_FILE ) {
@@ -219,7 +219,7 @@ static struct binfmt_misc *load_binfmt_misc(void)*/ static int load_misc_binary (struct linux_binprm *bprm) { -Node *fmt; +struct binfmt_misc_entry *fmt;struct file *interp_file =NULL ;int retval = -ENOEXEC ;struct binfmt_misc *misc;
@@ -290,7 +290,7 @@ static int load_misc_binary(struct linux_binprm *bprm) ret:/* -* If we actually put the node here all concurrent calls to +* If we actually put the entry here all concurrent calls to * load_misc_binary() will have finished. We also know * that for the refcount to be zero someone must have concurently * removed the binary type handler from the list and it's our job to
@@ -326,7 +326,7 @@ static char *scanarg(char *s, char del)return s; } -static char *check_special_flags (char *sfs,Node *e) +static char *check_special_flags (char *sfs,struct binfmt_misc_entry *e) {char *p = sfs;int cont =1 ;
@@ -370,9 +370,10 @@ static char *check_special_flags(char *sfs, Node *e)* ':name:type:offset:magic:mask:interpreter:flags' * where the ':' is the IFS, that can be chosen with the first char */ -static Node *create_entry (const char __user *buffer,size_t count) +static struct binfmt_misc_entry *create_entry (const char __user *buffer, +size_t count) { -Node *e; +struct binfmt_misc_entry *e;int memsize, err;char *buf, *p;char del;
@@ -385,14 +386,14 @@ static Node *create_entry(const char __user *buffer, size_t count) goto out; err = -ENOMEM ; - memsize =sizeof (Node) + count +8 ; + memsize =sizeof (*e) + count +8 ; e =kmalloc (memsize,GFP_KERNEL_ACCOUNT );if (!e) goto out; - p = buf = (char *)e +sizeof (Node); + p = buf = (char *)e +sizeof (*e); -memset (e,0 ,sizeof (Node)); +memset (e,0 ,sizeof (*e));if (copy_from_user (buf, buffer, count)) goto efault;
@@ -598,7 +599,7 @@ static int parse_command(const char __user *buffer, size_t count)/* generic stuff */ -static void entry_status (Node *e,char *page) +static void entry_status (struct binfmt_misc_entry *e,char *page) {char *dp = page;const char *status ="disabled" ;
@@ -682,7 +683,7 @@ static struct binfmt_misc *i_binfmt_misc(struct inode *inode)*/ static void bm_evict_inode (struct inode *inode) { -Node *e = inode->i_private; +struct binfmt_misc_entry *e = inode->i_private;clear_inode (inode);
@@ -710,7 +711,8 @@ static void bm_evict_inode(struct inode *inode)* to use writes to files in order to delete binary type handlers. But it has * worked for so long that it's not a pressing issue. */ -static void remove_binfmt_handler (struct binfmt_misc *misc,Node *e) +static void remove_binfmt_handler (struct binfmt_misc *misc, +struct binfmt_misc_entry *e) {spin_lock (&misc->entries_lock);hlist_del_init_rcu (&e->node);
@@ -723,7 +725,7 @@ static void remove_binfmt_handler(struct binfmt_misc *misc, Node *e)static ssize_t bm_entry_read (struct file *file,char __user *buf,size_t nbytes,loff_t *ppos) { -Node *e =file_inode (file)->i_private; +struct binfmt_misc_entry *e =file_inode (file)->i_private;ssize_t res;char *page;
@@ -743,7 +745,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) { struct inode *inode = file_inode(file); -Node *e = inode->i_private; +struct binfmt_misc_entry *e = inode->i_private;int res =parse_command (buffer, count);switch (res) {
@@ -792,7 +794,7 @@ static const struct file_operations bm_entry_operations = {/* /register */ /* add to filesystem */ -static int add_entry (Node *e,struct super_block *sb) +static int add_entry (struct binfmt_misc_entry *e,struct super_block *sb) {struct dentry *dentry =simple_start_creating (sb->s_root, e->name);struct inode *inode;
@@ -824,7 +826,7 @@ static int add_entry(Node *e, struct super_block *sb)static ssize_t bm_register_write (struct file *file,const char __user *buffer,size_t count,loff_t *ppos) { -Node *e; +struct binfmt_misc_entry *e;struct super_block *sb =file_inode (file)->i_sb;int err =0 ;struct file *f =NULL ;
@@ -890,7 +892,7 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,int res =parse_command (buffer, count);struct hlist_node *next;struct inode *inode; -Node *e; +struct binfmt_misc_entry *e; misc =i_binfmt_misc (file_inode (file));switch (res) {
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 9/23 binfmt_misc: remove the VERBOSE_STATUS toggle
In reply to Christian BraunerVERBOSE_STATUS is a compile-time constant that has been fixed to 1 for as long as git history reaches. Turning it off requires editing the source and yields entry files that only ever report "enabled"/"disabled", a format nothing has ever seen in the wild. Remove the pretend knob and the dead branch it guards.
--- fs/binfmt_misc.c | 9 --------- 1 file changed, 9 deletions(-)
fs/binfmt_misc.c +0 -9
@@ -38,10 +38,6 @@# define USE_DEBUG 0#endif -enum { -VERBOSE_STATUS =1 /* make it zero to save 400 bytes kernel memory */ -}; -/* Entry status and match type bit numbers. */ enum binfmt_misc_entry_bits {MISC_FMT_ENABLED_BIT =0 ,
@@ -607,11 +603,6 @@ static void entry_status(struct binfmt_misc_entry *e, char *page)if (test_bit (MISC_FMT_ENABLED_BIT , &e->flags)) status ="enabled" ; -if (!VERBOSE_STATUS ) { -sprintf (page,"%s\n" , status); -return ; - } - dp +=sprintf (dp,"%s\ninterpreter %s\n" , status, e->interpreter);/* print the special flags */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 10/23 binfmt_misc: use print_hex_dump_debug() for the register debug output
In reply to Christian BraunerThe hex dumps in create_entry() are compiled out unless someone edits the file to define DEBUG while the pr_debug() calls right next to them are dynamic-debug aware. Switch the dumps to print_hex_dump_debug() which follows the same rules as pr_debug() so the register parsing debug output is uniformly controlled through dynamic debug, and remove the USE_DEBUG machinery.
Drop the magic[masked] dump instead of converting it: it printed the bitwise AND of two buffers dumped right above it and required a temporary allocation on every registration just to recompute what the reader can derive from the magic and mask dumps directly.
--- fs/binfmt_misc.c | 52 +++++++++++++++------------------------------------- 1 file changed, 15 insertions(+), 37 deletions(-)
fs/binfmt_misc.c +15 -37
@@ -32,12 +32,6 @@#include "internal.h" -#ifdef DEBUG -# define USE_DEBUG 1 -#else -# define USE_DEBUG 0 -#endif -/* Entry status and match type bit numbers. */ enum binfmt_misc_entry_bits {MISC_FMT_ENABLED_BIT =0 ,
@@ -456,10 +450,9 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, goto einval;if (!e->magic[0 ]) goto einval; -if (USE_DEBUG ) -print_hex_dump_bytes ( -KBUILD_MODNAME ": register: magic[raw]: " , -DUMP_PREFIX_NONE , e->magic, p - e->magic); +print_hex_dump_debug ( +KBUILD_MODNAME ": register: magic[raw]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->magic, p - e->magic, true);/* Parse the 'mask' field. */ e->mask = p;
@@ -469,10 +462,12 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,if (!e->mask[0 ]) { e->mask =NULL ;pr_debug ("register: mask[raw]: none\n" ); - }else if (USE_DEBUG ) -print_hex_dump_bytes ( + }else { +print_hex_dump_debug (KBUILD_MODNAME ": register: mask[raw]: " , -DUMP_PREFIX_NONE , e->mask, p - e->mask); +DUMP_PREFIX_NONE ,16 ,1 , e->mask, p - e->mask, + true); + }/* * Decode the magic & mask fields.
@@ -488,30 +483,13 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, BINPRM_BUF_SIZE - e->size < e->offset) goto einval;pr_debug ("register: magic/mask length: %i\n" , e->size); -if (USE_DEBUG ) { -print_hex_dump_bytes ( -KBUILD_MODNAME ": register: magic[decoded]: " , -DUMP_PREFIX_NONE , e->magic, e->size); - -if (e->mask) { -int i; -char *masked =kmalloc (e->size,GFP_KERNEL_ACCOUNT ); - -print_hex_dump_bytes ( -KBUILD_MODNAME ": register: mask[decoded]: " , -DUMP_PREFIX_NONE , e->mask, e->size); - -if (masked) { -for (i =0 ; i < e->size; ++i) - masked[i] = e->magic[i] & e->mask[i]; -print_hex_dump_bytes ( -KBUILD_MODNAME ": register: magic[masked]: " , -DUMP_PREFIX_NONE , masked, e->size); - -kfree (masked); - } - } - } +print_hex_dump_debug ( +KBUILD_MODNAME ": register: magic[decoded]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->magic, e->size, true); +if (e->mask) +print_hex_dump_debug ( +KBUILD_MODNAME ": register: mask[decoded]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->mask, e->size, true); }else {/* Handle the 'E' (extension) format. */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 11/23 binfmt_misc: convert the entry file to seq_file
In reply to Christian BraunerReading an entry file allocates a whole page and formats the status into it with a chain of manually advanced sprintf() calls, silently relying on MAX_REGISTER_LENGTH plus the hex-expanded magic and mask always staying below PAGE_SIZE. Convert the read side to seq_file which sizes its buffer as needed and gets rid of the open-coded pointer arithmetic including the last bin2hex() user in the file. The output is byte for byte identical.
seq_open() clears FMODE_PWRITE for historical reasons and would silently turn pwrite() on entry files into -ESPIPE even though bm_entry_write() accepts writes at any offset. Restore the flag in bm_entry_open() the same way kernfs does for its seq_file backed files so pwrite() keeps working.
The only user-visible difference is that seeking is now bound by seq_lseek() instead of default_llseek(), i.e. SEEK_END stops working on entry files, which nothing can sensibly use anyway.
The status file keeps its simple_read_from_buffer() as it only ever returns one of two fixed strings.
--- fs/binfmt_misc.c | 74 +++++++++++++++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 35 deletions(-)
fs/binfmt_misc.c +39 -35
@@ -12,7 +12,6 @@#include <linux/kernel.h> #include <linux/module.h> -#include <linux/hex.h> #include <linux/init.h> #include <linux/sched/mm.h> #include <linux/magic.h>
@@ -25,6 +24,7 @@#include <linux/namei.h> #include <linux/mount.h> #include <linux/rculist.h> +#include <linux/seq_file.h> #include <linux/fs_context.h> #include <linux/syscalls.h> #include <linux/fs.h>
@@ -573,40 +573,47 @@ static int parse_command(const char __user *buffer, size_t count)/* generic stuff */ -static void entry_status (struct binfmt_misc_entry *e,char *page) +static void bm_seq_hex (struct seq_file *m,const u8 *data,int size) { -char *dp = page; -const char *status ="disabled" ; +for (int i =0 ; i < size; i++) +seq_printf (m,"%02x" , data[i]); +} + +static int bm_entry_show (struct seq_file *m,void *unused) +{ +struct binfmt_misc_entry *e = m->private;if (test_bit (MISC_FMT_ENABLED_BIT , &e->flags)) - status ="enabled" ; +seq_puts (m,"enabled\n" ); +else +seq_puts (m,"disabled\n" ); - dp +=sprintf (dp,"%s\ninterpreter %s\n" , status, e->interpreter); +seq_printf (m,"interpreter %s\n" , e->interpreter);/* print the special flags */ - dp +=sprintf (dp,"flags: " ); +seq_puts (m,"flags: " );if (e->flags &MISC_FMT_PRESERVE_ARGV0 ) - *dp++ ='P' ; +seq_putc (m,'P' );if (e->flags &MISC_FMT_OPEN_BINARY ) - *dp++ ='O' ; +seq_putc (m,'O' );if (e->flags &MISC_FMT_CREDENTIALS ) - *dp++ ='C' ; +seq_putc (m,'C' );if (e->flags &MISC_FMT_OPEN_FILE ) - *dp++ ='F' ; - *dp++ ='\n' ; +seq_putc (m,'F' ); +seq_putc (m,'\n' );if (!test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) { -sprintf (dp,"extension .%s\n" , e->magic); +seq_printf (m,"extension .%s\n" , e->magic); }else { - dp +=sprintf (dp,"offset %i\nmagic " , e->offset); - dp =bin2hex (dp, e->magic, e->size); +seq_printf (m,"offset %i\nmagic " , e->offset); +bm_seq_hex (m, e->magic, e->size);if (e->mask) { - dp +=sprintf (dp,"\nmask " ); - dp =bin2hex (dp, e->mask, e->size); +seq_puts (m,"\nmask " ); +bm_seq_hex (m, e->mask, e->size); } - *dp++ ='\n' ; - *dp ='\0' ; +seq_putc (m,'\n' ); } +return 0 ; }static struct inode *bm_get_inode (struct super_block *sb,int mode)
@@ -691,23 +698,18 @@ static void remove_binfmt_handler(struct binfmt_misc *misc,/* /<entry> */ -static ssize_t -bm_entry_read (struct file *file,char __user *buf,size_t nbytes,loff_t *ppos) +static int bm_entry_open (struct inode *inode,struct file *file) { -struct binfmt_misc_entry *e =file_inode (file)->i_private; -ssize_t res; -char *page; +int ret; - page =kmalloc (PAGE_SIZE ,GFP_KERNEL ); -if (!page) -return -ENOMEM ; - -entry_status (e, page); - - res =simple_read_from_buffer (buf, nbytes, ppos, page,strlen (page)); + ret =single_open (file, bm_entry_show, inode->i_private); +if (ret) +return ret; -kfree (page); -return res; +/* seq_open() clears FMODE_PWRITE, bm_entry_write() takes any offset */ +if (file->f_mode &FMODE_WRITE ) + file->f_mode |=FMODE_PWRITE ; +return 0 ; } static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
@@ -755,9 +757,11 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer, }static const struct file_operations bm_entry_operations = { - .read = bm_entry_read, + .open = bm_entry_open, + .read = seq_read, .write = bm_entry_write, - .llseek = default_llseek, + .llseek = seq_lseek, + .release = single_release, };/* /register */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 12/23 binfmt_misc: factor out the entry matching
In reply to Christian Braunersearch_binfmt_handler() open-codes both match types in one loop body with the maskless magic comparison spelled as a manual xor loop that is just memcmp() in disguise. Move the extension and magic checks into helpers so the walk reads as policy - skip disabled entries, match by entry type - and the maskless case actually uses memcmp().
No functional change.
--- fs/binfmt_misc.c | 50 +++++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 21 deletions(-)
fs/binfmt_misc.c +29 -21
@@ -78,6 +78,29 @@ static struct file_system_type bm_fs_type;*/ #define MAX_REGISTER_LENGTH 1920 +/* Check if @e's magic matches @bprm's buffer, applying the mask if set. */ +static bool entry_matches_magic (const struct binfmt_misc_entry *e, +const struct linux_binprm *bprm) +{ +const char *s = bprm->buf + e->offset; +int i; + +if (!e->mask) +return !memcmp (s, e->magic, e->size); + +for (i =0 ; i < e->size; i++) +if ((s[i] ^ e->magic[i]) & e->mask[i]) +return false; +return true; +} + +/* Check if @e's registered extension matches @ext, NULL if there is none. */ +static bool entry_matches_extension (const struct binfmt_misc_entry *e, +const char *ext) +{ +return ext && !strcmp (e->magic, ext); +} +/** * search_binfmt_handler - search for a binary handler for @bprm * @misc: handle to binfmt_misc instance
@@ -93,38 +116,23 @@ static struct file_system_type bm_fs_type;static struct binfmt_misc_entry *search_binfmt_handler (struct binfmt_misc *misc,struct linux_binprm *bprm) { -char *p =strrchr (bprm->interp,'.' ); +char *dot =strrchr (bprm->interp,'.' ); +const char *ext = dot ? dot +1 :NULL ;struct binfmt_misc_entry *e;/* Walk all the registered handlers. */ hlist_for_each_entry_rcu (e, &misc->entries, node) { -char *s; -int j; -/* Make sure this one is currently enabled. */ if (!test_bit (MISC_FMT_ENABLED_BIT , &e->flags))continue ; -/* Do matching based on extension if applicable. */ -if (!test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) { -if (p && !strcmp (e->magic, p +1 )) +if (test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) { +if (entry_matches_magic (e, bprm))return e; -continue ; - } - -/* Do matching based on magic & mask. */ - s = bprm->buf + e->offset; -if (e->mask) { -for (j =0 ; j < e->size; j++) -if ((*s++ ^ e->magic[j]) & e->mask[j]) -break ; }else { -for (j =0 ; j < e->size; j++) -if ((*s++ ^ e->magic[j])) -break ; +if (entry_matches_extension (e, ext)) +return e; } -if (j == e->size) -return e; }return NULL ;
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 13/23 binfmt_misc: rename load_binfmt_misc() to current_binfmt_misc()
In reply to Christian Braunerload_binfmt_misc() is one word swap away from load_misc_binary(), the binfmt loader it serves. It doesn't load anything, it looks up the binfmt_misc instance of the caller's user namespace, so name it after what it returns in the style of current_user_ns() and friends. Tighten the parent walk into a for loop and fix the stale wording and typos in the kernel-doc while at it.
--- fs/binfmt_misc.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
fs/binfmt_misc.c +11 -13
@@ -184,29 +184,27 @@ static void put_binfmt_handler(struct binfmt_misc_entry *e) }/** -* load_binfmt_misc - load the binfmt_misc of the caller's user namespace +* current_binfmt_misc - get the binfmt_misc instance of the caller's user namespace * -* To be called in load_misc_binary() to load the relevant struct binfmt_misc. -* If a user namespace doesn't have its own binfmt_misc mount it can make use -* of its ancestor's binfmt_misc handlers. This mimicks the behavior of -* pre-namespaced binfmt_misc where all registered binfmt_misc handlers where -* available to all user and user namespaces on the system. +* If a user namespace doesn't have its own binfmt_misc mount it uses the +* handlers of its closest ancestor with one. This mimics the behavior of +* pre-namespaced binfmt_misc where all registered handlers were available +* to all users and user namespaces on the system. The init user namespace +* instance is statically set up so the fallback is never reached in +* practice. * * Return: the binfmt_misc instance of the caller's user namespace */ -static struct binfmt_misc *load_binfmt_misc (void ) +static struct binfmt_misc *current_binfmt_misc (void ) {const struct user_namespace *user_ns;struct binfmt_misc *misc; - user_ns =current_user_ns (); -while (user_ns) { +for (user_ns =current_user_ns (); user_ns; user_ns = user_ns->parent) {/* Pairs with smp_store_release() in bm_fill_super(). */ misc =smp_load_acquire (&user_ns->binfmt_misc);if (misc)return misc; - - user_ns = user_ns->parent; }return &init_binfmt_misc;
@@ -222,7 +220,7 @@ static int load_misc_binary(struct linux_binprm *bprm)int retval = -ENOEXEC ;struct binfmt_misc *misc; - misc =load_binfmt_misc (); + misc =current_binfmt_misc ();if (!READ_ONCE (misc->enabled))return retval;
@@ -979,7 +977,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)INIT_HLIST_HEAD (&misc->entries);spin_lock_init (&misc->entries_lock); -/* Pairs with smp_load_acquire() in load_binfmt_misc(). */ +/* Pairs with smp_load_acquire() in current_binfmt_misc(). */ smp_store_release (&user_ns->binfmt_misc, misc); }
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 14/23 binfmt_misc: return errors directly in load_misc_binary()
In reply to Christian Braunerload_misc_binary() seeds retval with the error for checks that happen further down, reassigns it along the way and funnels every exit through a ret label whose only job is dropping the entry reference, so figuring out what an early return actually returns means replaying the assignment history. Give put_binfmt_handler() a cleanup class and take the reference with __free() so every failure can return its error right where the condition is checked. The comment at the label restated what the put_binfmt_handler() kernel-doc already explains, it goes with the label. Drop the dead NULL initialization of interp_file which is assigned on all paths before use.
No functional change.
--- fs/binfmt_misc.c | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-)
fs/binfmt_misc.c +14 -27
@@ -183,6 +183,8 @@ static void put_binfmt_handler(struct binfmt_misc_entry *e) } } +DEFINE_FREE(put_binfmt_handler, struct binfmt_misc_entry *, if (_T) put_binfmt_handler(_T)) +/** * current_binfmt_misc - get the binfmt_misc instance of the caller's user namespace *
@@ -215,30 +217,29 @@ static struct binfmt_misc *current_binfmt_misc(void)*/ static int load_misc_binary (struct linux_binprm *bprm) { -struct binfmt_misc_entry *fmt; -struct file *interp_file =NULL ; -int retval = -ENOEXEC ; + struct binfmt_misc_entry *fmt __free(put_binfmt_handler) = NULL; +struct file *interp_file;struct binfmt_misc *misc; +int retval; misc =current_binfmt_misc ();if (!READ_ONCE (misc->enabled)) -return retval; +return -ENOEXEC ; fmt =get_binfmt_handler (misc, bprm);if (!fmt) -return retval; +return -ENOEXEC ;/* Need to be able to load the file after exec */ - retval = -ENOENT ;if (bprm->interp_flags &BINPRM_FLAGS_PATH_INACCESSIBLE ) - goto ret; +return -ENOENT ;if (fmt->flags &MISC_FMT_PRESERVE_ARGV0 ) { bprm->interp_flags |=BINPRM_FLAGS_PRESERVE_ARGV0 ; }else { retval =remove_arg_zero (bprm);if (retval) - goto ret; +return retval; }if (fmt->flags &MISC_FMT_OPEN_BINARY )
@@ -247,19 +248,19 @@ static int load_misc_binary(struct linux_binprm *bprm)/* make argv[1] be the path to the binary */ retval =copy_string_kernel (bprm->interp, bprm);if (retval <0 ) - goto ret; +return retval; bprm->argc++;/* add the interp as argv[0] */ retval =copy_string_kernel (fmt->interpreter, bprm);if (retval <0 ) - goto ret; +return retval; bprm->argc++;/* Update interp in case binfmt_script needs it. */ retval =bprm_change_interp (fmt->interpreter, bprm);if (retval <0 ) - goto ret; +return retval;if (fmt->flags &MISC_FMT_OPEN_FILE ) { interp_file =file_clone_open (fmt->interp_file);
@@ -274,27 +275,13 @@ static int load_misc_binary(struct linux_binprm *bprm) }else { interp_file =open_exec (fmt->interpreter); } - retval =PTR_ERR (interp_file);if (IS_ERR (interp_file)) - goto ret; +return PTR_ERR (interp_file); bprm->interpreter = interp_file;if (fmt->flags &MISC_FMT_CREDENTIALS ) bprm->execfd_creds =1 ; - - retval =0 ; -ret: - -/* -* If we actually put the entry here all concurrent calls to -* load_misc_binary() will have finished. We also know -* that for the refcount to be zero someone must have concurently -* removed the binary type handler from the list and it's our job to -* free it. -*/ -put_binfmt_handler (fmt); - -return retval; +return 0 ; }/* Command parsers */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 15/23 binfmt_misc: give the parse_command() results names
In reply to Christian Braunerparse_command() maps "0" to 1, "1" to 2 and "-1" to 3 and the write handlers switch on those bare numbers, leaving every reader to redo the mapping in their head. Name the commands and drop the per-case comments that only existed to translate the numbers back.
No functional change.
--- fs/binfmt_misc.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)
fs/binfmt_misc.c +20 -18
@@ -539,9 +539,17 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,return ERR_PTR (-EINVAL ); } +/* Commands accepted by the /status and /<entry> files. */ +enum bm_command { +BM_CMD_IGNORE ,/* empty write */ +BM_CMD_DISABLE ,/* "0" */ +BM_CMD_ENABLE ,/* "1" */ +BM_CMD_REMOVE ,/* "-1" */ +}; +/* -* Set status of entry/binfmt_misc: -* '1' enables, '0' disables and '-1' clears entry/binfmt_misc +* Parse what userspace wrote to /status or an entry file: '1' enables, +* '0' disables and '-1' removes the entry or all entries. */ static int parse_command (const char __user *buffer,size_t count) {
@@ -552,15 +560,15 @@ static int parse_command(const char __user *buffer, size_t count)if (copy_from_user (s, buffer, count))return -EFAULT ;if (!count) -return 0 ; +return BM_CMD_IGNORE ;if (s[count -1 ] =='\n' ) count--;if (count ==1 && s[0 ] =='0' ) -return 1 ; +return BM_CMD_DISABLE ;if (count ==1 && s[0 ] =='1' ) -return 2 ; +return BM_CMD_ENABLE ;if (count ==2 && s[0 ] =='-' && s[1 ] =='1' ) -return 3 ; +return BM_CMD_REMOVE ;return -EINVAL ; }
@@ -713,16 +721,13 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,int res =parse_command (buffer, count);switch (res) { -case 1 : -/* Disable this handler. */ +case BM_CMD_DISABLE :clear_bit (MISC_FMT_ENABLED_BIT , &e->flags);break ; -case 2 : -/* Enable this handler. */ +case BM_CMD_ENABLE :set_bit (MISC_FMT_ENABLED_BIT , &e->flags);break ; -case 3 : -/* Delete this handler. */ +case BM_CMD_REMOVE : inode =d_inode (inode->i_sb->s_root);inode_lock_nested (inode,I_MUTEX_PARENT );
@@ -862,16 +867,13 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer, misc =i_binfmt_misc (file_inode (file));switch (res) { -case 1 : -/* Disable all handlers. */ +case BM_CMD_DISABLE :WRITE_ONCE (misc->enabled, false);break ; -case 2 : -/* Enable all handlers. */ +case BM_CMD_ENABLE :WRITE_ONCE (misc->enabled, true);break ; -case 3 : -/* Delete all handlers. */ +case BM_CMD_REMOVE : inode =d_inode (file_inode (file)->i_sb->s_root);inode_lock_nested (inode,I_MUTEX_PARENT );
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 16/23 binfmt_misc: factor out the entry removal
In reply to Christian BraunerBoth write handlers open-code the same removal dance - grab the root inode lock, unlink, unlock - each carrying a verbatim copy of the same eleven-line locking comment, and bm_entry_write() reuses its inode variable for the root inode halfway through to pull it off. Move the dance into bm_remove_entry() and bm_remove_all_entries() and the locking rules into the kernel-doc of remove_binfmt_handler() which both helpers wrap.
No functional change.
--- fs/binfmt_misc.c | 84 +++++++++++++++++++++++++++----------------------------- 1 file changed, 40 insertions(+), 44 deletions(-)
fs/binfmt_misc.c +40 -44
@@ -682,11 +682,19 @@ static void bm_evict_inode(struct inode *inode)* @e: binary type handler to remove * * Remove a binary type handler from the list of binary type handlers and -* remove its associated dentry. This is called from -* binfmt_{entry,status}_write(). In the future, we might want to think about -* adding a proper ->unlink() method to binfmt_misc instead of forcing caller's -* to use writes to files in order to delete binary type handlers. But it has -* worked for so long that it's not a pressing issue. +* remove its associated dentry. +* +* Adding and removing entries via bm_{entry,register,status}_write() +* happens under the exclusively held inode lock of the root dentry keeping +* the list stable for writers. load_misc_binary() walks it concurrently +* under RCU. The entries_lock is only held around the actual unlink to +* serialize against bm_evict_inode() which unlinks entries during umount +* without holding the root inode lock. +* +* In the future, we might want to think about adding a proper ->unlink() +* method to binfmt_misc instead of forcing callers to use writes to files +* in order to delete binary type handlers. But it has worked for so long +* that it's not a pressing issue. */ static void remove_binfmt_handler (struct binfmt_misc *misc,struct binfmt_misc_entry *e)
@@ -697,6 +705,31 @@ static void remove_binfmt_handler(struct binfmt_misc *misc,locked_recursive_removal (e->dentry,NULL ); } +/* Remove @e unless a concurrent write already unlinked it. */ +static void bm_remove_entry (struct binfmt_misc_entry *e,struct super_block *sb) +{ +struct inode *root =d_inode (sb->s_root); + +inode_lock_nested (root,I_MUTEX_PARENT ); +if (!hlist_unhashed (&e->node)) +remove_binfmt_handler (i_binfmt_misc (root), e); +inode_unlock (root); +} + +/* Remove all entries of the binfmt_misc instance @misc belonging to @sb. */ +static void bm_remove_all_entries (struct binfmt_misc *misc, +struct super_block *sb) +{ +struct inode *root =d_inode (sb->s_root); +struct binfmt_misc_entry *e; +struct hlist_node *next; + +inode_lock_nested (root,I_MUTEX_PARENT ); +hlist_for_each_entry_safe (e, next, &misc->entries, node) +remove_binfmt_handler (misc, e); +inode_unlock (root); +} +/* /<entry> */ static int bm_entry_open (struct inode *inode,struct file *file)
@@ -728,24 +761,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,set_bit (MISC_FMT_ENABLED_BIT , &e->flags);break ;case BM_CMD_REMOVE : - inode =d_inode (inode->i_sb->s_root); -inode_lock_nested (inode,I_MUTEX_PARENT ); - -/* -* In order to add new element or remove elements from the list -* 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 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); - -inode_unlock (inode); +bm_remove_entry (e, inode->i_sb);break ;default :return res;
@@ -861,9 +877,6 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer, {struct binfmt_misc *misc;int res =parse_command (buffer, count); -struct hlist_node *next; -struct inode *inode; -struct binfmt_misc_entry *e; misc =i_binfmt_misc (file_inode (file));switch (res) {
@@ -874,24 +887,7 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,WRITE_ONCE (misc->enabled, true);break ;case BM_CMD_REMOVE : - inode =d_inode (file_inode (file)->i_sb->s_root); -inode_lock_nested (inode,I_MUTEX_PARENT ); - -/* -* In order to add new element or remove elements from the list -* 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 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); - -inode_unlock (inode); +bm_remove_all_entries (misc,file_inode (file)->i_sb);break ;default :return res;
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 17/23 binfmt_misc: simplify check_special_flags()
In reply to Christian BraunerReplace the cont flag and the pointer increment repeated in every case with a for loop that returns from the default case, and shrink the multi-line 'C implies O' remark to one line.
No functional change.
--- fs/binfmt_misc.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-)
fs/binfmt_misc.c +5 -17
@@ -309,43 +309,31 @@ static char *scanarg(char *s, char del)return s; } -static char *check_special_flags (char *sfs,struct binfmt_misc_entry *e) +static char *check_special_flags (char *p,struct binfmt_misc_entry *e) { -char *p = sfs; -int cont =1 ; - -/* special flags */ -while (cont) { +for (;; p++) {switch (*p) {case 'P' :pr_debug ("register: flag: P (preserve argv0)\n" ); - p++; e->flags |=MISC_FMT_PRESERVE_ARGV0 ;break ;case 'O' :pr_debug ("register: flag: O (open binary)\n" ); - p++; e->flags |=MISC_FMT_OPEN_BINARY ;break ;case 'C' :pr_debug ("register: flag: C (preserve creds)\n" ); - p++; -/* this flags also implies the -open-binary flag */ - e->flags |= (MISC_FMT_CREDENTIALS | -MISC_FMT_OPEN_BINARY ); +/* C implies O */ + e->flags |=MISC_FMT_CREDENTIALS |MISC_FMT_OPEN_BINARY ;break ;case 'F' :pr_debug ("register: flag: F: open interpreter file now\n" ); - p++; e->flags |=MISC_FMT_OPEN_FILE ;break ;default : - cont =0 ; +return p; } } - -return p; }/*
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 18/23 binfmt_misc: use a flexible array member for the register string
In reply to Christian Braunercreate_entry() allocates the entry and the register string it parses into in one chunk and finds the string part again through manual pointer arithmetic behind a cast. Make the layout explicit with a flexible array member and struct_size(), and give the magic pad of trailing delimiters a name while at it.
No functional change.
--- fs/binfmt_misc.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
fs/binfmt_misc.c +9 -5
@@ -59,6 +59,7 @@ struct binfmt_misc_entry {struct file *interp_file;refcount_t users;/* sync removal with load_misc_binary() */ struct rcu_head rcu; +char buf[];/* register string, fields point in here */ };static struct file_system_type bm_fs_type;
@@ -78,6 +79,9 @@ static struct file_system_type bm_fs_type;*/ #define MAX_REGISTER_LENGTH 1920 +/* Trailing delimiter pad so field parsing always terminates at a delimiter. */ +#define MISC_DELIM_PAD 8 +/* Check if @e's magic matches @bprm's buffer, applying the mask if set. */ static bool entry_matches_magic (const struct binfmt_misc_entry *e,const struct linux_binprm *bprm)
@@ -345,9 +349,9 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, size_t count) {struct binfmt_misc_entry *e; -int memsize, err;char *buf, *p;char del; +int err;pr_debug ("register: received %zu bytes\n" , count);
@@ -357,12 +361,12 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, goto out; err = -ENOMEM ; - memsize =sizeof (*e) + count +8 ; - e =kmalloc (memsize,GFP_KERNEL_ACCOUNT ); + e =kmalloc (struct_size (e, buf, count +MISC_DELIM_PAD ), +GFP_KERNEL_ACCOUNT );if (!e) goto out; - p = buf = (char *)e +sizeof (*e); + p = buf = e->buf;memset (e,0 ,sizeof (*e));if (copy_from_user (buf, buffer, count))
@@ -373,7 +377,7 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,pr_debug ("register: delim: %#x {%c}\n" , del, del);/* Pad the buffer with the delim to simplify parsing below. */ -memset (buf + count, del,8 ); +memset (buf + count, del,MISC_DELIM_PAD );/* Parse the 'name' field. */ e->name = p;
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 19/23 binfmt_misc: split the field parsing out of create_entry()
In reply to Christian Braunercreate_entry() is a two hundred line parser with the M and E field handling inlined as the two arms of its largest branch. Move them into parse_magic_fields() and parse_extension_fields() which return the new parse position or NULL so create_entry() itself reads like the register string grammar again.
The offset parsing loses a provably dead check on the way: after *s = '\0' and p = s the subsequent if (*p++) always reads the just written NUL byte and can never fail, it only obscured that the code simply advances past the delimiter.
With the field parsing gone every remaining failure unwinds the same way, so hand the entry to __free(kfree), return errors directly and pass ownership out via no_free_ptr() on success instead of routing every exit through goto tails.
--- fs/binfmt_misc.c | 223 ++++++++++++++++++++++++++----------------------------- 1 file changed, 107 insertions(+), 116 deletions(-)
fs/binfmt_misc.c +107 -116
@@ -340,6 +340,95 @@ static char *check_special_flags(char *p, struct binfmt_misc_entry *e) } } +/* Parse the 'offset', 'magic' and 'mask' fields of an 'M' entry. */ +static char *parse_magic_fields (struct binfmt_misc_entry *e,char *p,char del) +{ +char *s; + +/* Parse the 'offset' field. */ + s =strchr (p, del); +if (!s) +return NULL ; + *s ='\0' ; +if (p != s) { +if (kstrtoint (p,10 , &e->offset) || e->offset <0 ) +return NULL ; + } + p = s +1 ; +pr_debug ("register: offset: %#x\n" , e->offset); + +/* Parse the 'magic' field. */ + e->magic = p; + p =scanarg (p, del); +if (!p || !e->magic[0 ]) +return NULL ; +print_hex_dump_debug ( +KBUILD_MODNAME ": register: magic[raw]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->magic, p - e->magic, true); + +/* Parse the 'mask' field. */ + e->mask = p; + p =scanarg (p, del); +if (!p) +return NULL ; +if (!e->mask[0 ]) { + e->mask =NULL ; +pr_debug ("register: mask[raw]: none\n" ); + }else { +print_hex_dump_debug ( +KBUILD_MODNAME ": register: mask[raw]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->mask, p - e->mask, true); + } + +/* +* Decode the magic & mask fields. Note: while we might have accepted +* embedded NUL bytes from above, the unescape helpers will stop at +* the first one they encounter. +*/ + e->size =string_unescape_inplace (e->magic,UNESCAPE_HEX ); +if (e->mask &&string_unescape_inplace (e->mask,UNESCAPE_HEX ) != e->size) +return NULL ; +if (e->size >BINPRM_BUF_SIZE ||BINPRM_BUF_SIZE - e->size < e->offset) +return NULL ; +pr_debug ("register: magic/mask length: %i\n" , e->size); +print_hex_dump_debug ( +KBUILD_MODNAME ": register: magic[decoded]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->magic, e->size, true); +if (e->mask) +print_hex_dump_debug ( +KBUILD_MODNAME ": register: mask[decoded]: " , +DUMP_PREFIX_NONE ,16 ,1 , e->mask, e->size, true); +return p; +} + +/* Parse the 'magic' field of an 'E' entry: the filename extension. */ +static char *parse_extension_fields (struct binfmt_misc_entry *e,char *p, +char del) +{ +/* Skip the 'offset' field. */ + p =strchr (p, del); +if (!p) +return NULL ; + *p++ ='\0' ; + +/* Parse the 'magic' field. */ + e->magic = p; + p =strchr (p, del); +if (!p) +return NULL ; + *p++ ='\0' ; +if (!e->magic[0 ] ||strchr (e->magic,'/' )) +return NULL ; +pr_debug ("register: extension: {%s}\n" , e->magic); + +/* Skip the 'mask' field. */ + p =strchr (p, del); +if (!p) +return NULL ; + *p++ ='\0' ; +return p; +} +/* * This registers a new binary format, it recognises the syntax * ':name:type:offset:magic:mask:interpreter:flags'
@@ -348,29 +437,26 @@ static char *check_special_flags(char *p, struct binfmt_misc_entry *e)static struct binfmt_misc_entry *create_entry (const char __user *buffer,size_t count) { -struct binfmt_misc_entry *e; + struct binfmt_misc_entry *e __free(kfree) = NULL;char *buf, *p;char del; -int err;pr_debug ("register: received %zu bytes\n" , count);/* some sanity checks */ - err = -EINVAL ;if ((count <11 ) || (count >MAX_REGISTER_LENGTH )) - goto out; +return ERR_PTR (-EINVAL ); - err = -ENOMEM ; e =kmalloc (struct_size (e, buf, count +MISC_DELIM_PAD ),GFP_KERNEL_ACCOUNT );if (!e) - goto out; +return ERR_PTR (-ENOMEM ); p = buf = e->buf;memset (e,0 ,sizeof (*e));if (copy_from_user (buf, buffer, count)) - goto efault; +return ERR_PTR (-EFAULT ); del = *p++;/* delimeter */
@@ -383,13 +469,13 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, e->name = p; p =strchr (p, del);if (!p) - goto einval; +return ERR_PTR (-EINVAL ); *p++ ='\0' ;if (!e->name[0 ] || !strcmp (e->name,"." ) || !strcmp (e->name,".." ) ||strchr (e->name,'/' )) - goto einval; +return ERR_PTR (-EINVAL );pr_debug ("register: name: {%s}\n" , e->name);
@@ -404,111 +490,26 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer, e->flags =BIT (MISC_FMT_ENABLED_BIT ) |BIT (MISC_FMT_MAGIC_BIT );break ;default : - goto einval; +return ERR_PTR (-EINVAL ); }if (*p++ != del) - goto einval; - -if (test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) { -/* Handle the 'M' (magic) format. */ -char *s; - -/* Parse the 'offset' field. */ - s =strchr (p, del); -if (!s) - goto einval; - *s ='\0' ; -if (p != s) { -int r =kstrtoint (p,10 , &e->offset); -if (r !=0 || e->offset <0 ) - goto einval; - } - p = s; -if (*p++) - goto einval; -pr_debug ("register: offset: %#x\n" , e->offset); - -/* Parse the 'magic' field. */ - e->magic = p; - p =scanarg (p, del); -if (!p) - goto einval; -if (!e->magic[0 ]) - goto einval; -print_hex_dump_debug ( -KBUILD_MODNAME ": register: magic[raw]: " , -DUMP_PREFIX_NONE ,16 ,1 , e->magic, p - e->magic, true); - -/* Parse the 'mask' field. */ - e->mask = p; - p =scanarg (p, del); -if (!p) - goto einval; -if (!e->mask[0 ]) { - e->mask =NULL ; -pr_debug ("register: mask[raw]: none\n" ); - }else { -print_hex_dump_debug ( -KBUILD_MODNAME ": register: mask[raw]: " , -DUMP_PREFIX_NONE ,16 ,1 , e->mask, p - e->mask, - true); - } +return ERR_PTR (-EINVAL ); -/* -* Decode the magic & mask fields. -* Note: while we might have accepted embedded NUL bytes from -* above, the unescape helpers here will stop at the first one -* it encounters. -*/ - e->size =string_unescape_inplace (e->magic,UNESCAPE_HEX ); -if (e->mask && -string_unescape_inplace (e->mask,UNESCAPE_HEX ) != e->size) - goto einval; -if (e->size >BINPRM_BUF_SIZE || -BINPRM_BUF_SIZE - e->size < e->offset) - goto einval; -pr_debug ("register: magic/mask length: %i\n" , e->size); -print_hex_dump_debug ( -KBUILD_MODNAME ": register: magic[decoded]: " , -DUMP_PREFIX_NONE ,16 ,1 , e->magic, e->size, true); -if (e->mask) -print_hex_dump_debug ( -KBUILD_MODNAME ": register: mask[decoded]: " , -DUMP_PREFIX_NONE ,16 ,1 , e->mask, e->size, true); - }else { -/* Handle the 'E' (extension) format. */ - -/* Skip the 'offset' field. */ - p =strchr (p, del); -if (!p) - goto einval; - *p++ ='\0' ; - -/* Parse the 'magic' field. */ - e->magic = p; - p =strchr (p, del); -if (!p) - goto einval; - *p++ ='\0' ; -if (!e->magic[0 ] ||strchr (e->magic,'/' )) - goto einval; -pr_debug ("register: extension: {%s}\n" , e->magic); - -/* Skip the 'mask' field. */ - p =strchr (p, del); -if (!p) - goto einval; - *p++ ='\0' ; - } +if (test_bit (MISC_FMT_MAGIC_BIT , &e->flags)) + p =parse_magic_fields (e, p, del); +else + p =parse_extension_fields (e, p, del); +if (!p) +return ERR_PTR (-EINVAL );/* Parse the 'interpreter' field. */ e->interpreter = p; p =strchr (p, del);if (!p) - goto einval; +return ERR_PTR (-EINVAL ); *p++ ='\0' ;if (!e->interpreter[0 ]) - goto einval; +return ERR_PTR (-EINVAL );pr_debug ("register: interpreter: {%s}\n" , e->interpreter);/* Parse the 'flags' field. */
@@ -516,19 +517,9 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,if (*p =='\n' ) p++;if (p != buf + count) - goto einval; - -return e; - -out: -return ERR_PTR (err); +return ERR_PTR (-EINVAL ); -efault: -kfree (e); -return ERR_PTR (-EFAULT ); -einval: -kfree (e); -return ERR_PTR (-EINVAL ); +return no_free_ptr (e); }/* Commands accepted by the /status and /<entry> files. */
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 20/23 binfmt_misc: use __free(kfree) in bm_register_write()
In reply to Christian Braunerbm_register_write() has to free the entry it got from create_entry() on every failure until add_entry() has linked it into the filesystem and made the inode its owner. Arm the entry with __free(kfree) so the error branches can simply return and disarm it via retain_and_null_ptr() once ownership has been handed to the inode. The interpreter file keeps its manual error cleanup as freeing the entry would not close it.
No functional change.
--- fs/binfmt_misc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
fs/binfmt_misc.c +5 -5
@@ -796,13 +796,12 @@ static int add_entry(struct binfmt_misc_entry *e, struct super_block *sb)static ssize_t bm_register_write (struct file *file,const char __user *buffer,size_t count,loff_t *ppos) { -struct binfmt_misc_entry *e; + struct binfmt_misc_entry *e __free(kfree) = NULL;struct super_block *sb =file_inode (file)->i_sb; -int err =0 ;struct file *f =NULL ; +int err; e =create_entry (buffer, count); -if (IS_ERR (e))return PTR_ERR (e);
@@ -819,7 +818,6 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,if (IS_ERR (f)) {pr_notice ("register: failed to install interpreter file %s\n" , e->interpreter); -kfree (e);return PTR_ERR (f); } e->interp_file = f;
@@ -831,9 +829,11 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,exe_file_allow_write_access (f);filp_close (f,NULL ); } -kfree (e);return err; } + +/* The entry is owned by its inode now. */ +retain_and_null_ptr (e);return count; }
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 21/23 binfmt_misc: assorted small cleanups
In reply to Christian BraunerUse umode_t for the mode argument of bm_get_inode(), constify the fixed status strings in bm_status_read(), give the super_operations the bm_ prefix everything else in this file uses, replace the stale scanarg() comment which still described parameters and an err variable it lost decades ago and fix the delimiter typo plus a missing space nearby.
No functional change.
--- fs/binfmt_misc.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
fs/binfmt_misc.c +9 -10
@@ -291,10 +291,9 @@ static int load_misc_binary(struct linux_binprm *bprm)/* Command parsers */ /* -* parses and copies one argument enclosed in del from *sp to *dp, -* recognising the \x special. -* returns pointer to the copied argument or NULL in case of an -* error (and sets err) or null argument length. +* Scan the argument starting at @s up to the delimiter @del, recognising +* the \x escape. Terminates the argument with a NUL and returns a pointer +* past it or NULL on a malformed escape. */ static char *scanarg (char *s,char del) {
@@ -309,7 +308,7 @@ static char *scanarg(char *s, char del)return NULL ; } } - s[-1 ] ='\0' ; + s[-1 ] ='\0' ;return s; }
@@ -458,7 +457,7 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,if (copy_from_user (buf, buffer, count))return ERR_PTR (-EFAULT ); - del = *p++;/* delimeter */ + del = *p++;/* delimiter */ pr_debug ("register: delim: %#x {%c}\n" , del, del);
@@ -600,7 +599,7 @@ static int bm_entry_show(struct seq_file *m, void *unused)return 0 ; } -static struct inode *bm_get_inode (struct super_block *sb,int mode) +static struct inode *bm_get_inode (struct super_block *sb,umode_t mode) {struct inode *inode =new_inode (sb);
@@ -848,7 +847,7 @@ static ssize_t bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos) { struct binfmt_misc *misc; -char *s; +const char *s; misc =i_binfmt_misc (file_inode (file)); s =READ_ONCE (misc->enabled) ?"enabled\n" :"disabled\n" ;
@@ -895,7 +894,7 @@ static void bm_put_super(struct super_block *sb)put_user_ns (user_ns); } -static const struct super_operations s_ops = { +static const struct super_operations bm_super_ops = { .statfs = simple_statfs, .evict_inode = bm_evict_inode, .put_super = bm_put_super,
@@ -964,7 +963,7 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc) err =simple_fill_super (sb,BINFMTFS_MAGIC , bm_files);if (!err) - sb->s_op = &s_ops; + sb->s_op = &bm_super_ops;return err; }
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 22/23 binfmt_misc: include what is used
In reply to Christian BraunerThe include list still reflects code that left this file years ago:
nothing here uses sched/mm.h, pagemap.h, namei.h, syscalls.h or anything from fs/internal.h anymore, mount.h and the bm_fs_type forward declaration lost their last user when the pinned bm_mnt machinery was removed. Drop all of that and instead spell out the headers the file actually relies on but so far pulled in
transitively: bitops, bits, bug, cleanup, cred, kstrtox, printk, refcount, string and user_namespace. With that nothing needs the kernel.h grab bag anymore, so it goes too, and the list is sorted alphabetically.
--- fs/binfmt_misc.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
fs/binfmt_misc.c +17 -17
@@ -10,27 +10,29 @@#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt -#include <linux/kernel.h> -#include <linux/module.h> -#include <linux/init.h> -#include <linux/sched/mm.h> -#include <linux/magic.h> #include <linux/binfmts.h> -#include <linux/slab.h> +#include <linux/bitops.h> +#include <linux/bits.h> +#include <linux/bug.h> +#include <linux/cleanup.h> +#include <linux/cred.h> #include <linux/ctype.h> -#include <linux/string_helpers.h> #include <linux/file.h> -#include <linux/pagemap.h> -#include <linux/namei.h> -#include <linux/mount.h> +#include <linux/fs.h> +#include <linux/fs_context.h> +#include <linux/init.h> +#include <linux/kstrtox.h> +#include <linux/magic.h> +#include <linux/module.h> +#include <linux/printk.h> #include <linux/rculist.h> +#include <linux/refcount.h> #include <linux/seq_file.h> -#include <linux/fs_context.h> -#include <linux/syscalls.h> -#include <linux/fs.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/string_helpers.h> #include <linux/uaccess.h> - -#include "internal.h" +#include <linux/user_namespace.h> /* Entry status and match type bit numbers. */ enum binfmt_misc_entry_bits {
@@ -62,8 +64,6 @@ struct binfmt_misc_entry {char buf[];/* register string, fields point in here */ }; -static struct file_system_type bm_fs_type; -/* * Max length of the register string. Determined by: * - 7 delimiters
Signature · 1 line
2.53.0
Christian Brauner
maintainerPATCH v2 23/23 binfmt_misc: allow removing entries via unlink(2)
In reply to Christian BraunerRemoving a binary type handler requires echoing -1 into its entry file which works but is an odd interface to discover for something that already looks like a plain file in a filesystem. The comment on remove_binfmt_handler() has been suggesting a proper ->unlink() method for years, so add one: unlinking an entry file unhashes the entry from the handler list and removes the file, exactly like writing -1 to it does. The status and register control files refuse removal with EPERM the same way binderfs protects binder-control. Writing -1 keeps working.
Permission-wise nothing new is exposed: unlink(2) requires write access to the root directory which is owned by the (user namespace) root with mode 0755, matching the privilege needed to write to the 0644 entry files. The VFS calls ->unlink() with the root inode lock held so the existing writer serialization scheme applies unchanged, and eviction of the unlinked inode drops the entry reference exactly as for the write based removal.
Document the new way in admin-guide/binfmt-misc.rst.
--- Documentation/admin-guide/binfmt-misc.rst | 3 +- fs/binfmt_misc.c | 77 ++++++++++++++++++++++--------- 2 files changed, 58 insertions(+), 22 deletions(-)
Documentation/admin-guide/binfmt-misc.rst +2 -1
@@ -133,7 +133,8 @@ or 1 (to enable) to ``/proc/sys/fs/binfmt_misc/status`` or Catting the file tells you the current status of ``binfmt_misc/the_entry``. You can remove one entry or all entries by echoing -1 to ``/proc/.../the_name`` -or ``/proc/sys/fs/binfmt_misc/status``. +or ``/proc/sys/fs/binfmt_misc/status``. A single entry can also be removed +by simply unlinking (``rm``) ``/proc/.../the_name``. Hints
fs/binfmt_misc.c +56 -21
114 lines not on this page · raw message
Signature · 1 line
2.53.0
Christian Brauner
maintainerQuoted 75 lines
The first two patches fix two
i_writecountimbalances onMISC_FMT_OPEN_FILEinterpreter files that turned up while auditing the file for the rework below and are marked for stable: removing an entry never restored the write access denied byopen_exec()at registration, leaving the interpreter unwritable until its inode gets evicted, and the write denial taken on the interpreter clone during exec is not paired with theFMODE_FSNOTIFY_HSMaware release the exec machinery uses, so pre-content watches make execs leak write denials.The rest reworks the locking and tidies the file up.
Once
binfmt_miscis loadedload_misc_binary()runs for everyexecve()on the system sincebinfmt_miscregisters at the head of the formats list. Every exec therefore performsread_lock()andread_unlock()on theentries_lockof the relevantbinfmt_miscinstance, 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 ownbinfmt_miscmount fall back to an ancestor's instance so on container-heavy systems every exec on the machine typically ends up hammering the cacheline ofinit_binfmt_misc. OnPREEMPT_RTthe 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 viakfree_rcu(). The removal paths need to detect whether an entry has already been unlinked and rely onlist_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 withhlist_unhashed()serving as the linked test. Hence the third patch converts the entry list to an hlist so the RCU conversion in the fourth 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_miscentry no longer writes to any shared cacheline at all. The fifth patch annotates the long-standing lockless ->enabled accesses for KCSAN and the three patches after it make the entry flags proper enums and give structbinfmt_misc_entrya name that isn't Node.The remaining patches are a cleanup pass over the whole file: remove the
VERBOSE_STATUSandUSE_DEBUGcompile-time toggles, convert the entry file toseq_file, factor out entry matching, entry removal and the register string field parsing, make the entry/register string allocation a flexible array member, give theparse_command()results names, letcleanup.hunwind the entry registration and exec error paths and prune the include list down to what is used. Aside fromseq_lseek()now bounding seeks on entry files and the ETXTBSY propagation in the second patch the cleanups have no user-visible effect.The last patch adds what the comment in
remove_binfmt_handler()had been suggesting for years: entries can now be removed via unlink(2) in addition to the -1 write. The status and register control files refuse removal.
Quoted 15 lines
---
Changes in v2:
- Allow removing entries via unlink(2).
- Add two stable-marked fixes restoringi_writecountbalance for
MISC_FMT_OPEN_FILEinterpreter files, first so they apply to
mainline directly.
- Turn the entry bit numbers and behavior flags into proper enums and
rename Node to structbinfmt_misc_entry.
- Add a cleanup pass over the whole file: remove theVERBOSE_STATUS
andUSE_DEBUGtoggles, convert the entry file toseq_file, factor
out entry matching, entry removal and register string parsing, use
a flexible array member for the register string, name the
parse_command()results, usecleanup.hfor the entry error
unwinding in registration and exec and prune the include list.
- Link to v1: https://patch.msgid.link/[email protected]
I'll send a v3 in a bit with one more fix.
Signature · 1 line
Christian Brauner <[email protected]>