| Message ID | 20231221184316.24506-3-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: use a read-write semaphore to protect the GPIO device list | expand |
On Thu, Dec 21, 2023 at 7:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > There's time between when we locate the relevant descriptor during > lookup and when we actually take the reference to its parent GPIO > device where - if the GPIO device in question is removed - we'll end up > with a dangling pointer to freed memory. Make sure devices cannot be > removed until we hold a new reference to the device. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 1baeb6778ec6..8a15b8f6b50e 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -4147,27 +4147,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer, > struct gpio_desc *desc; > int ret; > > - desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags); > - if (gpiod_not_found(desc) && platform_lookup_allowed) { > + scoped_guard(rwsem_read, &gpio_devices_sem) { I am too sleep deprived to be coding. This doesn't make sense because if we are held at the write semaphore in gpiodev_release() then it's already too late as the device is being removed already and we'll still end up with a dangling pointer. Should we hold off any reference putting for all GPIO devices when a descriptor lookup is in progress? Bart > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > + &flags, &lookupflags); > + if (gpiod_not_found(desc) && platform_lookup_allowed) { > + /* > + * Either we are not using DT or ACPI, or their lookup > + * did not return a result. In that case, use platform > + * lookup as a fallback. > + */ > + dev_dbg(consumer, > + "using lookup tables for GPIO lookup\n"); > + desc = gpiod_find(consumer, con_id, idx, &lookupflags); > + } > + > + if (IS_ERR(desc)) { > + dev_dbg(consumer, "No GPIO consumer %s found\n", > + con_id); > + return desc; > + } > + > /* > - * Either we are not using DT or ACPI, or their lookup did not > - * return a result. In that case, use platform lookup as a > - * fallback. > + * If a connection label was passed use that, else attempt to > + * use the device name as label > */ > - dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); > - desc = gpiod_find(consumer, con_id, idx, &lookupflags); > + ret = gpiod_request(desc, label); > } > > - if (IS_ERR(desc)) { > - dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); > - return desc; > - } > - > - /* > - * If a connection label was passed use that, else attempt to use > - * the device name as label > - */ > - ret = gpiod_request(desc, label); > if (ret) { > if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) > return ERR_PTR(ret); > -- > 2.40.1 >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 1baeb6778ec6..8a15b8f6b50e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4147,27 +4147,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer, struct gpio_desc *desc; int ret; - desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags); - if (gpiod_not_found(desc) && platform_lookup_allowed) { + scoped_guard(rwsem_read, &gpio_devices_sem) { + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, + &flags, &lookupflags); + if (gpiod_not_found(desc) && platform_lookup_allowed) { + /* + * Either we are not using DT or ACPI, or their lookup + * did not return a result. In that case, use platform + * lookup as a fallback. + */ + dev_dbg(consumer, + "using lookup tables for GPIO lookup\n"); + desc = gpiod_find(consumer, con_id, idx, &lookupflags); + } + + if (IS_ERR(desc)) { + dev_dbg(consumer, "No GPIO consumer %s found\n", + con_id); + return desc; + } + /* - * Either we are not using DT or ACPI, or their lookup did not - * return a result. In that case, use platform lookup as a - * fallback. + * If a connection label was passed use that, else attempt to + * use the device name as label */ - dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); - desc = gpiod_find(consumer, con_id, idx, &lookupflags); + ret = gpiod_request(desc, label); } - if (IS_ERR(desc)) { - dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); - return desc; - } - - /* - * If a connection label was passed use that, else attempt to use - * the device name as label - */ - ret = gpiod_request(desc, label); if (ret) { if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) return ERR_PTR(ret);