| Message ID | 20230822075122.6900-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: sim: dispose of irq mappings before destroying the irq_sim domain | expand |
On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > If a GPIO simulator device is unbound with interrupts still requested, > we will hit a use-after-free issue in __irq_domain_deactivate_irq(). The > owner of the irq domain must dispose of all mappings before destroying > the domain object. ... > +static void gpio_sim_dispose_mappings(void *data) > +{ > + struct gpio_sim_chip *chip = data; > + unsigned int i, irq; > + > + for (i = 0; i < chip->gc.ngpio; i++) { > + irq = irq_find_mapping(chip->irq_sim, i); > + if (irq) This duplicates check in the following call. > + irq_dispose_mapping(irq); > + } > +}
On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote: > On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote: ... > > > +static void gpio_sim_dispose_mappings(void *data) > > > +{ > > > + struct gpio_sim_chip *chip = data; > > > + unsigned int i, irq; > > > + > > > + for (i = 0; i < chip->gc.ngpio; i++) { > > > + irq = irq_find_mapping(chip->irq_sim, i); > > > > > + if (irq) > > > > This duplicates check in the following call. > > > > Ah so it can be a direct call: > > irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i)); > > ? Hehe, seems yes and no. According to the code — yes, but code seems buggy, and compiler may effectively drop the check (haven't checked this, though). OTOH, the problem may appear if and only if we have no sparse IRQ configuration which is probably rare. That said, I will go without check, it's not your issue. And I found other places in IRQ framework that misses that check. > > > + irq_dispose_mapping(irq); > > > + } > > > +}
On Tue, Aug 22, 2023 at 02:38:28PM +0200, Bartosz Golaszewski wrote: > On Tue, Aug 22, 2023 at 2:24 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 22, 2023 at 02:16:44PM +0200, Bartosz Golaszewski wrote: > > > On Tue, Aug 22, 2023 at 2:12 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Aug 22, 2023 at 09:51:21AM +0200, Bartosz Golaszewski wrote: ... > > > > > +static void gpio_sim_dispose_mappings(void *data) > > > > > +{ > > > > > + struct gpio_sim_chip *chip = data; > > > > > + unsigned int i, irq; > > > > > + > > > > > + for (i = 0; i < chip->gc.ngpio; i++) { > > > > > + irq = irq_find_mapping(chip->irq_sim, i); > > > > > > > > > + if (irq) > > > > > > > > This duplicates check in the following call. > > > > > > > > > > Ah so it can be a direct call: > > > > > > irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i)); > > > > > > ? > > > > Hehe, seems yes and no. According to the code — yes, but code seems buggy, > > and compiler may effectively drop the check (haven't checked this, though). > > > > OTOH, the problem may appear if and only if we have no sparse IRQ configuration > > which is probably rare. > > > > That said, I will go without check, it's not your issue. > > And I found other places in IRQ framework that misses that check. > > > > I disagree. If there's no strong contract from the provider of this > function then this check is so cheap that I'm ready to live with it. There are plenty of calls that don't check and there are calls that check. > > > > > + irq_dispose_mapping(irq); > > > > > + } > > > > > +}
On Tue, Aug 22, 2023 at 09:51:22AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Associate the swnode of the GPIO device's (which is the interrupt > controller here) with the irq domain. Otherwise the interrupt-controller > device attribute is will be a no-op. "is will be" ? Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: cb8c474e79be ("gpio: sim: new testing module") > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpio-sim.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c > index 27515384aa10..835999343f16 100644 > --- a/drivers/gpio/gpio-sim.c > +++ b/drivers/gpio/gpio-sim.c > @@ -414,7 +414,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) > if (!chip->pull_map) > return -ENOMEM; > > - chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines); > + chip->irq_sim = devm_irq_domain_create_sim(dev, swnode, num_lines); > if (IS_ERR(chip->irq_sim)) > return PTR_ERR(chip->irq_sim); > > -- > 2.39.2 >
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c index f1f6f1c32987..27515384aa10 100644 --- a/drivers/gpio/gpio-sim.c +++ b/drivers/gpio/gpio-sim.c @@ -291,6 +291,18 @@ static void gpio_sim_mutex_destroy(void *data) mutex_destroy(lock); } +static void gpio_sim_dispose_mappings(void *data) +{ + struct gpio_sim_chip *chip = data; + unsigned int i, irq; + + for (i = 0; i < chip->gc.ngpio; i++) { + irq = irq_find_mapping(chip->irq_sim, i); + if (irq) + irq_dispose_mapping(irq); + } +} + static void gpio_sim_sysfs_remove(void *data) { struct gpio_sim_chip *chip = data; @@ -406,6 +418,10 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) if (IS_ERR(chip->irq_sim)) return PTR_ERR(chip->irq_sim); + ret = devm_add_action_or_reset(dev, gpio_sim_dispose_mappings, chip); + if (ret) + return ret; + mutex_init(&chip->lock); ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy, &chip->lock);