Re: [PATCH v2 1/3] interconnect: debugfs: replace writable string helper

[Greg KH]

On Thu, Aug 06, 2026 at 04:48:52PM +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 interconnect debugfs client src_node and dst_node entries to
> local file operations before removing writable string support from
> debugfs_create_str().  Protect the string replacement and path lookup with
> the existing debugfs_lock.
> 
> Signed-off-by: Yichong Chen <chenyichong@xxxxxxxxxxxxx>
> ---
>  drivers/interconnect/debugfs-client.c | 75 ++++++++++++++++++++++-----
>  1 file changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
> index 08df9188ef94..b71c25fd9314 100644
> --- a/drivers/interconnect/debugfs-client.c
> +++ b/drivers/interconnect/debugfs-client.c
> @@ -5,6 +5,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/interconnect.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  
>  #include "internal.h"
>  
> @@ -36,6 +37,59 @@ struct debugfs_path {
>  	struct list_head list;
>  };
>  
> +static ssize_t icc_node_read(struct file *file, char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	char **node = file->private_data;
> +	char *copy;
> +	size_t len;
> +	ssize_t ret;
> +
> +	mutex_lock(&debugfs_lock);
> +	copy = kstrdup(*node ?: "", GFP_KERNEL);
> +	mutex_unlock(&debugfs_lock);

scoped guard?

> +	if (!copy)
> +		return -ENOMEM;
> +
> +	len = strlen(copy);
> +	copy[len++] = '\n';
> +	ret = simple_read_from_buffer(user_buf, count, ppos, copy, len);
> +	kfree(copy);
> +	return ret;
> +}
> +
> +static ssize_t icc_node_write(struct file *file, const char __user *user_buf,
> +			      size_t count, loff_t *ppos)
> +{
> +	char **node = file->private_data;
> +	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(&debugfs_lock);
> +	old = *node;
> +	*node = new;
> +	mutex_unlock(&debugfs_lock);

Same here.

> +
> +	kfree(old);
> +	return count;
> +}
> +
> +static const struct file_operations icc_node_fops = {
> +	.open = simple_open,
> +	.read = icc_node_read,
> +	.write = icc_node_write,
> +	.llseek = default_llseek,
> +};
> +
>  static struct icc_path *get_path(const char *src, const char *dst)
>  {
>  	struct debugfs_path *path;
> @@ -56,24 +110,17 @@ static int icc_get_set(void *data, u64 val)
>  
>  	mutex_lock(&debugfs_lock);
>  
> -	rcu_read_lock();
> -	src = rcu_dereference(src_node);
> -	dst = rcu_dereference(dst_node);
> -
>  	/*
>  	 * If we've already looked up a path, then use the existing one instead
>  	 * of calling icc_get() again. This allows for updating previous BW
>  	 * votes when "get" is written to multiple times for multiple paths.
>  	 */
> -	cur_path = get_path(src, dst);
> -	if (cur_path) {
> -		rcu_read_unlock();
> +	cur_path = get_path(src_node, dst_node);
> +	if (cur_path)
>  		goto out;
> -	}
>  
> -	src = kstrdup(src, GFP_ATOMIC);
> -	dst = kstrdup(dst, GFP_ATOMIC);
> -	rcu_read_unlock();
> +	src = kstrdup(src_node, GFP_KERNEL);
> +	dst = kstrdup(dst_node, GFP_KERNEL);

Why is GFP_KERNEL now ok, while GFP_ATOMIC wasn't?  Is this the rcu
stuff interacting somehow?

thanks,

greg k-h