Re: [PATCH] v4l: async: Protect against double notifier regstrations

[Sakari Ailus]

Hi Kieran,

On Tue, Jan 16, 2018 at 02:52:58PM +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> 
> It can be easy to attempt to register the same notifier twice
> in mis-handled error cases such as working with -EPROBE_DEFER.
> 
> This results in odd kernel crashes where the notifier_list becomes
> corrupted due to adding the same entry twice.
> 
> Protect against this so that a developer has some sense of the pending
> failure.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 2b08d03b251d..e8476f0755ca 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -374,6 +374,7 @@ static int __v4l2_async_notifier_register(struct 
> v4l2_async_notifier *notifier)
>       struct device *dev =
>               notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
>       struct v4l2_async_subdev *asd;
> +     struct v4l2_async_notifier *n;
>       int ret;
>       int i;
>  
> @@ -385,6 +386,19 @@ static int __v4l2_async_notifier_register(struct 
> v4l2_async_notifier *notifier)
>  
>       mutex_lock(&list_lock);
>  
> +     /*
> +      * Registering the same notifier can occur if a driver incorrectly
> +      * handles a -EPROBE_DEFER for example, and will break in a
> +      * confusing fashion with linked-list corruption.
> +      */

This would seem fine in the commit message, and it's essentially there
already. How about simply:

        /* Avoid re-registering a notifier. */
        
You should actually perform the check before initialising the notifier's
lists. Although things are likely in a quite bad shape already if this
happens.

> +     list_for_each_entry(n, &notifier_list, list) {
> +             if (n == notifier) {

if (WARN_ON(n == notifier)) {

And drop the error message below?

> +                     dev_err(dev, "Notifier has already been registered\n");
> +                     ret = -EEXIST;
> +                     goto err_unlock;
> +             }
> +     }
> +
>       for (i = 0; i < notifier->num_subdevs; i++) {
>               asd = notifier->subdevs[i];
>  

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi