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/Makefile +0 -0

 obj-$(CONFIG_GPIO_CRYSTAL_COVE)		+= gpio-crystalcove.o
 obj-$(CONFIG_GPIO_CS5535)		+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_DA9052)		+= gpio-da9052.o

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

+static ssize_t
+gpio_consumer_lookup_config_drive_store(struct config_item *item,
+					const char *page, size_t count)
+{
+	struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+	struct gpio_consumer_device *dev = lookup->parent;
+
+	guard(mutex)(&dev->lock);
+
+	if (gpio_consumer_device_is_live_unlocked(dev))
+		return -EBUSY;
+
+	if (sysfs_streq(page, "push-pull")) {

+}

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.

...

drivers/gpio/Makefile obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o · line 49

Quoted 5 lines

drivers/gpio/Makefile +1 -0

@@ -46,6 +46,7 @@ obj-$(CONFIG_GPIO_BT8XX)            += gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CADENCE)           += gpio-cadence.o
 obj-$(CONFIG_GPIO_CLPS711X)          += gpio-clps711x.o
 obj-$(CONFIG_GPIO_SNPS_CREG)         += gpio-creg-snps.o
+obj-$(CONFIG_GPIO_CONSUMER)          += gpio-consumer.o

Order?

Quoted 3 lines

drivers/gpio/Makefile +0 -0

 obj-$(CONFIG_GPIO_CRYSTAL_COVE)              += gpio-crystalcove.o
 obj-$(CONFIG_GPIO_CS5535)            += gpio-cs5535.o
 obj-$(CONFIG_GPIO_DA9052)            += gpio-da9052.o

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+             return dev_err_probe(dev, ret,
+                                  "Failed to read GPIO line names\n");

With one line it takes 83 characters (and note, that long before checkpatch went for 100, the string literals at the end of a long line were accepted)...

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+                             return dev_err_probe(dev, ret,
+                                             "Failed to request GPIO line interrupt\n");

And here with broken indentation you got 91.
Can you be consistent?

(I prefer as you know less LoCs)

...

Quoted 13 lines

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

+static ssize_t
+gpio_consumer_lookup_config_drive_store(struct config_item *item,
+                                     const char *page, size_t count)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
+
+     guard(mutex)(&dev->lock);
+
+     if (gpio_consumer_device_is_live_unlocked(dev))
+             return -EBUSY;
+
+     if (sysfs_streq(page, "push-pull")) {

drivers/gpio/gpio-consumer.c line 430

Quoted 10 lines

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

+             lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
+     } else if (sysfs_streq(page, "open-drain")) {
+             lookup->flags &= ~GPIO_OPEN_SOURCE;
+             lookup->flags |= GPIO_OPEN_DRAIN;
+     } else if (sysfs_streq(page, "open-source")) {
+             lookup->flags &= ~GPIO_OPEN_DRAIN;
+             lookup->flags |= GPIO_OPEN_SOURCE;
+     } else {
 
+             count = -EINVAL;

Strictly speaking this is incorrect.
You need

        ssize_t ret;
        ...
        ret = count;
        if (...)
                ret = -EINVAL;

drivers/gpio/gpio-consumer.c line 459

Quoted 9 lines

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

+     }
+
+     return count;
+}
 
+static ssize_t
+gpio_consumer_lookup_config_pull_store(struct config_item *item,
+                                    const char *page, size_t count)
+{

As per above.

Quoted 1 line

+}

...

drivers/gpio/gpio-consumer.c line 609

Quoted 2 lines

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

+             curr->chip_hwnum = lookup->offset < 0 ?
+                                     U16_MAX : lookup->offset;

I found this way better

                curr->chip_hwnum =
                        lookup->offset < 0 ? U16_MAX : lookup->offset;

...

Quoted 1 line

+ return ret ?: count;

Also possible way in the above mentioned cases.

...

I'm not going to bikeshed, I believe you can fix above accordingly, either way

Reviewed-by Andy Shevchenko <[email protected]>

--
With Best Regards,
Andy Shevchenko