Re: [PATCH v7] gpio: virtuser: new virtual driver
From: Bartosz Golaszewski
Date: Mon Jun 10 2024 - 09:22:59 EST
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