gpio: consumer: new virtual driver

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));
+}

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

+	scoped_guard(mutex, &dev->lock)
+		ret = sprintf(page, "%s\n", lookup->key);
+
+	return ret;
+}

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

+	guard(mutex)(&dev->lock);
+
+	if (gpio_consumer_device_is_live_unlocked(dev))
+		return -EBUSY;
+
+	kfree(lookup->key);
+	lookup->key = no_free_ptr(key);
+
+	return count;
+}

+}

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

+static ssize_t
+gpio_consumer_device_config_live_store(struct config_item *item,
+				       const char *page, size_t count)
+{
+	struct gpio_consumer_device *dev = to_gpio_consumer_device(item);
+	bool live;

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

+		gpio_consumer_device_deactivate_unlocked(dev);
+
+	return ret ?: count;
+}

Quoted 33 lines · Bartosz Golaszewski

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 line 94

Quoted 5 lines

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

+static void gpio_consumer_on_timer(struct timer_list *timer)
+{
+     struct gpio_consumer_timer_data *timer_data = to_timer_data(timer);
+
+     timer_data->val = timer_data->val ? 1 : 0;

I guess it should be 0 : 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));
+}

...

drivers/gpio/gpio-consumer.c line 301

Quoted 7 lines

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

+static ssize_t
+gpio_consumer_lookup_config_key_show(struct config_item *item, char *page)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
 
+     int ret;

Why is it needed now? Seems you were too fast to send v3, look at my comments in v2 thread.

Quoted 5 lines

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

+     scoped_guard(mutex, &dev->lock)
+             ret = sprintf(page, "%s\n", lookup->key);
+
+     return ret;
+}

...

drivers/gpio/gpio-consumer.c line 318

Quoted 10 lines

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

+static ssize_t
+gpio_consumer_lookup_config_key_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;
+     char *key __free(kfree) = NULL;
+     char *stripped;
+
+     key = kstrndup(page, count, GFP_KERNEL);

skip_spaces() will allow you to get rid of memmove().

drivers/gpio/gpio-consumer.c line 323

Quoted 5 lines

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

+     if (!key)
+             return -ENOMEM;
+
+     stripped = strstrip(key);
+     memmove(key, stripped, strlen(stripped) + 1);

And this become something like

        /* Get rid of trailing newline and spaces */
        strim(key);

Quoted 10 lines

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

+     guard(mutex)(&dev->lock);
+
+     if (gpio_consumer_device_is_live_unlocked(dev))
+             return -EBUSY;
+
+     kfree(lookup->key);
+     lookup->key = no_free_ptr(key);
+
+     return count;
+}

...

drivers/gpio/gpio-consumer.c line 396

Quoted 12 lines

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

+static enum gpio_lookup_flags
+gpio_consumer_lookup_get_flags(struct config_item *item)
+{
+     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
+     struct gpio_consumer_device *dev = lookup->parent;
+     enum gpio_lookup_flags flags;
+
+     scoped_guard(mutex, &dev->lock)
 
+             flags = lookup->flags;
+
+     return flags;

        guard()
        return lookup->flags;

?

Quoted 1 line

+}

...

Quoted 6 lines

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

+static ssize_t
+gpio_consumer_device_config_live_store(struct config_item *item,
+                                    const char *page, size_t count)
+{
+     struct gpio_consumer_device *dev = to_gpio_consumer_device(item);
+     bool live;

drivers/gpio/gpio-consumer.c line 743

Quoted 10 lines

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

+     int ret;
+
+     ret = kstrtobool(page, &live);
+     if (ret)
+             return ret;
+
+     guard(mutex)(&dev->lock);
+
+     if (live == gpio_consumer_device_is_live_unlocked(dev))
+             ret = -EPERM;

                return ... ?

Quoted 1 line

+ else if (live)

        if () ?

drivers/gpio/gpio-consumer.c line 746

Quoted 3 lines

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

+             ret = gpio_consumer_device_activate_unlocked(dev);
 
+     else

drop it ?

drivers/gpio/gpio-consumer.c line 750

Quoted 5 lines

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

+             gpio_consumer_device_deactivate_unlocked(dev);
+
+     return ret ?: count;
+}

--
With Best Regards,
Andy Shevchenko