Re: [PATCH v2] v4l: async: Protect against double notifier registrations

[Kieran Bingham]

Hi Geert,

Thanks for the review

On 17/01/18 08:00, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Wed, Jan 17, 2018 at 12:47 AM, Kieran Bingham
> <kieran.bing...@ideasonboard.com> 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, and use a WARN_ON to identify the fault.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> 
> Thanks for your patch!
> 
> However, I have several comments:
>   1. Instead of walking notifier_list (O(n)), can't you just check if
>      notifier.list is part of a list or not (O(1))?

Not safely as far as I can see: (unless you know better)

Looks like I'd have to at least check something like the following:
  notifier->next != LIST_POISON1 && notifier->next != NULL &&
  notifier->prev != LIST_POISON2 && notifier->prev != NULL &&
  notifier->next != notifier->prev

Although - that doesn't count the possibility that the struct list_head in the
object being added is essentially un-initialised before being added to the list
- so it could technically contain any value ...

(Looking forward to being told I'm completely missing something obvious here...)


>   2. Isn't notifier usually (always?) allocated dynamically, so if will be a
>      different pointer after a previous -EPROBE_DEFER anyway?

Nope. The notifier can be part of the device context structure to reduce
allocations.



>   3. If you enable CONFIG_DEBUG_LIST, it should scream, too.

Aha - maybe that was my missing link. -E_NOT_ENOUGH_DEBUG_ENABLED.

Although I've just looked through the code that checks against a double entry.
It may have helped me find my bug in fact, but I think that would only fire if
the entry tried to add twice consecutively, which certainly wouldn't be
guaranteed if a driver returned with -EPROBE_DEFER.

So - I've just tested that if you have A B C and HEAD,

list_add(A, HEAD);
list_add(A, HEAD);
  // would fire in __list_add_valid as (new == prev || new == next)

However,

list_add(A, HEAD);
list_add(B, HEAD);
list_add(C, HEAD);
list_add(B, HEAD);

Will not catch this double-add-B in __list_add_valid(), and will generate an
infinitely looping list if you try to then walk it with list_for_each_entry()

(As demonstrated by the attached list-test.c module which I used to test this)

Oh what fun :D

--
Kieran


> 
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -374,17 +374,26 @@ 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;
>>
>>         if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>>                 return -EINVAL;
>>
>> +       mutex_lock(&list_lock);
>> +
>> +       /* Avoid re-registering a notifier. */
>> +       list_for_each_entry(n, &notifier_list, list) {
>> +               if (WARN_ON(n == notifier)) {
>> +                       ret = -EEXIST;
>> +                       goto err_unlock;
>> +               }
>> +       }
>> +
>>         INIT_LIST_HEAD(&notifier->waiting);
>>         INIT_LIST_HEAD(&notifier->done);
>>
>> -       mutex_lock(&list_lock);
>> -
>>         for (i = 0; i < notifier->num_subdevs; i++) {
>>                 asd = notifier->subdevs[i];
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
>                                 -- Linus Torvalds
> 


#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/list.h>


struct item {
	struct list_head list;
	int i;
};

struct item items[5];

int __init helloworld_init(void)
{
	int a;
	struct list_head head;
	struct item * ob;
	bool catch_double_add = 1;

	INIT_LIST_HEAD(&head);

	printk("Hello World !\n");

	for (a = 0; a < 5; a++) {
		items[a].i = a;
		list_add(&items[a].list, &head);
	}

	for (a = 0; a < 5; a++)
		printk("Item[%d] = %d\n", a, items[a].i);

	list_for_each_entry(ob, &head, list) {
		printk("ob = %d\n", ob->i);
	}

	if (catch_double_add)
		list_add(&items[4].list, &head);
	else
		list_add(&items[2].list, &head);

	list_for_each_entry(ob, &head, list) {
		printk("ob = %d\n", ob->i);
	}

	return 0;
}

void __exit helloworld_exit(void)
{
	pr_info("Goodbye cruel world...\n");
}

module_init(helloworld_init);
module_exit(helloworld_exit);
MODULE_LICENSE("GPL");