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

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

[Damien Riegel]

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>
---
 drivers/watchdog/watchdog_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 551af04..e70b6e8 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -264,8 +264,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		if (ret) {
 			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
 				ret);
-			watchdog_dev_unregister(wdd);
 			device_destroy(watchdog_class, devno);
+			watchdog_dev_unregister(wdd);
 			ida_simple_remove(&watchdog_ida, wdd->id);
 			wdd->dev = NULL;
 			return ret;
@@ -324,10 +324,10 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
 		unregister_reboot_notifier(&wdd->reboot_nb);
 
 	devno = wdd->cdev.dev;
+	device_destroy(watchdog_class, devno);
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
 		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
-	device_destroy(watchdog_class, devno);
 	ida_simple_remove(&watchdog_ida, wdd->id);
 	wdd->dev = NULL;
 }
-- 
2.5.0

[PATCH v2 2/2] watchdog: core: factorize register error paths

[Damien Riegel]

The commit adding the reboot notifier in the core introduced a new error
path in __watchdog_register_device, making error paths quite redundant.

This commit factorizes all error paths that do some cleanup.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 drivers/watchdog/watchdog_core.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index e70b6e8..3d17c1c1 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		wdd->id = id;
 
 		ret = watchdog_dev_register(wdd);
-		if (ret) {
-			ida_simple_remove(&watchdog_ida, id);
-			return ret;
-		}
+		if (ret)
+			goto remove_ida;
 	}
 
 	devno = wdd->cdev.dev;
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
 					wdd, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
-		watchdog_dev_unregister(wdd);
-		ida_simple_remove(&watchdog_ida, id);
 		ret = PTR_ERR(wdd->dev);
-		return ret;
+		goto unregister_device;
 	}
 
 	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
@@ -264,11 +260,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		if (ret) {
 			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
 				ret);
-			device_destroy(watchdog_class, devno);
-			watchdog_dev_unregister(wdd);
-			ida_simple_remove(&watchdog_ida, wdd->id);
-			wdd->dev = NULL;
-			return ret;
+			goto destroy_device;
 		}
 	}
 
@@ -282,6 +274,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 	}
 
 	return 0;
+
+destroy_device:
+	device_destroy(watchdog_class, devno);
+unregister_device:
+	watchdog_dev_unregister(wdd);
+	wdd->dev = NULL;
+remove_ida:
+	ida_simple_remove(&watchdog_ida, id);
+
+	return ret;
 }
 
 /**
-- 
2.5.0

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

[Guenter Roeck]

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>

> ---
>   drivers/watchdog/watchdog_core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 551af04..e70b6e8 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -264,8 +264,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>   		if (ret) {
>   			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
>   				ret);
> -			watchdog_dev_unregister(wdd);
>   			device_destroy(watchdog_class, devno);
> +			watchdog_dev_unregister(wdd);
>   			ida_simple_remove(&watchdog_ida, wdd->id);
>   			wdd->dev = NULL;
>   			return ret;
> @@ -324,10 +324,10 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>   		unregister_reboot_notifier(&wdd->reboot_nb);
>
>   	devno = wdd->cdev.dev;
> +	device_destroy(watchdog_class, devno);
>   	ret = watchdog_dev_unregister(wdd);
>   	if (ret)
>   		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> -	device_destroy(watchdog_class, devno);
>   	ida_simple_remove(&watchdog_ida, wdd->id);
>   	wdd->dev = NULL;
>   }
>

Re: [PATCH v2 2/2] watchdog: core: factorize register error paths

[Guenter Roeck]

On 11/24/2015 03:45 PM, Damien Riegel wrote:
> The commit adding the reboot notifier in the core introduced a new error
> path in __watchdog_register_device, making error paths quite redundant.
>
> This commit factorizes all error paths that do some cleanup.
>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/watchdog_core.c | 26 ++++++++++++++------------
>   1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index e70b6e8..3d17c1c1 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>   		wdd->id = id;
>
>   		ret = watchdog_dev_register(wdd);
> -		if (ret) {
> -			ida_simple_remove(&watchdog_ida, id);
> -			return ret;
> -		}
> +		if (ret)
> +			goto remove_ida;
>   	}
>
>   	devno = wdd->cdev.dev;
>   	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
>   					wdd, "watchdog%d", wdd->id);
>   	if (IS_ERR(wdd->dev)) {
> -		watchdog_dev_unregister(wdd);
> -		ida_simple_remove(&watchdog_ida, id);
>   		ret = PTR_ERR(wdd->dev);
> -		return ret;
> +		goto unregister_device;
>   	}
>
>   	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> @@ -264,11 +260,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>   		if (ret) {
>   			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
>   				ret);
> -			device_destroy(watchdog_class, devno);
> -			watchdog_dev_unregister(wdd);
> -			ida_simple_remove(&watchdog_ida, wdd->id);
> -			wdd->dev = NULL;
> -			return ret;
> +			goto destroy_device;
>   		}
>   	}
>
> @@ -282,6 +274,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>   	}
>
>   	return 0;
> +
> +destroy_device:
> +	device_destroy(watchdog_class, devno);
> +unregister_device:
> +	watchdog_dev_unregister(wdd);
> +	wdd->dev = NULL;
> +remove_ida:
> +	ida_simple_remove(&watchdog_ida, id);
> +
> +	return ret;
>   }
>
>   /**
>

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

[Damien Riegel]

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.

Damien

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

[Guenter Roeck]

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

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

[Damien Riegel]

On Wed, Nov 25, 2015 at 09:57:20AM -0800, Guenter Roeck wrote:
> 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.

I can't think of a case where it would be an issue to call
device_create() first. After all, watchdog_dev_register just creates
entries in /dev, so it makes sense to create them only when the watchdog
is fully ready. I will send patches tomorrow.

> 
> 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
>