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.0On 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
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
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
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?
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
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!
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)
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.
© 2016 - 2026 Red Hat, Inc.