Re: [PATCH] params: serialize lookup_or_create_module_kobject()

Thanks for your review! I'll send a v2 patch~

> > lookup_or_create_module_kobject() first looks up the module kobject with
> > kset_find_obj() and, if not found, creates a new one with
> > kobject_init_and_add().  The function is called at runtime from
> > module_add_driver() since commit f95bbfe18512 ("drivers: base: handle
> > module_kobject creation"), which means two concurrent driver
> > registrations for the same built-in module name can both miss the
> > lookup and race to create the same kobject.
> > 
> > The loser of the race gets -EEXIST from kobject_init_and_add() and its
> > kobject is removed from module_kset by kobject_add_internal() before
> > the failure is reported.  The error path then calls kobject_put(),
> > which invokes module_kobj_release(), but that only completes
> > ->kobj_completion and never frees the dynamically allocated
> > module_kobject, leaking it (96 bytes) along with the object having been
> > detached from the kset.
> > 
> > This is triggerable by unprivileged users, e.g. by concurrently issuing
> > the RAW_IOCTL_INIT ioctl of the raw-gadget driver, which registers the
> > "raw_gadget" driver on the gadget bus:
> > 
> >   sysfs: cannot create duplicate filename '/module/raw_gadget'
> >   ...
> >   Adding module 'raw_gadget' to sysfs failed (-17), the system may be
> >   unstable.
> >   ...
> >   unreferenced object 0xffff8880188dacc0 (size 96):
> >     backtrace:
> >       lookup_or_create_module_kobject+0x47/0x100
> >       module_add_driver+0x73/0x1b0
> >       bus_add_driver+0x1d9/0x340
> >       driver_register+0xde/0x170
> > 
> > Fix it by serializing the lookup and the creation with a mutex, so that
> > the second caller finds the kobject created by the first one instead of
> > racing with it.
> > 
> > Fixes: f95bbfe18512 ("drivers: base: handle module_kobject creation")
> > Fixes: 7c76c813cfc4 ("kernel: globalize lookup_or_create_module_kobject()")
> > Signed-off-by: Jiakai Xu <[email protected]>
> > ---
> >  kernel/params.c | 29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> Did you forget an Assisted-by: tag?
> 
> > diff --git a/kernel/params.c b/kernel/params.c
> > index 8b25133fed242..78f00d3f6a165 100644
> > --- a/kernel/params.c
> > +++ b/kernel/params.c
> > @@ -20,6 +20,11 @@
> >  /* Protects all built-in parameters, modules use their own param_lock */
> >  static DEFINE_MUTEX(param_lock);
> >  
> > +/* Serializes module kobject lookup and creation in
> > + * lookup_or_create_module_kobject()
> > + */
> 
> Wrong coding style :(
> 
> > +static DEFINE_MUTEX(mod_kobject_mutex);
> > +
> >  /* Use the module's mutex, or if built-in use the built-in mutex */
> >  #ifdef CONFIG_MODULES
> >  #define KPARAM_MUTEX(mod)	((mod) ? &(mod)->param_lock : &param_lock)
> > @@ -754,13 +759,24 @@ lookup_or_create_module_kobject(const char *name)
> >  	struct kobject *kobj;
> >  	int err;
> >  
> > +	/*
> > +	 * The lookup and the creation must be done atomically, otherwise
> > +	 * concurrent callers may race to create the same kobject, and the
> > +	 * loser of the race gets -EEXIST from kobject_init_and_add().
> > +	 */
> > +	mutex_lock(&mod_kobject_mutex);
> > +
> 
> guard()?
> 
> thanks,
> 
> greg k-h