Re: rtc: fix chardev initialization races

[Uwe Kleine-König]

[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? (That's the motivation for me to discuss this old topic.)

> IOW if it exists, it's guaranteed to not go away on a probe error handling 
> path (and the rest is protected via ops_lock mutex).

Putting it in other words (assuming I got you right):

If there is a race, it needs fixing in the cdev code. The rtc driver can
stay as is.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |