| Message ID | 20240320125945.16985-1-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: cdev: sanitize the label before requesting the interrupt | expand |
On Fri, Mar 22, 2024 at 12:31:36PM +0300, Dan Carpenter wrote: > On Fri, Mar 22, 2024 at 08:46:50AM +0100, Bartosz Golaszewski wrote: > > On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Let's replace all "/" with "-". > > > > > > > > > > I actually prefer the ":" you originally suggested, as it more clearly > > > indicates a tier separation, whereas a hyphen is commonly used for > > > multi-word names. And as the hyphen is more commonly used the sanitized > > > name is more likely to conflict. > > > > > > > Sounds good, will do. > > > > > > > > + label = make_irq_label(le->label); > > > > + if (!label) > > > > + goto out_free_le; > > > > + > > > > > > Need to set ret = -ENOMEM before the goto, else you will return 0. > > > > > > > Eek, right, thanks. > > Smatch has a warning about this, btw. > drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret' > And that triggered a "what the hell does that mean" warning in my wetware error parser ;-). That could be better worded - it isn't "missing", it hasn't been appropriately set. So maybe "unset error code"? > The other warning here is: > drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258. > Looks like a false positive to me - as per the comment in the code, that path (2258) results in lineevent_release() being called and that releases the irq. Cheers, Kent.
On Fri, Mar 22, 2024 at 07:54:19PM +0800, Kent Gibson wrote: > On Fri, Mar 22, 2024 at 12:31:36PM +0300, Dan Carpenter wrote: > > On Fri, Mar 22, 2024 at 08:46:50AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > > > Let's replace all "/" with "-". > > > > > > > > > > > > > I actually prefer the ":" you originally suggested, as it more clearly > > > > indicates a tier separation, whereas a hyphen is commonly used for > > > > multi-word names. And as the hyphen is more commonly used the sanitized > > > > name is more likely to conflict. > > > > > > > > > > Sounds good, will do. > > > > > > > > > > + label = make_irq_label(le->label); > > > > > + if (!label) > > > > > + goto out_free_le; > > > > > + > > > > > > > > Need to set ret = -ENOMEM before the goto, else you will return 0. > > > > > > > > > > Eek, right, thanks. > > > > Smatch has a warning about this, btw. > > drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret' > > > > And that triggered a "what the hell does that mean" warning in my > wetware error parser ;-). > > That could be better worded - it isn't "missing", it hasn't been > appropriately set. So maybe "unset error code"? > It's kind of a pain to change the warning message after the fact because then they show up as new warnings for everyone... I maybe should poll people to see if they care about the hassel of it. > > The other warning here is: > > drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258. > > > > Looks like a false positive to me - as per the comment in the code, that path > (2258) results in lineevent_release() being called and that releases the irq. Ah right. Thanks! regards, dan carpenter
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index f384fa278764..8b5e8e92cbb5 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1083,10 +1083,20 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc, return 0; } +static inline char *make_irq_label(const char *orig) +{ + return kstrdup_and_replace(orig, '/', '-', GFP_KERNEL); +} + +static inline void free_irq_label(const char *label) +{ + kfree(label); +} + static void edge_detector_stop(struct line *line) { if (line->irq) { - free_irq(line->irq, line); + free_irq_label(free_irq(line->irq, line)); line->irq = 0; } @@ -1110,6 +1120,7 @@ static int edge_detector_setup(struct line *line, unsigned long irqflags = 0; u64 eflags; int irq, ret; + char *label; eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS; if (eflags && !kfifo_initialized(&line->req->events)) { @@ -1146,11 +1157,17 @@ static int edge_detector_setup(struct line *line, IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; irqflags |= IRQF_ONESHOT; + label = make_irq_label(line->req->label); + if (!label) + return -ENOMEM; + /* Request a thread to read the events */ ret = request_threaded_irq(irq, edge_irq_handler, edge_irq_thread, - irqflags, line->req->label, line); - if (ret) + irqflags, label, line); + if (ret) { + free_irq_label(label); return ret; + } line->irq = irq; return 0; @@ -1973,7 +1990,7 @@ static void lineevent_free(struct lineevent_state *le) blocking_notifier_chain_unregister(&le->gdev->device_notifier, &le->device_unregistered_nb); if (le->irq) - free_irq(le->irq, le); + free_irq_label(free_irq(le->irq, le)); if (le->desc) gpiod_free(le->desc); kfree(le->label); @@ -2114,6 +2131,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) int fd; int ret; int irq, irqflags = 0; + char *label; if (copy_from_user(&eventreq, ip, sizeof(eventreq))) return -EFAULT; @@ -2198,12 +2216,16 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) if (ret) goto out_free_le; + label = make_irq_label(le->label); + if (!label) + goto out_free_le; + /* Request a thread to read the events */ ret = request_threaded_irq(irq, lineevent_irq_handler, lineevent_irq_thread, irqflags, - le->label, + label, le); if (ret) goto out_free_le;