[PATCH v3 00/24] binfmt_misc: write access fixes, RCU handler lookup and cleanups

[PATCH v3 00/24] binfmt_misc: write access fixes, RCU handler lookup and cleanups

[Christian Brauner]

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.

Also, a register string whose delimiter is one of the flag characters
('P', 'O', 'C', 'F') makes the flag scan in create_entry() run past the
end of the register buffer. Reject such a delimiter up front.

The rest reworks the locking and tidies the file up.

The current rwlock 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. 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 penultimate 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.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Changes in v3:
- Reject a register string delimiter that is also a flag character.
  A flag-char delimiter made the create_entry() flag scan read past
  the end of the register buffer. Stable-marked, added as the last
  patch.
- Link to v2: https://patch.msgid.link/20260710-work-binfmt_misc-locking-v2-0-2a1c3d4126a7@kernel.org

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/20260708-work-binfmt_misc-locking-v1-0-a009dd5b56db@kernel.org

---
Christian Brauner (24):
      binfmt_misc: restore write access when removing an entry
      binfmt_misc: use exe_file_deny_write_access() for the interpreter clone
      binfmt_misc: reject a flag character as the field delimiter
      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                          | 839 +++++++++++++++---------------
 include/linux/binfmts.h                   |   4 +-
 kernel/user.c                             |   4 +-
 4 files changed, 433 insertions(+), 417 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260708-work-binfmt_misc-locking-15347df12ec8

[PATCH v3 01/24] binfmt_misc: restore write access when removing an entry

[Christian Brauner]

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.

Fixes: 948b701a607f ("binfmt_misc: add persistent opened binary handler for containers")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_misc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 84349fcb93f1..de50a7468b07 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -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);
 	}
 }

-- 
2.53.0

[PATCH v3 02/24] binfmt_misc: use exe_file_deny_write_access() for the interpreter clone

[Christian Brauner]

For 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.

Fixes: 0357ef03c94e ("fs: don't block write during exec on pre-content watched files")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_misc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index de50a7468b07..24142859658c 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -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);
 	}

-- 
2.53.0

[PATCH v3 03/24] binfmt_misc: reject a flag character as the field delimiter

[Christian Brauner]

The registration string starts with a user chosen delimiter that
separates the individual fields. So that the field parsers terminate
even on a truncated string create_entry() pads the buffer with that
same delimiter:

	memset(buf + count, del, 8);

Most fields are scanned for the delimiter with strchr()/scanarg() and
happily stop on the padding. The flags field is different: instead of
scanning for the delimiter check_special_flags() consumes the flag
characters 'P', 'O', 'C' and 'F' and stops at the first byte that is
none of them, relying on the trailing delimiter to end the scan.

If the delimiter is itself a flag character the padding no longer acts
as a terminator. The scan swallows all eight padding bytes and keeps
reading past the end of the allocation until it hits a byte that is
not a flag character. For example registering

	PaPEPPxPPiP

with 'P' as the delimiter (name "a", type extension, magic "x",
interpreter "i", empty flags) leaves the flag scan running off the end
of the buffer. The registration is rejected in the end because the
parser does not stop exactly at buf + count, but only after the out of
bounds read has already happened. With an unlucky allocation layout the
scan can walk into an unmapped page; under KASAN it is reported as a
slab out of bounds read. binfmt_misc mounts are available to
unprivileged users in a user namespace so the read is reachable without
privileges.

Reject a delimiter that is one of the flag characters up front. Such a
registration was always rejected anyway, only after the out of bounds
read, so no valid registration string changes meaning.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_misc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 24142859658c..b7664d90eb8f 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -385,6 +385,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
 
 	pr_debug("register: delim: %#x {%c}\n", del, del);
 
+	/* A flag-char delimiter runs the flag scan off the buffer. */
+	if (del == 'P' || del == 'O' || del == 'C' || del == 'F')
+		goto einval;
+
 	/* Pad the buffer with the delim to simplify parsing below. */
 	memset(buf + count, del, 8);
 

-- 
2.53.0

Re: [PATCH v3 03/24] binfmt_misc: reject a flag character as the field delimiter

[Jori Koolstra]

On Fri, Jul 10, 2026 at 11:33:04AM +0200, Christian Brauner wrote:
> The registration string starts with a user chosen delimiter that
> separates the individual fields. So that the field parsers terminate
> even on a truncated string create_entry() pads the buffer with that
> same delimiter:
> 
> 	memset(buf + count, del, 8);
> 
> Most fields are scanned for the delimiter with strchr()/scanarg() and
> happily stop on the padding. The flags field is different: instead of
> scanning for the delimiter check_special_flags() consumes the flag
> characters 'P', 'O', 'C' and 'F' and stops at the first byte that is
> none of them, relying on the trailing delimiter to end the scan.
> 
> If the delimiter is itself a flag character the padding no longer acts
> as a terminator. The scan swallows all eight padding bytes and keeps
> reading past the end of the allocation until it hits a byte that is
> not a flag character. For example registering
> 
> 	PaPEPPxPPiP
> 
> with 'P' as the delimiter (name "a", type extension, magic "x",
> interpreter "i", empty flags) leaves the flag scan running off the end
> of the buffer. The registration is rejected in the end because the
> parser does not stop exactly at buf + count, but only after the out of
> bounds read has already happened. With an unlucky allocation layout the
> scan can walk into an unmapped page; under KASAN it is reported as a
> slab out of bounds read. binfmt_misc mounts are available to
> unprivileged users in a user namespace so the read is reachable without
> privileges.
> 
> Reject a delimiter that is one of the flag characters up front. Such a
> registration was always rejected anyway, only after the out of bounds
> read, so no valid registration string changes meaning.


Looks good, I see no user visible changes.

Can't we support a less arcane and impenetrable format, like JSON
or YAML?

Reviewed-by: Jori Koolstra <jkoolstra@xs4all.nl>

> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  fs/binfmt_misc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 24142859658c..b7664d90eb8f 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -385,6 +385,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
>  
>  	pr_debug("register: delim: %#x {%c}\n", del, del);
>  
> +	/* A flag-char delimiter runs the flag scan off the buffer. */
> +	if (del == 'P' || del == 'O' || del == 'C' || del == 'F')
> +		goto einval;
> +
>  	/* Pad the buffer with the delim to simplify parsing below. */
>  	memset(buf + count, del, 8);
>  
> 
> -- 
> 2.53.0
> 

I am resending these reviews since I had an issue with my email client
and they were not properly attached to the correct thread.

Re: [PATCH v3 01/24] binfmt_misc: restore write access when removing an entry

[Jori Koolstra]

On Fri, Jul 10, 2026 at 11:33:02AM +0200, Christian Brauner wrote:
> 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.
> 
> Fixes: 948b701a607f ("binfmt_misc: add persistent opened binary handler for containers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  fs/binfmt_misc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 84349fcb93f1..de50a7468b07 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -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);
>  	}
>  }
> 
> -- 
> 2.53.0
> 

Reviewed-by: Jori Koolstra <jkoolstra@xs4all.nl>

Re: [PATCH v3 03/24] binfmt_misc: reject a flag character as the field delimiter

[Christian Brauner]

On 2026-07-25 17:37 +0200, Jori Koolstra wrote:
> On Fri, Jul 10, 2026 at 11:33:04AM +0200, Christian Brauner wrote:
> > The registration string starts with a user chosen delimiter that
> > separates the individual fields. So that the field parsers terminate
> > even on a truncated string create_entry() pads the buffer with that
> > same delimiter:
> > 
> > 	memset(buf + count, del, 8);
> > 
> > Most fields are scanned for the delimiter with strchr()/scanarg() and
> > happily stop on the padding. The flags field is different: instead of
> > scanning for the delimiter check_special_flags() consumes the flag
> > characters 'P', 'O', 'C' and 'F' and stops at the first byte that is
> > none of them, relying on the trailing delimiter to end the scan.
> > 
> > If the delimiter is itself a flag character the padding no longer acts
> > as a terminator. The scan swallows all eight padding bytes and keeps
> > reading past the end of the allocation until it hits a byte that is
> > not a flag character. For example registering
> > 
> > 	PaPEPPxPPiP
> > 
> > with 'P' as the delimiter (name "a", type extension, magic "x",
> > interpreter "i", empty flags) leaves the flag scan running off the end
> > of the buffer. The registration is rejected in the end because the
> > parser does not stop exactly at buf + count, but only after the out of
> > bounds read has already happened. With an unlucky allocation layout the
> > scan can walk into an unmapped page; under KASAN it is reported as a
> > slab out of bounds read. binfmt_misc mounts are available to
> > unprivileged users in a user namespace so the read is reachable without
> > privileges.
> > 
> > Reject a delimiter that is one of the flag characters up front. Such a
> > registration was always rejected anyway, only after the out of bounds
> > read, so no valid registration string changes meaning.
> 
> 
> Looks good, I see no user visible changes.
> 
> Can't we support a less arcane and impenetrable format, like JSON
> or YAML?

Adding a json parser into the kernel is something I had played with but
that has a bunch of very subtle pitfalls as the spec is underspecified
in various ways. Also, having the kernel produce json is one thing.
Having it _consume_ json is a whole other discussion. Ideally it would
be done in Rust.

Re: [PATCH v3 03/24] binfmt_misc: reject a flag character as the field delimiter

[Jori Koolstra]

> Op 27-07-2026 09:35 CEST schreef Christian Brauner <brauner@kernel.org>:
> 
>  
> On 2026-07-25 17:37 +0200, Jori Koolstra wrote:
> > On Fri, Jul 10, 2026 at 11:33:04AM +0200, Christian Brauner wrote:
> > > The registration string starts with a user chosen delimiter that
> > > separates the individual fields. So that the field parsers terminate
> > > even on a truncated string create_entry() pads the buffer with that
> > > same delimiter:
> > > 
> > > 	memset(buf + count, del, 8);
> > > 
> > > Most fields are scanned for the delimiter with strchr()/scanarg() and
> > > happily stop on the padding. The flags field is different: instead of
> > > scanning for the delimiter check_special_flags() consumes the flag
> > > characters 'P', 'O', 'C' and 'F' and stops at the first byte that is
> > > none of them, relying on the trailing delimiter to end the scan.
> > > 
> > > If the delimiter is itself a flag character the padding no longer acts
> > > as a terminator. The scan swallows all eight padding bytes and keeps
> > > reading past the end of the allocation until it hits a byte that is
> > > not a flag character. For example registering
> > > 
> > > 	PaPEPPxPPiP
> > > 
> > > with 'P' as the delimiter (name "a", type extension, magic "x",
> > > interpreter "i", empty flags) leaves the flag scan running off the end
> > > of the buffer. The registration is rejected in the end because the
> > > parser does not stop exactly at buf + count, but only after the out of
> > > bounds read has already happened. With an unlucky allocation layout the
> > > scan can walk into an unmapped page; under KASAN it is reported as a
> > > slab out of bounds read. binfmt_misc mounts are available to
> > > unprivileged users in a user namespace so the read is reachable without
> > > privileges.
> > > 
> > > Reject a delimiter that is one of the flag characters up front. Such a
> > > registration was always rejected anyway, only after the out of bounds
> > > read, so no valid registration string changes meaning.
> > 
> > 
> > Looks good, I see no user visible changes.
> > 
> > Can't we support a less arcane and impenetrable format, like JSON
> > or YAML?
> 
> Adding a json parser into the kernel is something I had played with but
> that has a bunch of very subtle pitfalls as the spec is underspecified
> in various ways. Also, having the kernel produce json is one thing.
> Having it _consume_ json is a whole other discussion. Ideally it would
> be done in Rust.

I might be interested in this, apart for the binfmt_misc stuff, this can
be also used in other places inside the kernel. And I would like to get
into Rust4Linux more. We could talk about it at Plumbers or MAINTAINER
SUMMIT (if I get an invite, but that's not too likely... :) )

Btw, could you CC me (if you think about it) for any of this binfmt_misc
stuff and the other such as failfs. I am very much interested and would
like to review more going forward. It's just been a bit of a slow process
as I am slowly picking up the pieces of VFS I am not familiar with yet.

Best,
Jori.