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 +3 -0

+	gpiod_set_value_cansleep(timer_data->desc, timer_data->val);
+	mod_timer(&timer_data->timer, jiffies + msecs_to_jiffies(1000));
+}

(quoted diff) +1 -0

+		return -EINVAL;

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

+		return ERR_PTR(dev->id);
+	}
+
+	config_group_init_type_name(&dev->group, name,
+				    &gpio_consumer_device_config_group_type);
+	mutex_init(&dev->lock);
+	INIT_LIST_HEAD(&dev->lookup_list);
+	dev->bus_notifier.notifier_call = gpio_consumer_bus_notifier_call;
+	dev->function = GPIO_CONSUMER_FUNCTION_ACTIVE;
+	init_completion(&dev->probe_completion);
+
+	return &dev->group;
+}

Quoted 26 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.

Quoted 8 lines

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.

I'll read documentation later. Some code comments below.

...

Quoted 3 lines

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

+static void gpio_consumer_on_timer(struct timer_list *timer)
+{
+     struct gpio_consumer_timer_data *timer_data = to_timer_data(timer);

Quoted 1 line

+ timer_data->val = timer_data->val == 0 ? 1 : 0;

Can be

        timer_data->val = timer_data->val ? 0 : 1;

But again, why not

        timer_data->val ^= 1;

?

Quoted 3 lines

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

+     gpiod_set_value_cansleep(timer_data->desc, timer_data->val);
+     mod_timer(&timer_data->timer, jiffies + msecs_to_jiffies(1000));
+}

...

Quoted 3 lines

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

+     key = kstrndup(page, count, GFP_KERNEL);
+     if (!key)
+             return -ENOMEM;

drivers/gpio/gpio-consumer.c line 322

Quoted 2 lines

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

+     stripped = strstrip(key);
+     memmove(key, stripped, strlen(stripped) + 1);

This can be avoided by

        key = kstrndup(skip_spaces(page), count, GFP_KERNEL);

no?

...

drivers/gpio/gpio-consumer.c line 368

Quoted 5 lines

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

+     ret = kstrtoint(page, 0, &offset);
+     if (ret)
+             return ret;
+
+     /* Use -1 to indicate lookup by name. */

This comment is unclear as offset can be -1 given by the user. What does above mean in that context?

Quoted 1 line

+ if (offset > (U16_MAX - 1))

And how does it related to this -1 if related at all?

Quoted 1 line

+ return -EINVAL;

...

drivers/gpio/gpio-consumer.c line 932

Quoted 13 lines

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

+static struct config_group *
+gpio_consumer_config_make_device_group(struct config_group *group,
+                                    const char *name)
+{
+     struct gpio_consumer_device *dev;
+
+     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+     if (!dev)
+             return ERR_PTR(-ENOMEM);
+
+     dev->id = ida_alloc(&gpio_consumer_ida, GFP_KERNEL);
+     if (dev->id < 0) {
+             kfree(dev);

Wondering if you can utilize cleanup.h.

drivers/gpio/gpio-consumer.c line 945

Quoted 13 lines

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

+             return ERR_PTR(dev->id);
+     }
+
+     config_group_init_type_name(&dev->group, name,
+                                 &gpio_consumer_device_config_group_type);
+     mutex_init(&dev->lock);
+     INIT_LIST_HEAD(&dev->lookup_list);
+     dev->bus_notifier.notifier_call = gpio_consumer_bus_notifier_call;
+     dev->function = GPIO_CONSUMER_FUNCTION_ACTIVE;
+     init_completion(&dev->probe_completion);
+
+     return &dev->group;
+}

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Tue, Aug 08, 2023 at 04:56:05PM +0200, Bartosz Golaszewski wrote:

Quoted 3 lines

But again, why not

        timer_data->val ^= 1;

This is not ok in my book. If I need to think for more than a second about what it does, then it's worse. I put clarity over brevity.

Quoted 6 lines

This can be avoided by

        key = kstrndup(skip_spaces(page), count, GFP_KERNEL);

no?

No, because we also want to remove the trailing spaces and newlines. But if you have a different suggestion with existing helpers, let me know. I didn't find any.

Quoted 8 lines

drivers/gpio/gpio-consumer.c line 368

Quoted 5 lines

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

+     ret = kstrtoint(page, 0, &offset);
+     if (ret)
+             return ret;
+
+     /* Use -1 to indicate lookup by name. */

This comment is unclear as offset can be -1 given by the user. What does above mean in that context?

I added this to the documentation. Negative number means: lookup by line name, positive or zero - lookup offset in chip.

Quoted 3 lines

Quoted 1 line

+ if (offset > (U16_MAX - 1))

And how does it related to this -1 if related at all?

GPIOLIB interprets U16_MAX as "lookup by line name". So we can allow max (U16_MAX - 1). I will add a comment.

Quoted 1 line

Quoted 1 line

+ return -EINVAL;

Quoted 11 lines

drivers/gpio/gpio-consumer.c line 932

Quoted 9 lines

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

+     struct gpio_consumer_device *dev;
+
+     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+     if (!dev)
+             return ERR_PTR(-ENOMEM);
+
+     dev->id = ida_alloc(&gpio_consumer_ida, GFP_KERNEL);
+     if (dev->id < 0) {
+             kfree(dev);

Wondering if you can utilize cleanup.h.

Whooaah! In february this year I suggested basic C RAII during my talk at fosdem and here we are? I missed this one. Yeah, I will use it! Even better, I will abuse the cr*p out of it in gpio-sim as well! Thanks for bringing this to my attention. This may be the best thing that happened to kernel C code in years if people widely adopt it. (This paragraph was written by a fan of GLib's autopointer paradigm. :) )

Quoted 8 lines

Quoted 7 lines

This can be avoided by

        key = kstrndup(skip_spaces(page), count, GFP_KERNEL);

no?

Quoted 3 lines

No, because we also want to remove the trailing spaces and newlines. But if you have a different suggestion with existing helpers, let me know. I didn't find any.

kstrto*() are newline friendly. The rest as you noted can be covered with sysfs_streq() / sysfs_match_string().

On Thu, Aug 10, 2023 at 4:53 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

This can be avoided by

        key = kstrndup(skip_spaces(page), count, GFP_KERNEL);

no?

Quoted 6 lines

Quoted 3 lines

No, because we also want to remove the trailing spaces and newlines. But if you have a different suggestion with existing helpers, let me know. I didn't find any.

kstrto*() are newline friendly. The rest as you noted can be covered with sysfs_streq() / sysfs_match_string().

It's a kstrndup() not a kstrtosomething(). It's not newline friendly.