Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 6 May 2022 22:54:10 +0530
From:   Jagdish Gediya <jvgediya@linux.ibm.com>
To:     David Laight <David.Laight@aculab.com>
Cc:     "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "rafael@kernel.org" <rafael@kernel.org>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "geert@linux-m68k.org" <geert@linux-m68k.org>,
        "linux@roeck-us.net" <linux@roeck-us.net>,
        "adobriyan@gmail.com" <adobriyan@gmail.com>
Subject: Re: [PATCH] kobject: Refactor kobject_set_name_vargs()
Message-ID: <YnVZujpw0RZy3eVT@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
References: <20220506133309.36794-1-jvgediya@linux.ibm.com>
 <5902e26ef400451b966be2dd0fbd1575@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5902e26ef400451b966be2dd0fbd1575@AcuMS.aculab.com>
Precedence: bulk

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 <jvgediya@linux.ibm.com>
> > ---
> >  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)
> 
>