On Wed, Dec 18, 2024 at 6:27 AM Petr Mladek <pmladek@xxxxxxxx> 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@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
> >
> > 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
