Setting name as per the format is not only useful for kobjects.
It can also be used to set name for other things for e.g. setting
the name of the struct attribute when multiple same kind of attributes
need to be created with some identifier in name, instead of managing
memory for names at such places case by case, it would be good if
something like current kobject_set_name_vargs() can be utilized.
Refactor kobject_set_name_vargs(), Create a new generic function
set_name_vargs() which can be used for kobjects as well as at
other places.
This patch doesn't introduce any functionality change.
Signed-off-by: Jagdish Gediya <[email protected]>
---
include/linux/string.h | 1 +
lib/kobject.c | 30 +-----------------------------
mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index b6572aeca2f5..f329962e5ae9 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -9,6 +9,7 @@
#include <linux/stdarg.h>
#include <uapi/linux/string.h>
+int set_name_vargs(const char **name, const char *fmt, va_list vargs);
extern char *strndup_user(const char __user *, long);
extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
diff --git a/lib/kobject.c b/lib/kobject.c
index 5f0e71ab292c..870d05971e3a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
va_list vargs)
{
- const char *s;
-
- if (kobj->name && !fmt)
- return 0;
-
- s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
- if (!s)
- return -ENOMEM;
-
- /*
- * ewww... some of these buggers have '/' in the name ... If
- * that's the case, we need to make sure we have an actual
- * allocated copy to modify, since kvasprintf_const may have
- * returned something from .rodata.
- */
- if (strchr(s, '/')) {
- char *t;
-
- t = kstrdup(s, GFP_KERNEL);
- kfree_const(s);
- if (!t)
- return -ENOMEM;
- strreplace(t, '/', '!');
- s = t;
- }
- kfree_const(kobj->name);
- kobj->name = s;
-
- return 0;
+ return set_name_vargs(&kobj->name, fmt, vargs);
}
/**
diff --git a/mm/util.c b/mm/util.c
index 54e5e761a9a9..808d29b17ea7 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
}
EXPORT_SYMBOL(kstrndup);
+/**
+ * set_name_vargs() - Set the name as per format
+ * @name: pointer to point to the name as per format
+ * @fmt: format string used to build the name
+ * @vargs: vargs to format the string.
+ */
+int set_name_vargs(const char **name, const char *fmt, va_list vargs)
+{
+ const char *s;
+
+ if (*name && !fmt)
+ return 0;
+
+ s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
+ if (!s)
+ return -ENOMEM;
+
+ /*
+ * ewww... some of these buggers have '/' in the name ... If
+ * that's the case, we need to make sure we have an actual
+ * allocated copy to modify, since kvasprintf_const may have
+ * returned something from .rodata.
+ */
+ if (strchr(s, '/')) {
+ char *t;
+
+ t = kstrdup(s, GFP_KERNEL);
+ kfree_const(s);
+ if (!t)
+ return -ENOMEM;
+ strreplace(t, '/', '!');
+ s = t;
+ }
+ kfree_const(*name);
+ *name = s;
+
+ return 0;
+}
+EXPORT_SYMBOL(set_name_vargs);
+
/**
* kmemdup - duplicate region of memory
*
--
2.35.1
On Fri, May 06, 2022 at 02:46:47PM +0000, David Laight wrote:
> From: Jagdish Gediya
> > Sent: 06 May 2022 14:33
> >
> > Setting name as per the format is not only useful for kobjects.
> > It can also be used to set name for other things for e.g. setting
> > the name of the struct attribute when multiple same kind of attributes
> > need to be created with some identifier in name, instead of managing
> > memory for names at such places case by case, it would be good if
> > something like current kobject_set_name_vargs() can be utilized.
> >
> > Refactor kobject_set_name_vargs(), Create a new generic function
> > set_name_vargs() which can be used for kobjects as well as at
> > other places.
> >
> > This patch doesn't introduce any functionality change.
> >
> > Signed-off-by: Jagdish Gediya <[email protected]>
> > ---
> > include/linux/string.h | 1 +
> > lib/kobject.c | 30 +-----------------------------
> > mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index b6572aeca2f5..f329962e5ae9 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -9,6 +9,7 @@
> > #include <linux/stdarg.h>
> > #include <uapi/linux/string.h>
> >
> > +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> > extern char *strndup_user(const char __user *, long);
> > extern void *memdup_user(const void __user *, size_t);
> > extern void *vmemdup_user(const void __user *, size_t);
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 5f0e71ab292c..870d05971e3a 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> > int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> > va_list vargs)
> > {
> ...
> > + return set_name_vargs(&kobj->name, fmt, vargs);
> > }
> >
> > /**
> > diff --git a/mm/util.c b/mm/util.c
> > index 54e5e761a9a9..808d29b17ea7 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> > }
> > EXPORT_SYMBOL(kstrndup);
> >
> > +/**
> > + * set_name_vargs() - Set the name as per format
> > + * @name: pointer to point to the name as per format
> > + * @fmt: format string used to build the name
> > + * @vargs: vargs to format the string.
> > + */
> > +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
> > +{
> > + const char *s;
> > +
> > + if (*name && !fmt)
> > + return 0;
> > +
> > + s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> > + if (!s)
> > + return -ENOMEM;
> > +
> > + /*
> > + * ewww... some of these buggers have '/' in the name ... If
> > + * that's the case, we need to make sure we have an actual
> > + * allocated copy to modify, since kvasprintf_const may have
> > + * returned something from .rodata.
> > + */
> > + if (strchr(s, '/')) {
> > + char *t;
> > +
> > + t = kstrdup(s, GFP_KERNEL);
> > + kfree_const(s);
> > + if (!t)
> > + return -ENOMEM;
>
> There is a kstrdup_const() that will DTRT here.
>
> > + strreplace(t, '/', '!');
> > + s = t;
> > + }
> > + kfree_const(*name);
> > + *name = s;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(set_name_vargs);
>
> Are you sure this can ever work from a module?
> This all relies on:
>
> static inline bool is_kernel_rodata(unsigned long addr)
> {
> return addr >= (unsigned long)__start_rodata &&
> addr < (unsigned long)__end_rodata;
> }
>
> which isn't going to do anything sane given an "xxx" inside a module.
>
> Indeed can kobject_set_name_vargs() end up with a constant string
> inside a module?
No, kobject_set_name_vargs() is not exported. I exported
set_name_vargs() because it can have a broader use, but you are right
it shouldn't be exported.
> Seems horribly fragile.
>
> David
>
> > +
> > /**
> > * kmemdup - duplicate region of memory
> > *
> > --
> > 2.35.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>
On Fri, May 06, 2022 at 04:02:28PM +0200, Greg KH wrote:
> On Fri, May 06, 2022 at 07:03:09PM +0530, Jagdish Gediya wrote:
> > Setting name as per the format is not only useful for kobjects.
> > It can also be used to set name for other things for e.g. setting
> > the name of the struct attribute when multiple same kind of attributes
> > need to be created with some identifier in name, instead of managing
> > memory for names at such places case by case, it would be good if
> > something like current kobject_set_name_vargs() can be utilized.
> >
> > Refactor kobject_set_name_vargs(), Create a new generic function
> > set_name_vargs() which can be used for kobjects as well as at
> > other places.
> >
> > This patch doesn't introduce any functionality change.
> >
> > Signed-off-by: Jagdish Gediya <[email protected]>
> > ---
> > include/linux/string.h | 1 +
> > lib/kobject.c | 30 +-----------------------------
> > mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index b6572aeca2f5..f329962e5ae9 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -9,6 +9,7 @@
> > #include <linux/stdarg.h>
> > #include <uapi/linux/string.h>
> >
> > +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> > extern char *strndup_user(const char __user *, long);
> > extern void *memdup_user(const void __user *, size_t);
> > extern void *vmemdup_user(const void __user *, size_t);
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 5f0e71ab292c..870d05971e3a 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> > int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> > va_list vargs)
> > {
> > - const char *s;
> > -
> > - if (kobj->name && !fmt)
> > - return 0;
> > -
> > - s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> > - if (!s)
> > - return -ENOMEM;
> > -
> > - /*
> > - * ewww... some of these buggers have '/' in the name ... If
> > - * that's the case, we need to make sure we have an actual
> > - * allocated copy to modify, since kvasprintf_const may have
> > - * returned something from .rodata.
> > - */
> > - if (strchr(s, '/')) {
> > - char *t;
> > -
> > - t = kstrdup(s, GFP_KERNEL);
> > - kfree_const(s);
> > - if (!t)
> > - return -ENOMEM;
> > - strreplace(t, '/', '!');
> > - s = t;
> > - }
> > - kfree_const(kobj->name);
> > - kobj->name = s;
> > -
> > - return 0;
> > + return set_name_vargs(&kobj->name, fmt, vargs);
> > }
> >
> > /**
> > diff --git a/mm/util.c b/mm/util.c
> > index 54e5e761a9a9..808d29b17ea7 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> > }
> > EXPORT_SYMBOL(kstrndup);
> >
> > +/**
> > + * set_name_vargs() - Set the name as per format
> > + * @name: pointer to point to the name as per format
> > + * @fmt: format string used to build the name
> > + * @vargs: vargs to format the string.
> > + */
> > +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
>
> Why is this a mm/ thing and not a lib/ thing?
I think it can be moved to lib/, Will correct it in next version.
> And who else will be needing to use this? Why the churn for no
> actual users?
I am working on a sepatare series for memory tiers where this kind
of functionality is needed, Based on numa topology of the system,
We can build the memory tiers nodemasks based on numa distances,
such masks need to be viewed/stored from sysfs using interfaces
/sys/*/memory_tier0, /sys/*/memory_tier1 etc. there can be upto
MAX_TIERS of memory tiers in the system, so we can define struct
device_attribute array statically but their names need to be set
as per tier number where this function is useful.
Also I think this requirement is generic although there are no
current users, It would be useful to not just restrict it to
kobjects.
> > +{
> > + const char *s;
> > +
> > + if (*name && !fmt)
> > + return 0;
> > +
> > + s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> > + if (!s)
> > + return -ENOMEM;
> > +
> > + /*
> > + * ewww... some of these buggers have '/' in the name ... If
> > + * that's the case, we need to make sure we have an actual
> > + * allocated copy to modify, since kvasprintf_const may have
> > + * returned something from .rodata.
> > + */
> > + if (strchr(s, '/')) {
> > + char *t;
> > +
> > + t = kstrdup(s, GFP_KERNEL);
> > + kfree_const(s);
> > + if (!t)
> > + return -ENOMEM;
> > + strreplace(t, '/', '!');
> > + s = t;
> > + }
> > + kfree_const(*name);
> > + *name = s;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(set_name_vargs);
>
> No need to export this as there are no users in modules.
Will remove in next version.
> And if there was, shouldn't it be EXPORT_SYMBOL_GPL() as that's what the
> kobject functions are exported as (most of them at least.)
>
> But again, why is this needed at all?
>
> thanks,
>
> greg k-h
>
From: Jagdish Gediya
> Sent: 06 May 2022 18:24
...
> > Are you sure this can ever work from a module?
> > This all relies on:
> >
> > static inline bool is_kernel_rodata(unsigned long addr)
> > {
> > return addr >= (unsigned long)__start_rodata &&
> > addr < (unsigned long)__end_rodata;
> > }
> >
> > which isn't going to do anything sane given an "xxx" inside a module.
> >
> > Indeed can kobject_set_name_vargs() end up with a constant string
> > inside a module?
>
> No, kobject_set_name_vargs() is not exported. I exported
> set_name_vargs() because it can have a broader use, but you are right
> it shouldn't be exported.
I was thinking that some function that creates a 'kobject'
could easily be called from a module - so you end up calling
the set_name code from a module and then end up calling
kfree() on a literal from a module.
Now it might be that it is impossible to actually the quoted
literal from a module into somewhere that is_kernel_rodata()
is applied to.
But, as i said, it is fragile.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi Jagdish,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on hnaz-mm/master linux/master linus/master v5.18-rc5 next-20220506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jagdish-Gediya/kobject-Refactor-kobject_set_name_vargs/20220506-213448
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git bc443c31def574e3bfaed50cb493b8305ad79435
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220507/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/4aac95bc8a052e61d5013a25a2099ed711e88fde
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jagdish-Gediya/kobject-Refactor-kobject_set_name_vargs/20220506-213448
git checkout 4aac95bc8a052e61d5013a25a2099ed711e88fde
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
mm/util.c: In function 'set_name_vargs':
>> mm/util.c:128:9: warning: function 'set_name_vargs' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
128 | s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
| ^
vim +128 mm/util.c
114
115 /**
116 * set_name_vargs() - Set the name as per format
117 * @name: pointer to point to the name as per format
118 * @fmt: format string used to build the name
119 * @vargs: vargs to format the string.
120 */
121 int set_name_vargs(const char **name, const char *fmt, va_list vargs)
122 {
123 const char *s;
124
125 if (*name && !fmt)
126 return 0;
127
> 128 s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
129 if (!s)
130 return -ENOMEM;
131
132 /*
133 * ewww... some of these buggers have '/' in the name ... If
134 * that's the case, we need to make sure we have an actual
135 * allocated copy to modify, since kvasprintf_const may have
136 * returned something from .rodata.
137 */
138 if (strchr(s, '/')) {
139 char *t;
140
141 t = kstrdup(s, GFP_KERNEL);
142 kfree_const(s);
143 if (!t)
144 return -ENOMEM;
145 strreplace(t, '/', '!');
146 s = t;
147 }
148 kfree_const(*name);
149 *name = s;
150
151 return 0;
152 }
153 EXPORT_SYMBOL(set_name_vargs);
154
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Fri, May 06, 2022 at 07:59:42PM +0530, Jagdish Gediya wrote:
> On Fri, May 06, 2022 at 04:02:28PM +0200, Greg KH wrote:
> > On Fri, May 06, 2022 at 07:03:09PM +0530, Jagdish Gediya wrote:
> > > Setting name as per the format is not only useful for kobjects.
> > > It can also be used to set name for other things for e.g. setting
> > > the name of the struct attribute when multiple same kind of attributes
> > > need to be created with some identifier in name, instead of managing
> > > memory for names at such places case by case, it would be good if
> > > something like current kobject_set_name_vargs() can be utilized.
> > >
> > > Refactor kobject_set_name_vargs(), Create a new generic function
> > > set_name_vargs() which can be used for kobjects as well as at
> > > other places.
> > >
> > > This patch doesn't introduce any functionality change.
> > >
> > > Signed-off-by: Jagdish Gediya <[email protected]>
> > > ---
> > > include/linux/string.h | 1 +
> > > lib/kobject.c | 30 +-----------------------------
> > > mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 42 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > index b6572aeca2f5..f329962e5ae9 100644
> > > --- a/include/linux/string.h
> > > +++ b/include/linux/string.h
> > > @@ -9,6 +9,7 @@
> > > #include <linux/stdarg.h>
> > > #include <uapi/linux/string.h>
> > >
> > > +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> > > extern char *strndup_user(const char __user *, long);
> > > extern void *memdup_user(const void __user *, size_t);
> > > extern void *vmemdup_user(const void __user *, size_t);
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index 5f0e71ab292c..870d05971e3a 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> > > int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> > > va_list vargs)
> > > {
> > > - const char *s;
> > > -
> > > - if (kobj->name && !fmt)
> > > - return 0;
> > > -
> > > - s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> > > - if (!s)
> > > - return -ENOMEM;
> > > -
> > > - /*
> > > - * ewww... some of these buggers have '/' in the name ... If
> > > - * that's the case, we need to make sure we have an actual
> > > - * allocated copy to modify, since kvasprintf_const may have
> > > - * returned something from .rodata.
> > > - */
> > > - if (strchr(s, '/')) {
> > > - char *t;
> > > -
> > > - t = kstrdup(s, GFP_KERNEL);
> > > - kfree_const(s);
> > > - if (!t)
> > > - return -ENOMEM;
> > > - strreplace(t, '/', '!');
> > > - s = t;
> > > - }
> > > - kfree_const(kobj->name);
> > > - kobj->name = s;
> > > -
> > > - return 0;
> > > + return set_name_vargs(&kobj->name, fmt, vargs);
> > > }
> > >
> > > /**
> > > diff --git a/mm/util.c b/mm/util.c
> > > index 54e5e761a9a9..808d29b17ea7 100644
> > > --- a/mm/util.c
> > > +++ b/mm/util.c
> > > @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> > > }
> > > EXPORT_SYMBOL(kstrndup);
> > >
> > > +/**
> > > + * set_name_vargs() - Set the name as per format
> > > + * @name: pointer to point to the name as per format
> > > + * @fmt: format string used to build the name
> > > + * @vargs: vargs to format the string.
> > > + */
> > > +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
> >
> > Why is this a mm/ thing and not a lib/ thing?
>
> I think it can be moved to lib/, Will correct it in next version.
>
> > And who else will be needing to use this? Why the churn for no
> > actual users?
>
> I am working on a sepatare series for memory tiers where this kind
> of functionality is needed, Based on numa topology of the system,
> We can build the memory tiers nodemasks based on numa distances,
> such masks need to be viewed/stored from sysfs using interfaces
> /sys/*/memory_tier0, /sys/*/memory_tier1 etc. there can be upto
> MAX_TIERS of memory tiers in the system, so we can define struct
> device_attribute array statically but their names need to be set
> as per tier number where this function is useful.
>
> Also I think this requirement is generic although there are no
> current users, It would be useful to not just restrict it to
> kobjects.
We don't make changes unless they are needed. This can be part of a
patch series that uses the change, otherwise it's unneeded churn right
now.
thanks,
greg k-h
From: Jagdish Gediya
> Sent: 06 May 2022 14:33
>
> Setting name as per the format is not only useful for kobjects.
> It can also be used to set name for other things for e.g. setting
> the name of the struct attribute when multiple same kind of attributes
> need to be created with some identifier in name, instead of managing
> memory for names at such places case by case, it would be good if
> something like current kobject_set_name_vargs() can be utilized.
>
> Refactor kobject_set_name_vargs(), Create a new generic function
> set_name_vargs() which can be used for kobjects as well as at
> other places.
>
> This patch doesn't introduce any functionality change.
>
> Signed-off-by: Jagdish Gediya <[email protected]>
> ---
> include/linux/string.h | 1 +
> lib/kobject.c | 30 +-----------------------------
> mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..f329962e5ae9 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -9,6 +9,7 @@
> #include <linux/stdarg.h>
> #include <uapi/linux/string.h>
>
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> extern char *strndup_user(const char __user *, long);
> extern void *memdup_user(const void __user *, size_t);
> extern void *vmemdup_user(const void __user *, size_t);
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 5f0e71ab292c..870d05971e3a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> va_list vargs)
> {
...
> + return set_name_vargs(&kobj->name, fmt, vargs);
> }
>
> /**
> diff --git a/mm/util.c b/mm/util.c
> index 54e5e761a9a9..808d29b17ea7 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> }
> EXPORT_SYMBOL(kstrndup);
>
> +/**
> + * set_name_vargs() - Set the name as per format
> + * @name: pointer to point to the name as per format
> + * @fmt: format string used to build the name
> + * @vargs: vargs to format the string.
> + */
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
> +{
> + const char *s;
> +
> + if (*name && !fmt)
> + return 0;
> +
> + s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> + if (!s)
> + return -ENOMEM;
> +
> + /*
> + * ewww... some of these buggers have '/' in the name ... If
> + * that's the case, we need to make sure we have an actual
> + * allocated copy to modify, since kvasprintf_const may have
> + * returned something from .rodata.
> + */
> + if (strchr(s, '/')) {
> + char *t;
> +
> + t = kstrdup(s, GFP_KERNEL);
> + kfree_const(s);
> + if (!t)
> + return -ENOMEM;
There is a kstrdup_const() that will DTRT here.
> + strreplace(t, '/', '!');
> + s = t;
> + }
> + kfree_const(*name);
> + *name = s;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(set_name_vargs);
Are you sure this can ever work from a module?
This all relies on:
static inline bool is_kernel_rodata(unsigned long addr)
{
return addr >= (unsigned long)__start_rodata &&
addr < (unsigned long)__end_rodata;
}
which isn't going to do anything sane given an "xxx" inside a module.
Indeed can kobject_set_name_vargs() end up with a constant string
inside a module?
Seems horribly fragile.
David
> +
> /**
> * kmemdup - duplicate region of memory
> *
> --
> 2.35.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, May 06, 2022 at 07:03:09PM +0530, Jagdish Gediya wrote:
> Setting name as per the format is not only useful for kobjects.
> It can also be used to set name for other things for e.g. setting
> the name of the struct attribute when multiple same kind of attributes
> need to be created with some identifier in name, instead of managing
> memory for names at such places case by case, it would be good if
> something like current kobject_set_name_vargs() can be utilized.
>
> Refactor kobject_set_name_vargs(), Create a new generic function
> set_name_vargs() which can be used for kobjects as well as at
> other places.
>
> This patch doesn't introduce any functionality change.
>
> Signed-off-by: Jagdish Gediya <[email protected]>
> ---
> include/linux/string.h | 1 +
> lib/kobject.c | 30 +-----------------------------
> mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..f329962e5ae9 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -9,6 +9,7 @@
> #include <linux/stdarg.h>
> #include <uapi/linux/string.h>
>
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> extern char *strndup_user(const char __user *, long);
> extern void *memdup_user(const void __user *, size_t);
> extern void *vmemdup_user(const void __user *, size_t);
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 5f0e71ab292c..870d05971e3a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> va_list vargs)
> {
> - const char *s;
> -
> - if (kobj->name && !fmt)
> - return 0;
> -
> - s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> - if (!s)
> - return -ENOMEM;
> -
> - /*
> - * ewww... some of these buggers have '/' in the name ... If
> - * that's the case, we need to make sure we have an actual
> - * allocated copy to modify, since kvasprintf_const may have
> - * returned something from .rodata.
> - */
> - if (strchr(s, '/')) {
> - char *t;
> -
> - t = kstrdup(s, GFP_KERNEL);
> - kfree_const(s);
> - if (!t)
> - return -ENOMEM;
> - strreplace(t, '/', '!');
> - s = t;
> - }
> - kfree_const(kobj->name);
> - kobj->name = s;
> -
> - return 0;
> + return set_name_vargs(&kobj->name, fmt, vargs);
> }
>
> /**
> diff --git a/mm/util.c b/mm/util.c
> index 54e5e761a9a9..808d29b17ea7 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> }
> EXPORT_SYMBOL(kstrndup);
>
> +/**
> + * set_name_vargs() - Set the name as per format
> + * @name: pointer to point to the name as per format
> + * @fmt: format string used to build the name
> + * @vargs: vargs to format the string.
> + */
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
Why is this a mm/ thing and not a lib/ thing?
And who else will be needing to use this? Why the churn for no
actual users?
> +{
> + const char *s;
> +
> + if (*name && !fmt)
> + return 0;
> +
> + s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> + if (!s)
> + return -ENOMEM;
> +
> + /*
> + * ewww... some of these buggers have '/' in the name ... If
> + * that's the case, we need to make sure we have an actual
> + * allocated copy to modify, since kvasprintf_const may have
> + * returned something from .rodata.
> + */
> + if (strchr(s, '/')) {
> + char *t;
> +
> + t = kstrdup(s, GFP_KERNEL);
> + kfree_const(s);
> + if (!t)
> + return -ENOMEM;
> + strreplace(t, '/', '!');
> + s = t;
> + }
> + kfree_const(*name);
> + *name = s;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(set_name_vargs);
No need to export this as there are no users in modules.
And if there was, shouldn't it be EXPORT_SYMBOL_GPL() as that's what the
kobject functions are exported as (most of them at least.)
But again, why is this needed at all?
thanks,
greg k-h