* [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
@ 2024-03-20 12:59 Bartosz Golaszewski
2024-03-22 1:30 ` Kent Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2024-03-20 12:59 UTC (permalink / raw
To: Kent Gibson, Linus Walleij
Cc: linux-gpio, linux-kernel, Alexey Dobriyan, Bartosz Golaszewski,
stable, Stefan Wahren
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Let's replace all "/" with "-".
Cc: stable@vger.kernel.org
Reported-by: Stefan Wahren <wahrenst@gmx.net>
Closes: https://lore.kernel.org/linux-gpio/39fe95cb-aa83-4b8b-8cab-63947a726754@gmx.net/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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;
+
/* 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
2024-03-20 12:59 [PATCH] gpio: cdev: sanitize the label before requesting the interrupt Bartosz Golaszewski
@ 2024-03-22 1:30 ` Kent Gibson
2024-03-22 7:46 ` Bartosz Golaszewski
0 siblings, 1 reply; 6+ messages in thread
From: Kent Gibson @ 2024-03-22 1:30 UTC (permalink / raw
To: Bartosz Golaszewski
Cc: Linus Walleij, linux-gpio, linux-kernel, Alexey Dobriyan,
Bartosz Golaszewski, stable, Stefan Wahren
On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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@vger.kernel.org
> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> Closes: https://lore.kernel.org/linux-gpio/39fe95cb-aa83-4b8b-8cab-63947a726754@gmx.net/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
2024-03-22 1:30 ` Kent Gibson
@ 2024-03-22 7:46 ` Bartosz Golaszewski
2024-03-22 9:31 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2024-03-22 7:46 UTC (permalink / raw
To: Kent Gibson
Cc: Linus Walleij, linux-gpio, linux-kernel, Alexey Dobriyan,
Bartosz Golaszewski, stable, Stefan Wahren
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.
Bart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
2024-03-22 7:46 ` Bartosz Golaszewski
@ 2024-03-22 9:31 ` Dan Carpenter
2024-03-22 11:54 ` Kent Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2024-03-22 9:31 UTC (permalink / raw
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, linux-gpio, linux-kernel,
Alexey Dobriyan, Bartosz Golaszewski, stable, Stefan Wahren
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'
The other warning here is:
drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
2024-03-22 9:31 ` Dan Carpenter
@ 2024-03-22 11:54 ` Kent Gibson
2024-03-22 12:58 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Kent Gibson @ 2024-03-22 11:54 UTC (permalink / raw
To: Dan Carpenter
Cc: Bartosz Golaszewski, Linus Walleij, linux-gpio, linux-kernel,
Alexey Dobriyan, Bartosz Golaszewski, stable, Stefan Wahren
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: cdev: sanitize the label before requesting the interrupt
2024-03-22 11:54 ` Kent Gibson
@ 2024-03-22 12:58 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2024-03-22 12:58 UTC (permalink / raw
To: Kent Gibson
Cc: Bartosz Golaszewski, Linus Walleij, linux-gpio, linux-kernel,
Alexey Dobriyan, Bartosz Golaszewski, stable, Stefan Wahren
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-22 12:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-20 12:59 [PATCH] gpio: cdev: sanitize the label before requesting the interrupt Bartosz Golaszewski
2024-03-22 1:30 ` Kent Gibson
2024-03-22 7:46 ` Bartosz Golaszewski
2024-03-22 9:31 ` Dan Carpenter
2024-03-22 11:54 ` Kent Gibson
2024-03-22 12:58 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.