On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > When an interrupt is requested, a procfs directory is created under > "/proc/irq/<irqnum>/<label>" where <label> is the string passed to one of > the request_irq() variants. > > What follows is that the string must not contain the "/" character or > the procfs mkdir operation will fail. We don't have such constraints for > GPIO consumer labels which are used verbatim as interrupt labels for > GPIO irqs. We must therefore sanitize the consumer string before > requesting the interrupt. > As previously mentioned, it would be nice for that constraint to be documented in the irq header to help prevent others falling into the same trap. Else a short comment in the code here to explain the need for sanitization. > 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. > Cc: stable@xxxxxxxxxxxxxxx > Reported-by: Stefan Wahren <wahrenst@xxxxxxx> > Closes: https://lore.kernel.org/linux-gpio/39fe95cb-aa83-4b8b-8cab-63947a726754@xxxxxxx/ > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > --- > drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > 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; > + Need to set ret = -ENOMEM before the goto, else you will return 0. Cheers, Kent. > /* 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; > -- > 2.40.1 >
