Re: [PATCH v2 2/3] soundwire: debugfs: replace writable string helper

On Thu, Aug 06, 2026 at 04:48:53PM +0800, Yichong Chen wrote:
> debugfs_create_str() is being made read-only because its generic write path
> is hard to make safe without adding more locking to the helper.
> 
> Convert the SoundWire firmware_file entry to a local write-only file
> operation before removing writable string support from
> debugfs_create_str().
> 
> Copy the firmware name before request_firmware() so later debugfs writes
> cannot replace the string while it is being used.
> 
> Signed-off-by: Yichong Chen <[email protected]>
> ---
>  drivers/soundwire/debugfs.c | 56 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 099eb84a548e..bba2b65a0668 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -145,6 +145,38 @@ static u32 start_addr;
>  static size_t num_bytes;
>  static u8 read_buffer[MAX_CMD_BYTES];
>  static char *firmware_file;
> +static DEFINE_MUTEX(firmware_file_lock);
> +
> +static ssize_t firmware_file_write(struct file *file,
> +				   const char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	char *old, *new;
> +
> +	if (*ppos)
> +		return -EINVAL;
> +	if (count + 1 > PAGE_SIZE)
> +		return -E2BIG;
> +
> +	new = memdup_user_nul(user_buf, count);
> +	if (IS_ERR(new))
> +		return PTR_ERR(new);
> +	strim(new);
> +
> +	mutex_lock(&firmware_file_lock);
> +	old = firmware_file;
> +	firmware_file = new;
> +	mutex_unlock(&firmware_file_lock);

scoped lock?

> +
> +	kfree(old);
> +	return count;
> +}
> +
> +static const struct file_operations firmware_file_fops = {
> +	.open = simple_open,
> +	.write = firmware_file_write,
> +	.llseek = default_llseek,
> +};
>  
>  static int set_command(void *data, u64 value)
>  {
> @@ -246,6 +278,7 @@ static int cmd_go(void *data, u64 value)
>  {
>  	const struct firmware *fw = NULL;
>  	struct sdw_slave *slave = data;
> +	char *fw_name = NULL;
>  	ktime_t start_t;
>  	ktime_t finish_t;
>  	int ret;
> @@ -265,15 +298,24 @@ static int cmd_go(void *data, u64 value)
>  	}
>  
>  	if (cmd == 0) {
> -		ret = request_firmware(&fw, firmware_file, &slave->dev);
> +		mutex_lock(&firmware_file_lock);

Again, scoped lock?

> +		if (firmware_file)
> +			fw_name = kstrdup(firmware_file, GFP_KERNEL);
> +		mutex_unlock(&firmware_file_lock);
> +		if (!fw_name) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		ret = request_firmware(&fw, fw_name, &slave->dev);
>  		if (ret < 0) {
> -			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
> +			dev_err(&slave->dev, "firmware %s not found\n", fw_name);
>  			goto out;
>  		}
>  		if (fw->size < num_bytes) {
>  			dev_err(&slave->dev,
>  				"firmware %s: firmware size %zd, desired %zd\n",
> -				firmware_file, fw->size, num_bytes);
> +				fw_name, fw->size, num_bytes);
>  			goto out;
>  		}
>  	}
> @@ -306,6 +348,7 @@ static int cmd_go(void *data, u64 value)
>  out:
>  	if (fw)
>  		release_firmware(fw);
> +	kfree(fw_name);
>  
>  	pm_runtime_mark_last_busy(&slave->dev);
>  	pm_runtime_put(&slave->dev);
> @@ -358,7 +401,8 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
>  
>  	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
>  	if (firmware_file)
> -		debugfs_create_str("firmware_file", 0200, d, &firmware_file);
> +		debugfs_create_file("firmware_file", 0200, d, NULL,
> +				    &firmware_file_fops);
>  
>  	slave->debugfs = d;
>  }
> @@ -370,8 +414,10 @@ void sdw_slave_debugfs_exit(struct sdw_slave *slave)
>  
>  void sdw_debugfs_init(void)
>  {
> +	mutex_lock(&firmware_file_lock);
>  	if (!firmware_file)
>  		firmware_file = kstrdup("", GFP_KERNEL);
> +	mutex_unlock(&firmware_file_lock);

Same here.

>  
>  	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
>  }
> @@ -379,6 +425,8 @@ void sdw_debugfs_init(void)
>  void sdw_debugfs_exit(void)
>  {
>  	debugfs_remove_recursive(sdw_debugfs_root);
> +	mutex_lock(&firmware_file_lock);

And here.

>  	kfree(firmware_file);
>  	firmware_file = NULL;

This doesn't need to get set to NULL, right?

thanks,

greg k-h