On 22/05/2018 17:33:40+0200, Jiri Kosina wrote:
> On Tue, 22 May 2018, Uwe Kleine-König wrote:
> > [adding linux-rtc ML and Alexandre to Cc:]
> >
> > Hello,
> >
> > On Tue, May 22, 2018 at 02:09:36PM +0200, Jiri Kosina wrote:
> > > On Mon, 21 May 2018, Uwe Kleine-König wrote:
> > >
> > > > > The race looks like that (thanks Jiri):
> > > > >
> > > > > CPU0: CPU1:
> > > > > sys_load_module()
> > > > > do_init_module()
> > > > > do_one_initcall()
> > > > > cmos_do_probe()
> > > > > rtc_device_register()
> > > > > __register_chrdev()
> > > > > cdev->owner = struct module*
> > > > > open("/dev/rtc0")
> > > > > rtc_device_unregister()
> > > > > module_put()
> > > > > free_module()
> > > > > module_free(mod->module_core)
> > > > > /* struct module *module is now
> > > > > freed */
> > > > > chrdev_open()
> > > > > spin_lock(cdev_lock)
> > > > > cdev_get()
> > > > > try_module_get()
> > > > > module_is_live()
> > > > > /* dereferences already
> > > > > freed struct module* */
> > > >
> > > > [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> > > > fail after rtc_device_register successfully finished and pointed to this
> > > > reasoning as explaination.]
> > > >
> > > > If there is really such a race then (I hope) there is
> > > > something in the cdev code that needs fixing. According to my
> > > > understanding, when rtc_device_unregister returned, the cdev is gone and
> > > > so chrdev_open is supposed to fail.
> > >
> > > Oh wow, hello back to 4 years ago!
> >
> > :-)
> >
>
> > > Looking at the current code, I don't think there is no such race any more,
> > > as the last thing cmos_do_probe() -> __rtc_register_device() does that can
> > > potentially fail is the chardev creation itself.
> >
> > OK, so you agree that it's also save to do something in a driver's probe
> > after rtc_device_register() and call rtc_device_unregister() in the error
> > path, right?
>
> Hmm, not really; that's what the code apparently did 4 years ago (judging
> from the scenario in the old mail, I of course forgot all the details),
> but doesn't do it any more.
>
> Looking at the current code, if you call rtc_device_unregister() in the
> probe path, where is the guarantee that cdev_get() will not derefernce
> already freed struct module*?
>
I've also tried to produce it and couldn't. The whole magic happens in
kobj_lookup. Freeing the module before ends up with the application
opening the char dev getting -ENXIO. Freeing the module after
kobj_lookup is not possible and ends up with -EBUSY.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
