Re: [PATCH v2 1/2] watchdog: core: call device_destroy before watchdog_dev_unregister

[Guenter Roeck <linux@roeck-us.net>]

Hi Damien,

On 11/25/2015 09:09 AM, Damien Riegel wrote:
> On Tue, Nov 24, 2015 at 06:20:11PM -0800, Guenter Roeck wrote:
>> On 11/24/2015 03:45 PM, Damien Riegel wrote:
>>> device_create is called after watchdog_dev_register, so it makes more
>>> sense to call the cleanup functions in reverse order, ie. device_destroy
>>> before watchdog_dev_unregister.
>>>
>>> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>
> On second thought, I am wondering if the proper fix would not be to call
> device_create before watchdog_dev_register. Consider the following
> scenario:
>
>    watchdog_register_device
>      __watchdog_register_device
> 	  watchdog_dev_register returns successfully, char dev is live
>        device_create fails, setting wdd->dev to an ERR_PTR
> 	  ...
> 	  meanwhile, a user opens the watchdog, hence ops->start is called.
> 	  If ops->start uses wdd->dev (to print a debug message for
> 	  instance), it will dereference an invalid pointer.
>
> Admittedly, it should be quite rare, but there is still a chance for a
> race condition here.
>
Only we should not have race conditions, and this might actually happen
if user space listens for a udev event on the character device, and device
creation is delayed for some reasons.

I think you are right, that is a problem. Back to the drawing board.

Ok, next question: Does it hurt to call device_create() first ?
That creates the sysfs entries for the driver.

If that doesn't work either, the only other idea I have would be to reject
an attempt to open the character device with -EAGAIN or similar if the
device node is not yet (or not anymore) available. Or maybe that would
be the correct approach anyway ? Or can we use some lock to synchronize
the two operations ?

Thanks,
Guenter