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@xxxxxxxxxxx>
> Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
