[PATCH] of: Add printf '%pOFm' for generating modalias

[PATCH] of: Add printf '%pOFm' for generating modalias

[Rob Herring (Arm)]

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
@@ -199,14 +199,9 @@ ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len)
 	if (!dev || !dev->of_node || dev->of_node_reused)
 		return -ENODEV;
 
-	sl = of_modalias(dev->of_node, str, len - 2);
-	if (sl < 0)
-		return sl;
-	if (sl > len - 2)
+	sl = snprintf(str, len, "%pOFm\n", dev->of_node);
+	if (sl >= len)
 		return -ENOMEM;
-
-	str[sl++] = '\n';
-	str[sl] = 0;
 	return sl;
 }
 EXPORT_SYMBOL_GPL(of_device_modalias);
@@ -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);
 }
 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

Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[quic_zijuhu]

On 12/18/2024 2:37 AM, 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.
> 

good solution (^^).

> 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

Ack.

>  
>  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
> @@ -199,14 +199,9 @@ ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len)
>  	if (!dev || !dev->of_node || dev->of_node_reused)
>  		return -ENODEV;
>  
> -	sl = of_modalias(dev->of_node, str, len - 2);
> -	if (sl < 0)
> -		return sl;
> -	if (sl > len - 2)
> +	sl = snprintf(str, len, "%pOFm\n", dev->of_node);
> +	if (sl >= len)
>  		return -ENOMEM;
> -
> -	str[sl++] = '\n';
> -	str[sl] = 0;
>  	return sl;
>  }

Ack.

>  EXPORT_SYMBOL_GPL(of_device_modalias);
> @@ -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);
>  }

Ack.

>  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;
> 

Ack.

>  	/* 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);

char *str __free(kfree) = NULL;

otherwise, wild pointer dereference may happen when return below.

>  
>  	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);

Ack.

>  }
>  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  may be removed. it does not matter to keep it here as well.

Ack.


>  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;
>  }
>  

Ack.

>  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;
>  		}

may Add below tag if the only issue is solved.

Reviewed-by: Zijun Hu <quic_zijuhu@quicinc.com>

Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Rasmus Villemoes]

On Tue, Dec 17 2024, "Rob Herring (Arm)" <robh@kernel.org> 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(-)

This diffstat lacks a lib/test_printf.c line. Please do add test cases
when extending vsnprintf().

>  
> 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 */


I took a look at of_modalias() with that change applied. While it does
seem to end up returning the required "total size had the buffer been
big enough", this part

                csize = snprintf(str, len, "C%s", compat);
                tsize += csize;
                if (csize >= len)
                        continue;

seems that it will overwrite/replace a longer compat string with a
shorter, later one, if we happen to be close to the end of the available
space. That's _probably_ not a problem for vsnprintf() itself, or
callers such as kasprintf() that do need the exact size but don't care
about what might have been produced on the first call to determine that
size, but the printf test suite does expect the result of a truncated
vsnprintf() to match the full string up to the truncation point. We can
probably allow certain test cases to opt out of certain sanity checks if
absolutely needed, but perhaps it's simpler to fix of_modalias(). 

Unrelated, I think the space replacement could be simplified to

  if (len > 0)
    strreplace(str, ' ', '_');
  
>  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;

This is definitely wrong. I think it's fixable by using

  buf < end ? end - buf : 0

Rasmus

ssize_t: was: Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Petr Mladek]

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

We should keep ssize_t.

"end - buf" passed from device_node_string() in vprintf.c might be
negative. The "buf" pointer is used to count the number of characters
which might be written when the buffer is big enough.

>  {
>  	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;

There is later called

		csize = snprintf(str, len, "C%s", compat);

and snprintf() uses size_t for the len attribute. It would go wild
when we pass a negative len.


> +	size_t csize;
> +	size_t tsize;
>  
>  	/* Name & Type */
>  	/* %p eats all alphanum characters, so %c must be used here */

Best Regards,
Petr

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

[Petr Mladek]

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

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

[John Ogness]

On 2024-12-18, Petr Mladek <pmladek@suse.com> wrote:
> My concern is taking a lock when processing a printf format, see
> below for more details.

Your concern is valid! printk() uses vsnprintf() to format records for
the kernel log. printk() may be called from contexts where locking is
forbidden (such as NMI). If vsnprintf() can take a lock, then either:

vsnprintf() must be made to be lockless

  or

printk() must take another approach to string formatting

  or

we accept that printk() can deadlock for certain format types in certain
contexts.

John Ogness

Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Rob Herring]

On Wed, Dec 18, 2024 at 4:16 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On Tue, Dec 17 2024, "Rob Herring (Arm)" <robh@kernel.org> 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(-)
>
> This diffstat lacks a lib/test_printf.c line. Please do add test cases
> when extending vsnprintf().

Thanks for the review.

There's tests in the DT unittest already for all the pOF formats. I
guess the exact conditions tested are different given the below issue.
I would be happy to move those tests, but that doesn't look completely
trivial. I suppose I can manually construct a struct device_node and
list of struct property, but that goes against efforts to make both of
those structs opaque outside of DT code. We could add helper
functions, but they would only be for this test as this is not the
normal way we handle DT nodes (i.e. created from a DTB and applied as
a changeset to the base tree).

> >
> > 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 */
>
>
> I took a look at of_modalias() with that change applied. While it does
> seem to end up returning the required "total size had the buffer been
> big enough", this part
>
>                 csize = snprintf(str, len, "C%s", compat);
>                 tsize += csize;
>                 if (csize >= len)
>                         continue;
>
> seems that it will overwrite/replace a longer compat string with a
> shorter, later one, if we happen to be close to the end of the available
> space. That's _probably_ not a problem for vsnprintf() itself, or
> callers such as kasprintf() that do need the exact size but don't care
> about what might have been produced on the first call to determine that
> size, but the printf test suite does expect the result of a truncated
> vsnprintf() to match the full string up to the truncation point. We can
> probably allow certain test cases to opt out of certain sanity checks if
> absolutely needed, but perhaps it's simpler to fix of_modalias().

Yeah, for purposes of of_modalias, if things don't fit it's going to
get thrown away and we don't care. But should be possible to fix.

>
> Unrelated, I think the space replacement could be simplified to
>
>   if (len > 0)
>     strreplace(str, ' ', '_');

Nice. That probably didn't exist at the time this was written.

Rob

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

[Rob Herring]

On Wed, Dec 18, 2024 at 6:27 AM Petr Mladek <pmladek@suse.com> wrote:
>
> 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()

If this doesn't take the DT spinlock, it is a bug (though next to 0
chance of hitting it). Getting the full path requires walking parent
nodes which should take the spinlock (See of_get_parent()). I think we
discussed this issue when this got converted to fwnode API...

The compatible format specifiers also take the lock... Those are
somewhat rarely used IIRC, so perhaps could get rid of them if "no
locks allowed" becomes the rule and we ignore the issue for getting
parent nodes.

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

I would also consider a put that goes to 0 a bug. A caller using the
format strings should hold a ref to the node already.

Rob

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

[Steven Rostedt]

On Wed, 18 Dec 2024 13:27:39 +0100
Petr Mladek <pmladek@suse.com> wrote:

> 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?

We already can take locks from printk formats.

  https://lore.kernel.org/all/20241120102602.3e17f2d5@gandalf.local.home/

At least when SELinux is enabled.

But that may be an SELinux issue and not a printk one.

-- Steve

Re: ssize_t: was: Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Rob Herring]

On Wed, Dec 18, 2024 at 5:35 AM Petr Mladek <pmladek@suse.com> wrote:
>
> 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.
> >
> > --- 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)
>
> We should keep ssize_t.

My intent was to align of_modalias() with snprintf()...

> "end - buf" passed from device_node_string() in vprintf.c might be
> negative. The "buf" pointer is used to count the number of characters
> which might be written when the buffer is big enough.

Isn't Rasmus' suggestion sufficient?:

buf += of_modalias(dn, buf, buf < end ? end - buf : 0)

Rob

Re: ssize_t: was: Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Petr Mladek]

On Wed 2024-12-18 11:10:54, Rob Herring wrote:
> On Wed, Dec 18, 2024 at 5:35 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > 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.
> > >
> > > --- 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)
> >
> > We should keep ssize_t.
> 
> My intent was to align of_modalias() with snprintf()...

Sure.

> > "end - buf" passed from device_node_string() in vprintf.c might be
> > negative. The "buf" pointer is used to count the number of characters
> > which might be written when the buffer is big enough.
> 
> Isn't Rasmus' suggestion sufficient?:
> 
> buf += of_modalias(dn, buf, buf < end ? end - buf : 0)

Yes, this should do the trick. I sent my overview before reading
Rasmus' reply.

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2024-12-18 15:13:13, John Ogness wrote:
> On 2024-12-18, Petr Mladek <pmladek@suse.com> wrote:
> > My concern is taking a lock when processing a printf format, see
> > below for more details.
> 
> Your concern is valid! printk() uses vsnprintf() to format records for
> the kernel log. printk() may be called from contexts where locking is
> forbidden (such as NMI). If vsnprintf() can take a lock, then either:
> 
> vsnprintf() must be made to be lockless
> 
>   or
> 
> printk() must take another approach to string formatting

There were the ideas to introduce a %pX[1] or %pf[2] formats. They would
allow to pass pointer to a custom callback and parameters which
then would be used by vsprintf(). This might allow to make it more
obvious that the given vsprintf()/printk() might do some locking.

>   or
> 
> we accept that printk() can deadlock for certain format types in certain
> contexts.

I see that %pOFm does not add any new locking dependency.
The same lock (devtree_lock) is already taken, for example,
by %pOFC format.

I do not want to revert everything now just because of theoretical
problems. It somehow works because people use these formats only
in dedicated subsystems. Also lockdep is able to catch a lot of
problems.

Well, it would be nice to document the lock dependency in
Documentation/core-api/printk-formats.rst

Best Regards,
Petr

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

[John Ogness]

On 2024-12-19, Petr Mladek <pmladek@suse.com> wrote:
> I do not want to revert everything now just because of theoretical
> problems.

What would you revert? This has always been an issue for printk().

> Well, it would be nice to document the lock dependency in
> Documentation/core-api/printk-formats.rst

Yes. If any locking is involved at all, such specifiers should be
documented as not safe in NMI context or within printk_cpu_sync
blocks. Also, it should be checked if all such locks are
raw_spinlock_t. If any other lock type is used, it probably is already
generating a lockdep splat since printk() formats records with local
interrupts off.

Perhaps we should create a kunit that calls printk() for each of the
supported specifiers and see if any lockdep splats appear.

John

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

[Petr Mladek]

On Thu 2024-12-19 20:17:21, John Ogness wrote:
> On 2024-12-19, Petr Mladek <pmladek@suse.com> wrote:
> > I do not want to revert everything now just because of theoretical
> > problems.
> 
> What would you revert? This has always been an issue for printk().

I did mean the already existing printf modifier which already
take a lock, for example, %pOFC.

> > Well, it would be nice to document the lock dependency in
> > Documentation/core-api/printk-formats.rst
> 
> Yes. If any locking is involved at all, such specifiers should be
> documented as not safe in NMI context or within printk_cpu_sync
> blocks. Also, it should be checked if all such locks are
> raw_spinlock_t. If any other lock type is used, it probably is already
> generating a lockdep splat since printk() formats records with local
> interrupts off.

Great point!

> Perhaps we should create a kunit that calls printk() for each of the
> supported specifiers and see if any lockdep splats appear.

It might be possible to somehow reuse the existing module lib/test_printf.c.

Best Regards,
Petr

Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Andy Shevchenko]

On Tue, Dec 17, 2024 at 12:37:09PM -0600, 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.

Shouldn't ACPI case also be considered? Otherwise we might see a deviation and
then completely asynced variants of modalias based on different type of fwnode.

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Thu, Dec 19, 2024 at 08:17:21PM +0106, John Ogness wrote:
> On 2024-12-19, Petr Mladek <pmladek@suse.com> wrote:
> > I do not want to revert everything now just because of theoretical
> > problems.
> 
> What would you revert? This has always been an issue for printk().
> 
> > Well, it would be nice to document the lock dependency in
> > Documentation/core-api/printk-formats.rst
> 
> Yes. If any locking is involved at all, such specifiers should be
> documented as not safe in NMI context or within printk_cpu_sync
> blocks. 

For the folks that don't read documentation, should we bail out on 
in_nmi() for these as well?

> Also, it should be checked if all such locks are
> raw_spinlock_t. If any other lock type is used, it probably is already
> generating a lockdep splat since printk() formats records with local
> interrupts off.

At least for DT, that is the case. There is a mutex, but that's only 
taken if the tree is modified.

> Perhaps we should create a kunit that calls printk() for each of the
> supported specifiers and see if any lockdep splats appear.

That's already in place with the DT unit test (which doesn't use kunit 
ATM).

Rob

Re: [PATCH] of: Add printf '%pOFm' for generating modalias

[Rob Herring]

On Mon, Dec 23, 2024 at 09:58:27PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 17, 2024 at 12:37:09PM -0600, 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.
> 
> Shouldn't ACPI case also be considered? Otherwise we might see a deviation and
> then completely asynced variants of modalias based on different type of fwnode.

I don't understand what you are proposing. How the ACPI code and DT code 
generate the DT modalias are completely different already. Are you 
wanting to plumb a fwnode modalias op? We'd probably be better off with 
a printf op handler and moving much of the code out of lib/vsprintf.c.

Also, looks like the ACPI modalias code does kmallocs. Given the locking 
discussion, wouldn't that be problematic?

Rob

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

[Petr Mladek]

On Mon 2024-12-30 14:26:43, Rob Herring wrote:
> On Thu, Dec 19, 2024 at 08:17:21PM +0106, John Ogness wrote:
> > On 2024-12-19, Petr Mladek <pmladek@suse.com> wrote:
> > > I do not want to revert everything now just because of theoretical
> > > problems.
> > 
> > What would you revert? This has always been an issue for printk().
> > 
> > > Well, it would be nice to document the lock dependency in
> > > Documentation/core-api/printk-formats.rst
> > 
> > Yes. If any locking is involved at all, such specifiers should be
> > documented as not safe in NMI context or within printk_cpu_sync
> > blocks. 
> 
> For the folks that don't read documentation, should we bail out on 
> in_nmi() for these as well?

I like this idea.

Best Regards,
Petr

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

[John Ogness]

On 2025-01-02, Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2024-12-30 14:26:43, Rob Herring wrote:
>> On Thu, Dec 19, 2024 at 08:17:21PM +0106, John Ogness wrote:
>> > On 2024-12-19, Petr Mladek <pmladek@suse.com> wrote:
>> > > I do not want to revert everything now just because of theoretical
>> > > problems.
>> > 
>> > What would you revert? This has always been an issue for printk().
>> > 
>> > > Well, it would be nice to document the lock dependency in
>> > > Documentation/core-api/printk-formats.rst
>> > 
>> > Yes. If any locking is involved at all, such specifiers should be
>> > documented as not safe in NMI context or within printk_cpu_sync
>> > blocks. 
>> 
>> For the folks that don't read documentation, should we bail out on 
>> in_nmi() for these as well?
>
> I like this idea.

Perhaps also include a check using the upcoming
is_printk_cpu_sync_owner() [0] as well.

John Ogness

[0] https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=for-6.14-cpu_sync-fixup&id=0161e2d6950fe66cf6ac1c10d945bae971f33667