Re: [PATCH v7] gpio: virtuser: new virtual driver

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, May 29, 2024 at 11:00 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > The GPIO subsystem used to have 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) or described on the device-tree.
> >
> > This driver is aimed as a helper in spotting any regressions in
> > hot-unplug handling in GPIOLIB.
>
> ...
>
> > User must pass exactly the number of values that the array contains
>
> Can't we assume non-active values for the rest if less than needed were
> provided? For more than that, why do we care?
>

Honestly, what good would it do? It would just be more confusing IMO.

> ...
>
> > +#include <linux/atomic.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/completion.h>
> > +#include <linux/configfs.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/machine.h>
>
> > +#include <linux/idr.h>
>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq_work.h>
>
> > +#include <linux/kernel.h>
>
> Do you need this?

ARRAY_SIZE() used to live here when I first wrote this but it was
since moved. I'll drop this.

>
> > +#include <linux/limits.h>
> > +#include <linux/list.h>
> > +#include <linux/lockdep.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/overflow.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
>
> > +#include <linux/string.h>
>
> Implied by string_helpers.h
>

Yeah, but we still use symbols directly from string.h, we shouldn't
depend on implicit includes.

> > +#include <linux/string_helpers.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
>
> ...
>
> > +struct gpio_virtuser_line_array_data {
> > +     struct gpio_descs *descs;
> > +     struct kobject *kobj;
> > +     struct attribute_group *attr_group;
> > +};
> > +
> > +struct gpio_virtuser_line_data {
> > +     struct gpio_desc *desc;
> > +     struct kobject *kobj;
> > +     struct attribute_group *attr_group;
> > +     char consumer[GPIO_CONSUMER_NAME_MAX_LEN];
> > +     struct mutex consumer_lock;
> > +     unsigned int debounce;
> > +     atomic_t irq;
> > +     atomic_t irq_count;
> > +};
>
> Maybe
>
> struct gpio_virtuser_sysfs_data {
>         union {
>                 struct gpio_desc *desc;
>                 struct gpio_descs *descs;
>         };
>         struct kobject *kobj;
>         struct attribute_group *attr_group;
> };
>
> struct gpio_virtuser_line_array_data {
>         struct gpio_virtuser_sysfs_data sd;
> };
>
> struct gpio_virtuser_line_data {
>         struct gpio_virtuser_sysfs_data sd;
>         char consumer[GPIO_CONSUMER_NAME_MAX_LEN];
>         struct mutex consumer_lock;
>         unsigned int debounce;
>         atomic_t irq;
>         atomic_t irq_count;
> };
>
> ?
>
> ...
>
> > +struct gpio_virtuser_attr_ctx {
> > +     struct device_attribute dev_attr;
> > +     void *data;
> > +};
>
> struct dev_ext_attribute ?
>

Sounds good, I'll rework this.

> ...
>
> > +struct gpio_virtuser_attr_descr {
> > +     const char *name;
> > +     ssize_t (*show)(struct device *, struct device_attribute *, char *);
> > +     ssize_t (*store)(struct device *, struct device_attribute *,
> > +                      const char *, size_t);
> > +};
>
> struct device_attribute ? (Yes, I know that that one is a bit bigger but
> benefit is that we have some code that you may reuse)
>

Not sure what you mean here, these are callbacks for sysfs.

> ...
>
> > +static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
> > +                                                 unsigned long *values,
> > +                                                 size_t num_values)
> > +{
> > +     ssize_t len = 0;
> > +     size_t i;
> > +
> > +     for (i = 0; i < num_values; i++)
> > +             len += sysfs_emit_at(buf, len, "%d",
> > +                                  test_bit(i, values) ? 1 : 0);
> > +     return len + sysfs_emit_at(buf, len, "\n");
>
> Why not use %pb?
>

Because it outputs hex? I want to output binary, can I do it?

> > +}
>
> ...
>
> > +static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
> > +                                              unsigned long *values)
> > +{
> > +     size_t i;
> > +
> > +     for (i = 0; i < len; i++) {
>
> Perhaps
>
>                 bool val;
>                 int ret;
>
>                 ret = kstrtobool(...);

kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.

>                 if (ret)
>                         return ret;
>
>                 assign_bit(...); // btw, why atomic?
>
> > +             if (buf[i] == '0')
> > +                     clear_bit(i, values);
> > +             else if (buf[i] == '1')
> > +                     set_bit(i, values);
> > +             else
> > +                     return -EINVAL;
>
> > +     }
>
> BUT, why not bitmap_parse()?
>

Because it parses hex, not binary.

> > +     return 0;
> > +}
>
> ...
>
> > +     unsigned long *values __free(bitmap) = bitmap_alloc(descs->ndescs,
> > +                                                         GFP_KERNEL);
>
> Perhaps
>
>         unsigned long *values __free(bitmap) =
>                  bitmap_alloc(descs->ndescs, GFP_KERNEL);
>
> ...
>
> > +     unsigned long *values __free(bitmap) = bitmap_zalloc(descs->ndescs,
> > +                                                          GFP_KERNEL);
>
> In the similar way?
>
> ...
>
> > +     unsigned long *values __free(bitmap) = bitmap_zalloc(descs->ndescs,
> > +                                                          GFP_KERNEL);
>
> Ditto.
>
> ...
>
> > +{
> > +     return sysfs_emit(buf, "%s\n",
> > +                       dir == GPIO_LINE_DIRECTION_IN ? "input" : "output");
>
> I think this maybe transformed to something like str_input_output() in
> string_choices.h (and you don't even need to include that as it's implied by
> string_helpers.h)
>

These helpers take bool as argument. Hard to tell whether input or
output should correspond to true. I'd leave it as is.

> > +}
>
> ...
>
> > +static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
> > +{
> > +     if (sysfs_streq(buf, "input")) {
> > +             *dir = GPIO_LINE_DIRECTION_IN;
> > +             return 0;
> > +     }
> > +
> > +     if (sysfs_streq(buf, "output-high"))
> > +             *val = 1;
> > +     else if (sysfs_streq(buf, "output-low"))
> > +             *val = 0;
> > +     else
> > +             return -EINVAL;
> > +
> > +     *dir = GPIO_LINE_DIRECTION_OUT;
>
> This can be transformed to use sysfs_match_string() with
>
> static const char * const dirs[] = { "output-low", "output-high", "input" };
>
>         int ret;
>
>         ret = sysfs_match_string(...);
>         if (ret < 0)
>                 return ret;
>
>         *val = ret;
>         *dir = ret == 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>
> And with this approach it even not clear why do you need dir and val to be
> separated here (esp. if we add a enum like
>

We do want them to be separated not for better UX but to be able to
test all kernel APIs (gpiod_direction_input|output() and
gpiod_set_value()).

>         GPIO_VIRTUSER_OUT_LOW,
>         GPIO_VIRTUSER_OUT_HIGH,
>         GPIO_VIRTUSER_IN,
>
> (with it the string array can also be indexed).
>
> > +     return 0;
> > +}
>
> ...
>
> > +static int gpio_virtuser_parse_value(const char *buf)
> > +{
> > +     int value, ret;
> > +
> > +     value = sysfs_match_string(gpio_virtuser_sysfs_value_strings, buf);
> > +     if (value < 0) {
> > +             /* Can be 0 or 1 too. */
> > +             ret = kstrtoint(buf, 0, &value);
> > +             if (ret)
> > +                     return ret;
>
> > +             if (value != 0 && value != 1)
> > +                     return -EINVAL;
>
> Why not kstrtobool()?
>

I don't want to accept all the other strings kstrtobool() is fine with.

> > +     }
> > +
> > +     return value;
> > +}
>
> ...
>
> > +     ret = kstrtouint(buf, 10, &debounce);
>
> Why restrict to decimal?
>

Not sure what you gain from passing a period in hex?


> > +     if (ret)
> > +             return ret;
>
> ...
>
> > +static ssize_t
> > +gpio_virtuser_sysfs_consumer_store(struct device *dev,
> > +                                struct device_attribute *attr,
> > +                                const char *buf, size_t len)
> > +{
> > +     struct gpio_virtuser_line_data *data = to_gpio_virtuser_data(attr);
> > +     int ret;
>
> > +     if (strlen(buf) > GPIO_CONSUMER_NAME_MAX_LEN)
> > +             return -EINVAL;
>
> You don't need this if you use strscpy() below and check its returned value.
>

Ok.

> > +     guard(mutex)(&data->consumer_lock);
> > +
> > +     ret = gpiod_set_consumer_name(data->desc, buf);
> > +     if (ret)
> > +             return ret;
> > +
> > +     sprintf(data->consumer, buf);
> > +
> > +     return len;
> > +}
>
> ...
>
> > +     data->attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
> > +                                             "gpiod:%s", id);
>
> Why two lines?
>

It's not even longer than previously with the new struct layout so it
warrants a break.

> > +     if (!data->attr_group->name)
> > +             return -ENOMEM;
>
> ...
>
> > +     ret = devm_add_action_or_reset(dev, gpio_virtuser_mutex_destroy,
> > +                                    &data->consumer_lock);
>
> Don't we have devm_mutex_init() (`git tag --contains` shows v6.10-rc1 to me)
>

Ah we do now. This code dates back from early 2023 I think. I'll use it.

> > +             return ret;
>
> ...
>
> > +static int gpio_virtuser_prop_is_gpio(struct property *prop)
> > +{
> > +     char *dash = strpbrk(prop->name, "-");
>
> Why not strrchr() ?
>

Ok.

> > +     return dash && strcmp(dash, "-gpios") == 0;
>
> Can't we reuse the suffix from the array from the gpiolib internal header?
> Also I don't like the form of '-' in the line. "gpios" is good and chance
> that linker deduplicates the same string if it occurs somewhere else in the
> binary (in case this goes with =y in .config).
>

I'm not sure I follow what you're saying here. Please rephrase.

> > +}
>
> ...
>
> > +/*
> > + * If this is an OF-based system, then we iterate over properties and consider
> > + * all whose names end in "-gpios". For configfs we expect an additional string
> > + * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
> > + * to request.
>
> Why not any other system? What's wrong for having this available for ACPI, for
> example? Okay, I see that this is probably due to absence of API.
>
> OTOH the last call in the function assumes non-OF cases. Why can't we have the
> same approach in both?
>

Again: I have no idea what you mean. We support device-tree and
configfs as sources of configuration for these virtual consumers. If
you want to add something more, be my guest once it's upstream.

The reason to use a different approach is to not require the
"gpio-virtuser,ids" property in device-tree.

> > + */
> > +static int gpio_virtuser_count_ids(struct device *dev)
> > +{
> > +     struct fwnode_handle *fwnode = dev_fwnode(dev);
>
> Why? This function is mostly OF one, make it simpler.
>
>         struct device_node *np = dev_of_node(dev);
>
> > +     struct property *prop;
> > +     int ret = 0;
>
> > +     if (is_of_node(fwnode)) {
>
> Instead of this check...
>
>         if (np) {
>
> ...can be used.
>

Ok.

>
> > +             for_each_property_of_node(to_of_node(fwnode), prop) {
>
>         for_each_property_of_node(np, prop) {
>
> > +                     if (gpio_virtuser_prop_is_gpio(prop))
> > +                             ++ret;
>
> Why pre-increment?

Why not?

>
> > +             }
>
> > +             return ret;
> > +     }
>
> > +     return device_property_string_array_count(dev, "gpio-virtuser,ids");
> > +}
>
> ...
>
> > +static int gpio_virtuser_get_ids(struct device *dev, const char **ids,
> > +                              int num_ids)
> > +{
> > +     struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +     struct property *prop;
> > +     size_t pos = 0, diff;
> > +     char *dash, *tmp;
> > +
> > +     if (is_of_node(fwnode)) {
> > +             for_each_property_of_node(to_of_node(fwnode), prop) {
>
> As per above function.
>

Ok

> > +                     if (!gpio_virtuser_prop_is_gpio(prop))
> > +                             continue;
> > +
> > +                     dash = strpbrk(prop->name, "-");
> > +                     diff = dash - prop->name;
> > +
> > +                     tmp = devm_kmemdup(dev, prop->name, diff + 1,
> > +                                        GFP_KERNEL);
>
> devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at least I
> would rather expect wrapped kstrndup() than this.
>

Meh, this logic is fine as we know the range exactly. IMO kstrndup()
here would be overkill. I'd leave it for now.

> > +                     if (!tmp)
> > +                             return -ENOMEM;
> > +
> > +                     tmp[diff] = '\0';
> > +                     ids[pos++] = tmp;
> > +             }
> > +
> > +             return 0;
> > +     }
> > +
> > +     return device_property_read_string_array(dev, "gpio-virtuser,ids",
> > +                                              ids, num_ids);
> > +}
>
> ...
>
> > +static int gpio_virtuser_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct gpio_descs *descs;
> > +     int ret, num_ids = 0, i;
> > +     const char **ids;
> > +     unsigned int j;
> > +
> > +     num_ids = gpio_virtuser_count_ids(dev);
> > +     if (num_ids < 0)
> > +             return dev_err_probe(dev, num_ids,
> > +                                  "Failed to get the number of GPIOs to request\n");
> > +
> > +     if (num_ids == 0) {
> > +             dev_err(dev, "No GPIO IDs specified\n");
> > +             return -EINVAL;
>
> It's okay to
>
>                 return dev_err_probe(...);
>
> with know error code.
>

Ok.

> > +     }
> > +
> > +     ids = devm_kcalloc(dev, num_ids, sizeof(*ids), GFP_KERNEL);
> > +     if (!ids)
> > +             return -ENOMEM;
> > +
> > +     ret = gpio_virtuser_get_ids(dev, ids, num_ids);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to get the IDs of GPIOs to request\n");
> > +
> > +     for (i = 0; i < num_ids; i++) {
> > +             descs = devm_gpiod_get_array(dev, ids[i], GPIOD_ASIS);
> > +             if (IS_ERR(descs))
> > +                     return dev_err_probe(dev, PTR_ERR(descs),
> > +                                          "Failed to request the '%s' GPIOs\n",
> > +                                          ids[i]);
> > +
> > +             ret = gpio_virtuser_sysfs_init_line_array_attrs(dev, descs,
> > +                                                             ids[i]);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "Failed to setup the sysfs array interface for the '%s' GPIOs\n",
> > +                                          ids[i]);
> > +
> > +             for (j = 0; j < descs->ndescs; j++) {
> > +                     ret = gpio_virtuser_sysfs_init_line_attrs(dev,
> > +                                                     descs->desc[j],
> > +                                                     ids[i], j);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "Failed to setup the sysfs line interface for the '%s' GPIOs\n",
> > +                                                  ids[i]);
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static int gpio_virtuser_bus_notifier_call(struct notifier_block *nb,
> > +                                        unsigned long action, void *data)
> > +{
> > +     struct gpio_virtuser_device *vdev;
> > +     struct device *dev = data;
> > +     char devname[32];
> > +
> > +     vdev = container_of(nb, struct gpio_virtuser_device, bus_notifier);
> > +     snprintf(devname, sizeof(devname), "gpio-virtuser.%d", vdev->id);
> > +
> > +     if (strcmp(dev_name(dev), devname))
>
>         if (!device_match_name(...))
>

Ok

> > +             return NOTIFY_DONE;
> > +
> > +     switch (action) {
> > +     case BUS_NOTIFY_BOUND_DRIVER:
> > +             vdev->driver_bound = true;
> > +             break;
> > +     case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > +             vdev->driver_bound = false;
> > +             break;
> > +     default:
> > +             return NOTIFY_DONE;
> > +     }
> > +
> > +     complete(&vdev->probe_completion);
> > +     return NOTIFY_OK;
> > +}
>
> ...
>
> > +static ssize_t
> > +gpio_virtuser_lookup_entry_config_key_store(struct config_item *item,
> > +                                         const char *page, size_t count)
> > +{
> > +     struct gpio_virtuser_lookup_entry *entry =
> > +                                     to_gpio_virtuser_lookup_entry(item);
> > +     struct gpio_virtuser_device *dev = entry->parent->parent;
> > +
> > +     char *key = kstrndup(skip_spaces(page), count, GFP_KERNEL);
>
> Missing __free() ?
>

Right.

> > +     if (!key)
> > +             return -ENOMEM;
>
> > +     strim(key);
>
> > +     guard(mutex)(&dev->lock);
> > +
> > +     if (gpio_virtuser_device_is_live(dev))
> > +             return -EBUSY;
> > +
> > +     kfree(entry->key);
> > +     entry->key = no_free_ptr(key);
> > +
> > +     return count;
> > +}
>
> ...
>
> > +     if (sysfs_streq(page, "pull-up")) {
> > +             entry->flags &= ~(GPIO_PULL_DOWN | GPIO_PULL_DISABLE);
> > +             entry->flags |= GPIO_PULL_UP;
> > +     } else if (sysfs_streq(page, "pull-down")) {
> > +             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DISABLE);
> > +             entry->flags |= GPIO_PULL_DOWN;
> > +     } else if (sysfs_streq(page, "pull-disabled")) {
> > +             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DOWN);
> > +             entry->flags |= GPIO_PULL_DISABLE;
> > +     } else if (sysfs_streq(page, "as-is")) {
> > +             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DOWN |
> > +                               GPIO_PULL_DISABLE);
> > +     } else {
> > +             count = -EINVAL;
>
>                 return -EINVAL won't (ab)use count semantics.
> > +     }
> > +
> > +     return count;
>
> ...
>
> > +     return sprintf(page, "%s\n", flags & GPIO_ACTIVE_LOW ? "1" : "0");
>
> Somewhere above you used %d for very similar situation, why %s here?
> Or why "5d" there?
>

No reason, I'll unify it.

> ...
>
> > +     return sprintf(page, "%s\n", flags & GPIO_TRANSITORY ? "1" : "0");
>
> Ditto.
>
> ...
>
> > +     return sprintf(page, "%c\n", live ? '1' : '0');
>
> Wow! Third type of the same.
>
> ...
>
> > +     struct gpiod_lookup_table *table __free(kfree) =
> > +             kzalloc(struct_size(table, table, num_entries + 1), GFP_KERNEL);
> > +     if (!table)
> > +             return -ENOMEM;
>
> > +     table->dev_id = kasprintf(GFP_KERNEL, "gpio-virtuser.%d",
> > +                               dev->id);
>
> Perfectly one line in comparison with the few lines above).
>

Ok

> > +     if (!table->dev_id)
> > +             return -ENOMEM;
>
> ...
>
> > +                     curr->chip_hwnum = entry->offset < 0
> > +                                             ? U16_MAX : entry->offset;
>
> Can we leave ? on the previous line?
>
> ...
>
> > +                     ++i;
>
> Why pre-increment?
>
> ...
>
> > +static struct fwnode_handle *
> > +gpio_virtuser_make_device_swnode(struct gpio_virtuser_device *dev)
> > +{
> > +     struct property_entry properties[2];
> > +     struct gpio_virtuser_lookup *lookup;
> > +     size_t num_ids;
> > +     int i = 0;
>
> Why signed? And in all this kind of case, I would split assignment...
>
> > +     memset(properties, 0, sizeof(properties));
> > +
> > +     num_ids = list_count_nodes(&dev->lookup_list);
> > +     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),
> > +                                        GFP_KERNEL);
> > +     if (!ids)
> > +             return ERR_PTR(-ENOMEM);
> > +
>
> To be here, that the reader will see immediately (close enough) what is the
> initial values. Moreover this code will be robuse against changes in between
> (if i become reusable).
>

Sorry, I can't parse it.

> > +     list_for_each_entry(lookup, &dev->lookup_list, siblings)
> > +             ids[i++] = lookup->con_id;
> > +
> > +     properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN("gpio-virtuser,ids",
> > +                                                     ids, num_ids);
> > +
> > +     return fwnode_create_software_node(properties, NULL);
> > +}
>
> ...
>
> > +     guard(mutex)(&dev->lock);
> > +
> > +     if (live == gpio_virtuser_device_is_live(dev))
> > +             ret = -EPERM;
>
> With guard in place, just return directly, ...
>
> > +     else if (live)
>
> ...drop 'else'...
>
> > +             ret = gpio_virtuser_device_activate(dev);
> > +     else
>
> ...ditto...
>
> > +             gpio_virtuser_device_deactivate(dev);
> > +
> > +     return ret ?: count;
>
> ...and simply return count here.
>

Ok.

>
> ...
>
> > +     struct gpio_virtuser_device *dev __free(kfree) = kzalloc(sizeof(*dev),
> > +                                                              GFP_KERNEL);
>
>         struct gpio_virtuser_device *dev __free(kfree) =
>                 kzalloc(sizeof(*dev), GFP_KERNEL);
>
> > +     if (!dev)
> > +             return ERR_PTR(-ENOMEM);
>
> ...
>
> > +     ret = platform_driver_register(&gpio_virtuser_driver);
> > +     if (ret) {
>
> > +             pr_err("Failed to register the platform driver: %d\n",
> > +                    ret);
>
> I would keep one line.
>

Ok.

> > +             return ret;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thanks for the review!
Bart