Re: [PATCH] debugfs: serialize debugfs_create_str() writers

On Mon, Aug 03, 2026 at 04:34:01PM +0800, Yichong Chen wrote:
> On Mon, Aug 03, 2026 at 08:19:05AM +0200, Greg KH wrote:
> > Ouch, you are doing to serialize _all_ debugfs strings on one lock?
> >
> > This feels wrong :(
> >
> > Wait, why rcu if you have a lock?
> >
> > guard() is nicer.
> >
> > My larger question is, what code is broken because of this?  What
> > debugfs string replacements are happening?  I hate the string debugfs
> > code as it has had lots of issues like this over the years so maybe we
> > should just drop it and force users to "roll their own" implementation
> > that would be much simpler without the rcu/locking mess at all?
> 
> Thanks for the review.
> 
> The patch was trying to fix a double-free.  Two concurrent writes to the same
> debugfs_create_str() file can both observe the same old pointer before
> either replacement is published, and then both free that old string.  I
> reproduced this with KASAN and got:
> 
>   BUG: KASAN: double-free in debugfs_write_file_str()
> 
> I agree with your comments that the current fix is not a good direction.  A
> global mutex serializes unrelated debugfs string files, and mixing that with
> the RCU read side makes the helper more complicated.
> 
> The directly writable in-tree users I found are:
> 
>   drivers/interconnect/debugfs-client.c:
>     /sys/kernel/debug/interconnect/test_client/src_node, mode 0600
>     /sys/kernel/debug/interconnect/test_client/dst_node, mode 0600
> 
>   drivers/soundwire/debugfs.c:
>     firmware_file, mode 0200
> 
> So the write path is reachable through in-tree debugfs users, although I
> do not know whether anyone relies on concurrent writes to these files in
> practice.
> 
> I can rework this in a few ways:
> 
>   1. make the locking per-file instead of global, if we want to keep the
>      generic writable string helper;
> 
>   2. drop the write support from debugfs_create_str() and convert the
>      writable users to their own small file operations;

If we only have 3 writable string users, I would suggest we do this, and
just leave the debugfs string functionality for read-only files as that
is the MUCH simpler case.

If we have more, well, let's see how many more, and just what exactly a
conversion to "small file operation" would entail.  Try it for the
soundwire file above and let's see what the diff looks like.

>   3. take another direction if you have a preferred approach.
> 
> Dropping write support would avoid keeping this locking in the generic
> helper, but debugfs_create_str() is exported, so that would also change
> behavior for any out-of-tree users relying on writable strings.

For obvious reasons, we do not care about out-of-tree users for any
in-kernel api, as that way would be insanity. :)

thanks,

greg k-h