gpio: consumer: new virtual driver

From: Bartosz Golaszewski <[email protected]>

The GPIO subsystem has a serious problem with undefined behavior and use-after-free bugs on hot-unplug of GPIO chips. This can be considered a corner-case by some as most GPIO controllers are enabled early in the boot process and live until the system goes down but most GPIO drivers do allow unbind over sysfs, many are loadable modules that can be (force) unloaded and there are also GPIO devices that can be dynamically detached, for instance CP2112 which is a USB GPIO expender.

Bugs can be triggered both from user-space as well as by in-kernel users. We have the means of testing it from user-space via the character device but the issues manifest themselves differently in the kernel.

This is a proposition of adding a new virtual driver - a configurable GPIO consumer that can be configured over configfs (similarly to gpio-sim).

The configfs interface allows users to create dynamic GPIO lookup tables that are registered with the GPIO subsystem. Every config group represents a consumer device. Every sub-group represents a single GPIO lookup. The device can work in three modes: just keeping the line active, toggling it every second or requesting its interrupt and reporting edges. Every lookup allows to specify the key, offset and flags as per the lookup struct defined in linux/gpio/machine.h.

The module together with gpio-sim allows to easily trigger kernel hot-unplug errors. A simple use-case is to create a simulated chip, setup the consumer to lookup one of its lines in 'monitor' mode, unbind the simulator, unbind the consumer and observe the fireworks in dmesg.

This driver is aimed as a helper in tackling the hot-unplug problem in GPIO as well as basis for future regression testing once the fixes are upstream.

drivers/gpio/gpio-consumer.c new file +5 -0

+	struct gpio_consumer_device *dev = lookup->parent;
+
+	guard(mutex)(&dev->lock);
+
+	return sprintf(page, "%s\n", lookup->key);

+}

drivers/gpio/gpio-consumer.c new file +3 -0

+	guard(mutex)(&dev->lock);
+
+	return lookup->flags;

(quoted diff) +2 -0

+	return sprintf(page, "%s\n", flags & GPIO_TRANSITORY ? "1" : "0");
+}

Quoted 34 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The GPIO subsystem has a serious problem with undefined behavior and use-after-free bugs on hot-unplug of GPIO chips. This can be considered a corner-case by some as most GPIO controllers are enabled early in the boot process and live until the system goes down but most GPIO drivers do allow unbind over sysfs, many are loadable modules that can be (force) unloaded and there are also GPIO devices that can be dynamically detached, for instance CP2112 which is a USB GPIO expender.

Bugs can be triggered both from user-space as well as by in-kernel users. We have the means of testing it from user-space via the character device but the issues manifest themselves differently in the kernel.

This is a proposition of adding a new virtual driver - a configurable GPIO consumer that can be configured over configfs (similarly to gpio-sim).

The configfs interface allows users to create dynamic GPIO lookup tables that are registered with the GPIO subsystem. Every config group represents a consumer device. Every sub-group represents a single GPIO lookup. The device can work in three modes: just keeping the line active, toggling it every second or requesting its interrupt and reporting edges. Every lookup allows to specify the key, offset and flags as per the lookup struct defined in linux/gpio/machine.h.

The module together with gpio-sim allows to easily trigger kernel hot-unplug errors. A simple use-case is to create a simulated chip, setup the consumer to lookup one of its lines in 'monitor' mode, unbind the simulator, unbind the consumer and observe the fireworks in dmesg.

This driver is aimed as a helper in tackling the hot-unplug problem in GPIO as well as basis for future regression testing once the fixes are upstream.

...

Quoted 5 lines

drivers/gpio/gpio-consumer.c new file +5 -0

+     struct gpio_consumer_device *dev = lookup->parent;
+
+     guard(mutex)(&dev->lock);
+
+     return sprintf(page, "%s\n", lookup->key);

...

drivers/gpio/gpio-consumer.c line 344

Quoted 11 lines

drivers/gpio/gpio-consumer.c new file +11 -0

+static ssize_t
+gpio_consumer_lookup_config_offset_show(struct config_item *item, char *page)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
+     unsigned int offset;
+
+     scoped_guard(mutex, &dev->lock)
+             offset = lookup->offset;
+
+     return sprintf(page, "%d\n", offset);

Consistently it can be simplified same way

        guard(mutex)(&dev->lock);

        return sprintf(page, "%d\n", lookup->offset);

BUT. Thinking about this more. With guard() we put sprintf() inside the lock, which is suboptimal from runtime point of view. So, I think now that all these should actually use scoped_guard() rather than guard().

Quoted 1 line

+}

...

Quoted 3 lines

drivers/gpio/gpio-consumer.c new file +3 -0

+     guard(mutex)(&dev->lock);
+
+     return lookup->flags;

...

Quoted 4 lines

drivers/gpio/gpio-consumer.c new file +4 -0

+static ssize_t
+gpio_consumer_lookup_config_transitory_show(struct config_item *item,
+                                         char *page)
+{

drivers/gpio/gpio-consumer.c line 399

Quoted 3 lines

drivers/gpio/gpio-consumer.c new file +3 -0

+     enum gpio_lookup_flags flags;
+
+     flags = gpio_consumer_lookup_get_flags(item);

This is perfectly one line < 80 characters.

Quoted 2 lines

(quoted diff) +2 -0

+     return sprintf(page, "%s\n", flags & GPIO_TRANSITORY ? "1" : "0");
+}

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Tue, Aug 15, 2023 at 08:56:50PM +0200, Bartosz Golaszewski wrote:

Quoted 5 lines

drivers/gpio/gpio-consumer.c line 304

Quoted 5 lines

drivers/gpio/gpio-consumer.c new file +5 -0

+     struct gpio_consumer_device *dev = lookup->parent;
+
+     guard(mutex)(&dev->lock);
+
+     return sprintf(page, "%s\n", lookup->key);

Quoted 16 lines

drivers/gpio/gpio-consumer.c line 344

Quoted 11 lines

drivers/gpio/gpio-consumer.c new file +11 -0

+static ssize_t
+gpio_consumer_lookup_config_offset_show(struct config_item *item, char *page)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
+     unsigned int offset;
+
+     scoped_guard(mutex, &dev->lock)
+             offset = lookup->offset;
+
+     return sprintf(page, "%d\n", offset);

Consistently it can be simplified same way

        guard(mutex)(&dev->lock);

Quoted 6 lines

        return sprintf(page, "%d\n", lookup->offset);

BUT. Thinking about this more. With guard() we put sprintf() inside the lock, which is suboptimal from runtime point of view. So, I think now that all these should actually use scoped_guard() rather than guard().

Precisely why I used a scoped guard here. Same elsewhere.

Quoted 1 line

Quoted 1 line

+}

Quoted 5 lines

drivers/gpio/gpio-consumer.c line 399

Quoted 3 lines

drivers/gpio/gpio-consumer.c new file +3 -0

+     enum gpio_lookup_flags flags;
+
+     flags = gpio_consumer_lookup_get_flags(item);

This is perfectly one line < 80 characters.

There's nothing wrong with setting the variable on another line though.

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Tue, Aug 15, 2023 at 08:56:50PM +0200, Bartosz Golaszewski wrote:

...

Quoted 5 lines

Quoted 5 lines

drivers/gpio/gpio-consumer.c line 304

Quoted 5 lines

drivers/gpio/gpio-consumer.c new file +5 -0

+     struct gpio_consumer_device *dev = lookup->parent;
+
+     guard(mutex)(&dev->lock);
+
+     return sprintf(page, "%s\n", lookup->key);

(1)

...

Quoted 18 lines

Quoted 18 lines

drivers/gpio/gpio-consumer.c line 344

Quoted 11 lines

drivers/gpio/gpio-consumer.c new file +11 -0

+static ssize_t
+gpio_consumer_lookup_config_offset_show(struct config_item *item, char *page)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
+     unsigned int offset;
+
+     scoped_guard(mutex, &dev->lock)
+             offset = lookup->offset;
+
+     return sprintf(page, "%d\n", offset);

Consistently it can be simplified same way

        guard(mutex)(&dev->lock);

        return sprintf(page, "%d\n", lookup->offset);

Quoted 6 lines

Quoted 4 lines

BUT. Thinking about this more. With guard() we put sprintf() inside the lock, which is suboptimal from runtime point of view. So, I think now that all these should actually use scoped_guard() rather than guard().

Precisely why I used a scoped guard here. Same elsewhere.

So the 1) has to be amended then.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+}

...

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

drivers/gpio/gpio-consumer.c new file +3 -0

+     enum gpio_lookup_flags flags;
+
+     flags = gpio_consumer_lookup_get_flags(item);

Quoted 3 lines

Quoted 1 line

This is perfectly one line < 80 characters.

There's nothing wrong with setting the variable on another line though.

Why do we need 3 LoCs instead of a single one? Do you increase your line statistics? :-) I really would like to know the rationale behind this.

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Tue, Aug 15, 2023 at 08:56:50PM +0200, Bartosz Golaszewski wrote:

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpio/gpio-consumer.c line 304

Quoted 5 lines

drivers/gpio/gpio-consumer.c new file +5 -0

+     struct gpio_consumer_device *dev = lookup->parent;
+
+     guard(mutex)(&dev->lock);
+
+     return sprintf(page, "%s\n", lookup->key);

(1)

Quoted 1 line

So the 1) has to be amended then.

No! lookup->key is a string stored in the lookup struct BUT protected by the consumer device's lock (it must be or else the lookup could get removed while it's being modified). Unless you want to duplicate the string in order to release the mutex earlier, it has to be locked until sprintf() returns.