[PATCH 0/2] Fix vargs handling in iucv_alloc_device()

[PATCH 0/2] Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

Export kobject_set_name_vargs() and use it in iucv_alloc_device() to
fix vargs handling.

@Greg and Rafael:
Could you please ack the first patch so I can take both patches via the
s390 tree?

Thank you

Heiko Carstens (1):
  s390/iucv: Fix vargs handling in iucv_alloc_device()

Vasily Gorbik (1):
  kobject: Export kobject_set_name_vargs() symbol

 lib/kobject.c   | 1 +
 net/iucv/iucv.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH 1/2] kobject: Export kobject_set_name_vargs() symbol

[Vasily Gorbik]

The net/iucv module requires the kobject_set_name_vargs() function to
set kobject names dynamically based on variable arguments. Export the
kobject_set_name_vargs() symbol, allowing the net/iucv module and other
potential modules to use this utility function.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202408091131.ATGn6YSh-lkp@intel.com/
Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
---
 lib/kobject.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index 72fa20f405f1..96caa8bfdc40 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -295,6 +295,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 
 	return 0;
 }
+EXPORT_SYMBOL(kobject_set_name_vargs);
 
 /**
  * kobject_set_name() - Set the name of a kobject.
-- 
2.41.0

Re: [PATCH 1/2] kobject: Export kobject_set_name_vargs() symbol

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 12:42:34PM +0200, Vasily Gorbik wrote:
> The net/iucv module requires the kobject_set_name_vargs() function to
> set kobject names dynamically based on variable arguments. Export the
> kobject_set_name_vargs() symbol, allowing the net/iucv module and other
> potential modules to use this utility function.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202408091131.ATGn6YSh-lkp@intel.com/
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>  lib/kobject.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 72fa20f405f1..96caa8bfdc40 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -295,6 +295,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(kobject_set_name_vargs);

EXPORT_SYMBOL_GPL() please.

thanks,

greg k-h

[PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

From: Heiko Carstens <hca@linux.ibm.com>

iucv_alloc_device() gets a format string and a varying number of
arguments. This is incorrectly forwarded by calling dev_set_name() with
the format string and a va_list, while dev_set_name() expects also a
varying number of arguments.

Fix this and call kobject_set_name_vargs() instead which expects a
va_list parameter.

Fixes: 4452e8ef8c36 ("s390/iucv: Provide iucv_alloc_device() / iucv_release_device()")
Reference-ID: https://bugzilla.linux.ibm.com/show_bug.cgi?id=208008
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 net/iucv/iucv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 1e42e13ad24e..64102a31b569 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -92,7 +92,7 @@ struct device *iucv_alloc_device(const struct attribute_group **attrs,
 	if (!dev)
 		goto out_error;
 	va_start(vargs, fmt);
-	rc = dev_set_name(dev, fmt, vargs);
+	rc = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
 	va_end(vargs);
 	if (rc)
 		goto out_error;
-- 
2.41.0

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 12:42:37PM +0200, Vasily Gorbik wrote:
> From: Heiko Carstens <hca@linux.ibm.com>
> 
> iucv_alloc_device() gets a format string and a varying number of
> arguments. This is incorrectly forwarded by calling dev_set_name() with
> the format string and a va_list, while dev_set_name() expects also a
> varying number of arguments.
> 
> Fix this and call kobject_set_name_vargs() instead which expects a
> va_list parameter.

I don't understand, why can't dev_set_name() be called here?

Calling "raw" kobject functions is almost never the correct thing to be
doing, ESPECIALLY as you have a struct device here.

thanks,

greg k-h

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

On Tue, Aug 13, 2024 at 12:52:19PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2024 at 12:42:37PM +0200, Vasily Gorbik wrote:
> > From: Heiko Carstens <hca@linux.ibm.com>
> > 
> > iucv_alloc_device() gets a format string and a varying number of
> > arguments. This is incorrectly forwarded by calling dev_set_name() with
> > the format string and a va_list, while dev_set_name() expects also a
> > varying number of arguments.
> > 
> > Fix this and call kobject_set_name_vargs() instead which expects a
> > va_list parameter.
> 
> I don't understand, why can't dev_set_name() be called here?
> 
> Calling "raw" kobject functions is almost never the correct thing to be
> doing, ESPECIALLY as you have a struct device here.

struct device *iucv_alloc_device(const struct attribute_group **attrs,
                                 void *priv, const char *fmt, ...);

va_start(vargs, fmt); initializes vargs to point to the first argument after fmt.

__printf(2, 0) int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, va_list vargs);

__printf(2, 3) int dev_set_name(struct device *dev, const char *name, ...);

dev_set_name is expecting to receive individual variable arguments
directly (...), not a va_list.

The (...) in dev_set_name is meant to be expanded into individual
arguments, but when you pass a va_list to it, this expansion doesn't
happen. Instead, the va_list is just treated as a pointer or a single
argument, leading to undefined or incorrect behavior.

So, would it be okay to reuse kobject_set_name_vargs() here, or would you propose
introducing another helper just for this case? e.g.

int dev_set_name_vargs(struct device *dev, const char *fmt, va_list vargs)
{
჻·······return kobject_set_name_vargs(&dev->kobj, fmt, vargs);
}
EXPORT_SYMBOL_GPL(dev_set_name_vargs)

The bz link should be:
Link: https://bugzilla.suse.com/show_bug.cgi?id=1228425

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 01:50:27PM +0200, Vasily Gorbik wrote:
> On Tue, Aug 13, 2024 at 12:52:19PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 13, 2024 at 12:42:37PM +0200, Vasily Gorbik wrote:
> > > From: Heiko Carstens <hca@linux.ibm.com>
> > > 
> > > iucv_alloc_device() gets a format string and a varying number of
> > > arguments. This is incorrectly forwarded by calling dev_set_name() with
> > > the format string and a va_list, while dev_set_name() expects also a
> > > varying number of arguments.
> > > 
> > > Fix this and call kobject_set_name_vargs() instead which expects a
> > > va_list parameter.
> > 
> > I don't understand, why can't dev_set_name() be called here?
> > 
> > Calling "raw" kobject functions is almost never the correct thing to be
> > doing, ESPECIALLY as you have a struct device here.
> 
> struct device *iucv_alloc_device(const struct attribute_group **attrs,
>                                  void *priv, const char *fmt, ...);
> 
> va_start(vargs, fmt); initializes vargs to point to the first argument after fmt.
> 
> __printf(2, 0) int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, va_list vargs);
> 
> __printf(2, 3) int dev_set_name(struct device *dev, const char *name, ...);
> 
> dev_set_name is expecting to receive individual variable arguments
> directly (...), not a va_list.
> 
> The (...) in dev_set_name is meant to be expanded into individual
> arguments, but when you pass a va_list to it, this expansion doesn't
> happen. Instead, the va_list is just treated as a pointer or a single
> argument, leading to undefined or incorrect behavior.
> 
> So, would it be okay to reuse kobject_set_name_vargs() here, or would you propose
> introducing another helper just for this case? e.g.
> 
> int dev_set_name_vargs(struct device *dev, const char *fmt, va_list vargs)
> {
> ჻·······return kobject_set_name_vargs(&dev->kobj, fmt, vargs);
> }
> EXPORT_SYMBOL_GPL(dev_set_name_vargs)

This function makes more sense if you really want to do this.

But step back, why is this needed at all anyway?  No other subsystem or
driver needs/wants this, what makes this api so special?  Why not figure
out your name beforehand?

thanks,

greg k-h

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

On Tue, Aug 13, 2024 at 02:43:33PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2024 at 01:50:27PM +0200, Vasily Gorbik wrote:
> > On Tue, Aug 13, 2024 at 12:52:19PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 13, 2024 at 12:42:37PM +0200, Vasily Gorbik wrote:
> > > > From: Heiko Carstens <hca@linux.ibm.com>
> > > > 
> > > > iucv_alloc_device() gets a format string and a varying number of
> > > > arguments. This is incorrectly forwarded by calling dev_set_name() with
> > > > the format string and a va_list, while dev_set_name() expects also a
> > > > varying number of arguments.
> > > > 
> > > > Fix this and call kobject_set_name_vargs() instead which expects a
> > > > va_list parameter.
> > > 
> > > I don't understand, why can't dev_set_name() be called here?
> > > 
> > > Calling "raw" kobject functions is almost never the correct thing to be
> > > doing, ESPECIALLY as you have a struct device here.
> > 
> > struct device *iucv_alloc_device(const struct attribute_group **attrs,
> >                                  void *priv, const char *fmt, ...);
> > 
> > va_start(vargs, fmt); initializes vargs to point to the first argument after fmt.
> > 
> > __printf(2, 0) int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, va_list vargs);
> > 
> > __printf(2, 3) int dev_set_name(struct device *dev, const char *name, ...);
> > 
> > dev_set_name is expecting to receive individual variable arguments
> > directly (...), not a va_list.
> > 
> > The (...) in dev_set_name is meant to be expanded into individual
> > arguments, but when you pass a va_list to it, this expansion doesn't
> > happen. Instead, the va_list is just treated as a pointer or a single
> > argument, leading to undefined or incorrect behavior.
> > 
> > So, would it be okay to reuse kobject_set_name_vargs() here, or would you propose
> > introducing another helper just for this case? e.g.
> > 
> > int dev_set_name_vargs(struct device *dev, const char *fmt, va_list vargs)
> > {
> > ჻·······return kobject_set_name_vargs(&dev->kobj, fmt, vargs);
> > }
> > EXPORT_SYMBOL_GPL(dev_set_name_vargs)
> 
> This function makes more sense if you really want to do this.
> 
> But step back, why is this needed at all anyway?  No other subsystem or
> driver needs/wants this, what makes this api so special?  Why not figure
> out your name beforehand?

That's comming from this patch series:
https://lore.kernel.org/all/20240506194454.1160315-1-hca@linux.ibm.com/#t

"""
Unify IUCV device allocation as suggested by Arnd Bergmann in order
to get rid of code duplication in various device drivers.
"""

It just introduces and utilizes a couple of wrappers to reduce code
duplication, and in this case, introducing this level of indirection
led to the need for forwarding vargs.

And reimplementing parts of kobject_set_name_vargs to format the device
name beforehand is probably not what we want.

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Alexandra Winter]

On 13.08.24 14:43, Greg Kroah-Hartman wrote:
>>> I don't understand, why can't dev_set_name() be called here?
>>>
[...]
> 
> But step back, why is this needed at all anyway?  No other subsystem or
> driver needs/wants this, what makes this api so special?  Why not figure
> out your name beforehand?
> 
> thanks,


Vasily, the following update to Heiko's patch does not touch lib/kobject.c
According to a quick test it still solves the original issue and does compile
with W=1 and iucv as a module.

diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 64102a31b569..6a819ba4ccab 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -86,13 +86,17 @@ struct device *iucv_alloc_device(const struct attribute_group **attrs,
 {
        struct device *dev;
        va_list vargs;
+       char buf[20];
        int rc;

        dev = kzalloc(sizeof(*dev), GFP_KERNEL);
        if (!dev)
                goto out_error;
        va_start(vargs, fmt);
-       rc = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
+       rc = vsnprintf(buf, 20, fmt, vargs);
+       if (!rc)
+               rc = dev_set_name(dev, buf);
        va_end(vargs);
        if (rc)
                goto out_error;



Maybe Greg has somethign like this in mind?

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

On Tue, Aug 13, 2024 at 03:35:48PM +0200, Alexandra Winter wrote:
> 
> 
> On 13.08.24 14:43, Greg Kroah-Hartman wrote:
> >>> I don't understand, why can't dev_set_name() be called here?
> >>>
> [...]
> > 
> > But step back, why is this needed at all anyway?  No other subsystem or
> > driver needs/wants this, what makes this api so special?  Why not figure
> > out your name beforehand?
> > 
> > thanks,
> 
> 
> Vasily, the following update to Heiko's patch does not touch lib/kobject.c
> According to a quick test it still solves the original issue and does compile
> with W=1 and iucv as a module.
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 64102a31b569..6a819ba4ccab 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -86,13 +86,17 @@ struct device *iucv_alloc_device(const struct attribute_group **attrs,
>  {
>         struct device *dev;
>         va_list vargs;
> +       char buf[20];
>         int rc;
> 
>         dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>         if (!dev)
>                 goto out_error;
>         va_start(vargs, fmt);
> -       rc = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
> +       rc = vsnprintf(buf, 20, fmt, vargs);
> +       if (!rc)
> +               rc = dev_set_name(dev, buf);
>         va_end(vargs);
>         if (rc)
>                 goto out_error;
> 
> Maybe Greg has somethign like this in mind?

Thanks Alexandra,
 
but I'm still leaning towards forwarding vargs and avoid splitting the name
formatting logic, if Greg agrees that the use case justifies adding a
new helper. Let's see what Greg prefers.
 
int dev_set_name_vargs(struct device *dev, const char *fmt, va_list vargs)
{
        return kobject_set_name_vargs(&dev->kobj, fmt, vargs);
}
EXPORT_SYMBOL_GPL(dev_set_name_vargs)

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 03:35:48PM +0200, Alexandra Winter wrote:
> 
> 
> On 13.08.24 14:43, Greg Kroah-Hartman wrote:
> >>> I don't understand, why can't dev_set_name() be called here?
> >>>
> [...]
> > 
> > But step back, why is this needed at all anyway?  No other subsystem or
> > driver needs/wants this, what makes this api so special?  Why not figure
> > out your name beforehand?
> > 
> > thanks,
> 
> 
> Vasily, the following update to Heiko's patch does not touch lib/kobject.c
> According to a quick test it still solves the original issue and does compile
> with W=1 and iucv as a module.
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 64102a31b569..6a819ba4ccab 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -86,13 +86,17 @@ struct device *iucv_alloc_device(const struct attribute_group **attrs,
>  {
>         struct device *dev;
>         va_list vargs;
> +       char buf[20];
>         int rc;
> 
>         dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>         if (!dev)
>                 goto out_error;
>         va_start(vargs, fmt);
> -       rc = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
> +       rc = vsnprintf(buf, 20, fmt, vargs);
> +       if (!rc)
> +               rc = dev_set_name(dev, buf);

This looks best, let's not create a core function that no one has ever
needed yet just for one user :)

thanks,

greg k-h

Re: [PATCH 2/2] s390/iucv: Fix vargs handling in iucv_alloc_device()

[Vasily Gorbik]

On Tue, Aug 13, 2024 at 05:29:52PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2024 at 03:35:48PM +0200, Alexandra Winter wrote:
> > 
> > 
> > On 13.08.24 14:43, Greg Kroah-Hartman wrote:
> > >>> I don't understand, why can't dev_set_name() be called here?
> > >>>
> > [...]
> > > 
> > > But step back, why is this needed at all anyway?  No other subsystem or
> > > driver needs/wants this, what makes this api so special?  Why not figure
> > > out your name beforehand?
> > > 
> > > thanks,
> > 
> > 
> > Vasily, the following update to Heiko's patch does not touch lib/kobject.c
> > According to a quick test it still solves the original issue and does compile
> > with W=1 and iucv as a module.
> > 
> > diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> > index 64102a31b569..6a819ba4ccab 100644
> > --- a/net/iucv/iucv.c
> > +++ b/net/iucv/iucv.c
> > @@ -86,13 +86,17 @@ struct device *iucv_alloc_device(const struct attribute_group **attrs,
> >  {
> >         struct device *dev;
> >         va_list vargs;
> > +       char buf[20];
> >         int rc;
> > 
> >         dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >         if (!dev)
> >                 goto out_error;
> >         va_start(vargs, fmt);
> > -       rc = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
> > +       rc = vsnprintf(buf, 20, fmt, vargs);
> > +       if (!rc)
> > +               rc = dev_set_name(dev, buf);
> 
> This looks best, let's not create a core function that no one has ever
> needed yet just for one user :)

Okay, fair enough. Thank you, Greg!

I'll drop these two patches. Alexandra, I assume you will send out your
alternative fix separately. Thank you!