lock in vsprintf(): was: Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Petr Mladek <pmladek@suse.com>]

Adding Linus and some other guys into Cc.

My concern is taking a lock when processing a printf format, see
below for more details.

On Tue 2024-12-17 12:37:09, Rob Herring (Arm) wrote:
> The callers for of_modalias() generally need the module alias as part of
> some larger string. That results in some error prone manipulation of the
> buffer prepend/append the module alias string. In fact,
> of_device_uevent_modalias() has several issues. First, it's off by one
> too few characters in utilization of the full buffer. Second, the error
> paths leave OF_MODALIAS with a truncated value when in the end nothing
> should be added to the buffer. It is also fragile because it needs
> internal details of struct kobj_uevent_env. add_uevent_var() really
> wants to write the env variable and value in one shot which would need
> either a temporary buffer for value or a format specifier.
> 
> Fix these issues by adding a new printf format specifier, "%pOFm". With
> the format specifier in place, simplify all the callers of
> of_modalias(). of_modalias() can also be simplified with vsprintf()
> being the only caller as it avoids the error conditions.
> 
> Cc: Zijun Hu <quic_zijuhu@quicinc.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/core-api/printk-formats.rst |  1 +
>  drivers/of/device.c                       | 25 ++--------------
>  drivers/of/module.c                       | 35 +++++------------------
>  drivers/of/unittest.c                     |  2 ++
>  include/linux/of.h                        |  8 +++---
>  lib/vsprintf.c                            |  7 +++--
>  6 files changed, 22 insertions(+), 56 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index ecccc0473da9..d72fe3d8c427 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -496,6 +496,7 @@ equivalent to %pOFf.
>  	- F - device node flags
>  	- c - major compatible string
>  	- C - full compatible string
> +	- m - module alias string
>  
>  The separator when using multiple arguments is ':'
>  
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index edf3be197265..ae8c47d5db8e 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -256,24 +251,10 @@ EXPORT_SYMBOL_GPL(of_device_uevent);
>  
>  int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env)
>  {
> -	int sl;
> -
>  	if ((!dev) || (!dev->of_node) || dev->of_node_reused)
>  		return -ENODEV;
>  
> -	/* Devicetree modalias is tricky, we add it in 2 steps */
> -	if (add_uevent_var(env, "MODALIAS="))
> -		return -ENOMEM;
> -
> -	sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
> -			 sizeof(env->buf) - env->buflen);
> -	if (sl < 0)
> -		return sl;
> -	if (sl >= (sizeof(env->buf) - env->buflen))
> -		return -ENOMEM;
> -	env->buflen += sl;
> -
> -	return 0;
> +	return add_uevent_var(env, "MODALIAS=%pOFm", dev->of_node);

The proposed %pOFm format takes a lock inside. It calls:

size_t of_modalias(const struct device_node *np, char *str, size_t len)
{
[...]
	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
			 of_node_get_device_type(np));
[...]

which calls:

  + of_node_get_device_type()
    + of_get_property()
      + of_find_property()

, where

struct property *of_find_property(const struct device_node *np,
				  const char *name,
				  int *lenp)
{
[...]
	raw_spin_lock_irqsave(&devtree_lock, flags);
	pp = __of_find_property(np, name, lenp);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
[...]	return pp;

I personally think that taking locks when formatting string is
a way to hell.

In this case, add_uevent_var() is lockless so that it should not
cause any problem. But just imagine that it does:

int add_uevent_var(struct kobj_uevent_env *env, const char *format, ...)
{

	some_lock();

	va_start(args, format);
	len = vsnprintf(&env->buf[env->buflen],
			sizeof(env->buf) - env->buflen,
			format, args);
	va_end(args);

	some_unlock();

	return 0;
}

Would anyone consider that the vsprintf() here might need to take a lock?

Also, the format might be used in printk(). We put a huge effort into
creating a lockless ringbuffer and safe console locking. I would
really appreciate to avoid any locking in the formatting part.


That said, we already have a precedent. "%pOFf" might take a lock,
for example, via:

  + fwnode_full_name_string()
    + fwnode_handle_put()
      + of_fwnode_put()
	+ of_node_put()
	  + kobject_put()
	    + kref_put()
	      + schedule_delayed_work()
		+ queue_delayed_work()
		  + queue_delayed_work_on()
		    + __queue_delayed_work()
		      + add_timer_on()
			+ add_timer_on()
			  + lock_timer_base()
			   + raw_spin_lock_irqsave(&base->lock, *flags);

But this would happen only when the last reference is released
when formatting the string which is kind of corner case.

As I said, I think that taking lock in vsprintf() formats is highly
unexpected and thus a way to hell.

What do others think, please?

Best Regards,
Petr



>  }
>  EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>  
> diff --git a/drivers/of/module.c b/drivers/of/module.c
> index 1e735fc130ad..80879d2abea8 100644
> --- a/drivers/of/module.c
> +++ b/drivers/of/module.c
> @@ -8,21 +8,14 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  
> -ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
> +/* Do not use directly, use %pOFm format specifier instead */
> +size_t of_modalias(const struct device_node *np, char *str, size_t len)
>  {
>  	const char *compat;
>  	char *c;
>  	struct property *p;
> -	ssize_t csize;
> -	ssize_t tsize;
> -
> -	/*
> -	 * Prevent a kernel oops in vsnprintf() -- it only allows passing a
> -	 * NULL ptr when the length is also 0. Also filter out the negative
> -	 * lengths...
> -	 */
> -	if ((len > 0 && !str) || len < 0)
> -		return -EINVAL;
> +	size_t csize;
> +	size_t tsize;
>  
>  	/* Name & Type */
>  	/* %p eats all alphanum characters, so %c must be used here */
> @@ -53,29 +46,15 @@ ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
>  
>  int of_request_module(const struct device_node *np)
>  {
> -	char *str;
> -	ssize_t size;
> -	int ret;
> +	char *str __free(kfree);
>  
>  	if (!np)
>  		return -ENODEV;
>  
> -	size = of_modalias(np, NULL, 0);
> -	if (size < 0)
> -		return size;
> -
> -	/* Reserve an additional byte for the trailing '\0' */
> -	size++;
> -
> -	str = kmalloc(size, GFP_KERNEL);
> +	str = kasprintf(GFP_KERNEL, "%pOFm", np);
>  	if (!str)
>  		return -ENOMEM;
>  
> -	of_modalias(np, str, size);
> -	str[size - 1] = '\0';
> -	ret = request_module(str);
> -	kfree(str);
> -
> -	return ret;
> +	return request_module(str);
>  }
>  EXPORT_SYMBOL_GPL(of_request_module);
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index daf9a2dddd7e..93921399f02d 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -342,6 +342,8 @@ static void __init of_unittest_printf(void)
>  	of_unittest_printf_one(np, "%pOFc", "test-sub-device");
>  	of_unittest_printf_one(np, "%pOFC",
>  			"\"test-sub-device\",\"test-compat2\",\"test-compat3\"");
> +	of_unittest_printf_one(np, "%pOFm",
> +			"of:NdevT(null)Ctest-sub-deviceCtest-compat2Ctest-compat3");
>  }
>  
>  struct node_hash {
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f921786cb8ac..9fe7d17ce7e2 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -382,7 +382,7 @@ extern int of_count_phandle_with_args(const struct device_node *np,
>  	const char *list_name, const char *cells_name);
>  
>  /* module functions */
> -extern ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len);
> +extern size_t of_modalias(const struct device_node *np, char *str, size_t len);
>  extern int of_request_module(const struct device_node *np);
>  
>  /* phandle iterator functions */
> @@ -762,10 +762,10 @@ static inline int of_count_phandle_with_args(const struct device_node *np,
>  	return -ENOSYS;
>  }
>  
> -static inline ssize_t of_modalias(const struct device_node *np, char *str,
> -				  ssize_t len)
> +static inline size_t of_modalias(const struct device_node *np, char *str,
> +				 size_t len)
>  {
> -	return -ENODEV;
> +	return 0;
>  }
>  
>  static inline int of_request_module(const struct device_node *np)
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 9d3dac38a3f4..6a4f99b39de0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2169,10 +2169,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  
>  	/* simple case without anything any more format specifiers */
>  	fmt++;
> -	if (fmt[0] == '\0' || strcspn(fmt,"fnpPFcC") > 0)
> +	if (fmt[0] == '\0' || strcspn(fmt,"fnpPFcCm") > 0)
>  		fmt = "f";
>  
> -	for (pass = false; strspn(fmt,"fnpPFcC"); fmt++, pass = true) {
> +	for (pass = false; strspn(fmt,"fnpPFcCm"); fmt++, pass = true) {
>  		int precision;
>  		if (pass) {
>  			if (buf < end)
> @@ -2226,6 +2226,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  				has_mult = true;
>  			}
>  			break;
> +		case 'm':
> +			buf += of_modalias(dn, buf, end - buf);
> +			break;
>  		default:
>  			break;
>  		}
> -- 
> 2.45.2