Message-ID: <CAL_JsqKuzMMDthdhQgGfmsKQySMReNX2kL4CvMsbKCo96AH_JQ@mail.gmail.com>
Date: Wed, 18 Dec 2024 09:28:05 -0600
From: Rob Herring <robh@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Petr Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>, Jonathan Corbet <corbet@....net>, 
	Saravana Kannan <saravanak@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Zijun Hu <quic_zijuhu@...cinc.com>, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH] of: Add printf '%pOFm' for generating modalias

On Wed, Dec 18, 2024 at 4:16 AM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> On Tue, Dec 17 2024, "Rob Herring (Arm)" <robh@...nel.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@...cinc.com>
> > Signed-off-by: Rob Herring (Arm) <robh@...nel.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

Powered by blists - more mailing lists