[Letux-kernel] omap-gpmc gta04 regression 4.7-rc1

[Grygorii Strashko]

On 06/07/2016 07:22 PM, Linus Walleij wrote:
> On Tue, Jun 7, 2016 at 5:29 PM, Grygorii Strashko
> <grygorii.strashko at ti.com> wrote:
>> On 06/07/2016 04:33 PM, Linus Walleij wrote:
>>> On Tue, Jun 7, 2016 at 9:50 AM, Roger Quadros <rogerq at ti.com> wrote:
>>>
>>>> Linus, any idea why of_gpiochip_find_and_xlate() would do a NULL pointer dereference?
>>>
>>> Nope. I see this uses the array fetch function which I did patch around but
>>> "should" not affect this.
>>>
>>>> We have a case here where the GPMC driver registers a gpiochip but after a while
>>>> unregisters it due to some other orthogonal resource not being available.
>>>> Is this registering and unregistering considered acceptable from gpiochip point of view?
>>>
>>> That would be if it doesn't really go away aftert gpiochip_remove() because
>>> some refcount doesn't go zero, so double-check that. It should be fine
>>> anyway though ... just the instance floating around somewhere.
>>>
>>
>> Linus, I've tried to find place in gpiolib.c where gpiochip is removed
>> from gpio_devices list as part of gpiochip_remove(), but I can't. Am I missed smth.?
> 
> It is supposed to happen in  gpiodevice_release() as an effect of the
> references to the kobject in the device going to zero when the last
> put_device() is called in gpiochip_remove().
> 
> Unless there is already some other user that has taken a descriptor,
> such as a kernel driver or userspace.

Ah. Thanks for the info.

> 
>> If that is the case then such kind of crash is possible, because gpiochip_find()
>> uses this list.
> 
> Hm I suspect it could be that the reference count is off, so the device
> isn't removed from the list?
> 
> But we should still numb the device somehow so that the crash doesn't
> happen even if the device is still in the list.

Seems there is a window for the race in
void gpiochip_remove(struct gpio_chip *chip)
{
	struct gpio_device *gdev = chip->gpiodev;
	struct gpio_desc *desc;
	unsigned long	flags;
	unsigned	i;
	bool		requested = false;

	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
	gpiochip_sysfs_unregister(gdev);
	/* Numb the device, cancelling all outstanding operations */
	gdev->chip = NULL;
^^^^^ Once we've reached this point the gpiochip_find() will start crashing :(

	struct gpio_chip *gpiochip_find(void *data,
	{
		[...]
		spin_lock_irqsave(&gpio_lock, flags);
		list_for_each_entry(gdev, &gpio_devices, list)
			if (match(gdev->chip, data))
					^^^^ chip is null, match = of_gpiochip_find_and_xlate()
				break;



	gpiochip_irqchip_remove(chip);
	acpi_gpiochip_remove(chip);
	gpiochip_remove_pin_ranges(chip);
	gpiochip_free_hogs(chip);
	[...]

	/*
	 * The gpiochip side puts its use of the device to rest here:
	 * if there are no userspace clients, the chardev and device will
	 * be removed, else it will be dangling until the last user is
	 * gone.
	 */
	put_device(&gdev->dev);
^^^ if i understand right gpiodev/chip will be in gpio_devices list till this point
    [if not used]




-- 
regards,
-grygorii