| Message ID | 20240220111019.133697-3-herve.codina@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-cdev: Release IRQ used by gpio-cdev on gpio chip removal | expand |
On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > When gpio chip device is removed while some related gpio are used by the > user-space, the following warning can appear: > remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' > WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c > ... > Call trace: > remove_proc_entry+0x190/0x19c > unregister_irq_proc+0xd0/0x104 > free_desc+0x4c/0xc4 > irq_free_descs+0x6c/0x90 > irq_dispose_mapping+0x104/0x14c > gpiochip_irqchip_remove+0xcc/0x1a4 > gpiochip_remove+0x48/0x100 > ... > > Indeed, the gpio cdev uses an IRQ but this IRQ is not released when the > gpio chip device is removed. > > Release IRQs used in the device removal notifier functions. > Also move one of these function definition in order to avoid a forward > declaration (move after the edge_detector_stop() definition). > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- > 1 file changed, 22 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 2a88736629ef..aec4a4c8490a 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -688,17 +688,6 @@ static void line_set_debounce_period(struct line *line, > GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \ > GPIO_V2_LINE_EDGE_FLAGS) > > -static int linereq_unregistered_notify(struct notifier_block *nb, > - unsigned long action, void *data) > -{ > - struct linereq *lr = container_of(nb, struct linereq, > - device_unregistered_nb); > - > - wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR); > - > - return NOTIFY_OK; > -} > - > static void linereq_put_event(struct linereq *lr, > struct gpio_v2_line_event *le) > { > @@ -1189,6 +1178,23 @@ static int edge_detector_update(struct line *line, > return edge_detector_setup(line, lc, line_idx, edflags); > } > > +static int linereq_unregistered_notify(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct linereq *lr = container_of(nb, struct linereq, > + device_unregistered_nb); > + int i; > + > + for (i = 0; i < lr->num_lines; i++) { > + if (lr->lines[i].desc) > + edge_detector_stop(&lr->lines[i]); > + } > + Firstly, the re-ordering in the previous patch creates a race, as the NULLing of the gdev->chip serves to numb the cdev ioctls, so there is now a window between the notifier being called and that numbing, during which userspace may call linereq_set_config() and re-request the irq. There is also a race here with linereq_set_config(). That can be prevented by holding the lr->config_mutex - assuming the notifier is not being called from atomic context. You also have a race with the line being freed that could pull the lr out from under you, so a use after free problem. I'd rather live with the warning :(. Fixing that requires rethinking the lifecycle management for the linereq/lineevent. Cheers, Kent. > + wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR); > + > + return NOTIFY_OK; > +} > + > static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc, > unsigned int line_idx) > { > @@ -1898,6 +1904,11 @@ static int lineevent_unregistered_notify(struct notifier_block *nb, > struct lineevent_state *le = container_of(nb, struct lineevent_state, > device_unregistered_nb); > > + if (le->irq) { > + free_irq(le->irq, le); > + le->irq = 0; > + } > + > wake_up_poll(&le->wait, EPOLLIN | EPOLLERR); > > return NOTIFY_OK; > -- > 2.43.0 >
Hi Kent, On Tue, 20 Feb 2024 22:29:59 +0800 Kent Gibson <warthog618@gmail.com> wrote: > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > > When gpio chip device is removed while some related gpio are used by the > > user-space, the following warning can appear: > > remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' > > WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c > > ... > > Call trace: > > remove_proc_entry+0x190/0x19c > > unregister_irq_proc+0xd0/0x104 > > free_desc+0x4c/0xc4 > > irq_free_descs+0x6c/0x90 > > irq_dispose_mapping+0x104/0x14c > > gpiochip_irqchip_remove+0xcc/0x1a4 > > gpiochip_remove+0x48/0x100 > > ... > > > > Indeed, the gpio cdev uses an IRQ but this IRQ is not released when the > > gpio chip device is removed. > > > > Release IRQs used in the device removal notifier functions. > > Also move one of these function definition in order to avoid a forward > > declaration (move after the edge_detector_stop() definition). > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > --- > > drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- > > 1 file changed, 22 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > > index 2a88736629ef..aec4a4c8490a 100644 > > --- a/drivers/gpio/gpiolib-cdev.c > > +++ b/drivers/gpio/gpiolib-cdev.c > > @@ -688,17 +688,6 @@ static void line_set_debounce_period(struct line *line, > > GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \ > > GPIO_V2_LINE_EDGE_FLAGS) > > > > -static int linereq_unregistered_notify(struct notifier_block *nb, > > - unsigned long action, void *data) > > -{ > > - struct linereq *lr = container_of(nb, struct linereq, > > - device_unregistered_nb); > > - > > - wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR); > > - > > - return NOTIFY_OK; > > -} > > - > > static void linereq_put_event(struct linereq *lr, > > struct gpio_v2_line_event *le) > > { > > @@ -1189,6 +1178,23 @@ static int edge_detector_update(struct line *line, > > return edge_detector_setup(line, lc, line_idx, edflags); > > } > > > > +static int linereq_unregistered_notify(struct notifier_block *nb, > > + unsigned long action, void *data) > > +{ > > + struct linereq *lr = container_of(nb, struct linereq, > > + device_unregistered_nb); > > + int i; > > + > > + for (i = 0; i < lr->num_lines; i++) { > > + if (lr->lines[i].desc) > > + edge_detector_stop(&lr->lines[i]); > > + } > > + > > Firstly, the re-ordering in the previous patch creates a race, > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > there is now a window between the notifier being called and that numbing, > during which userspace may call linereq_set_config() and re-request > the irq. Well in my previous patch, if gdev->chip need to NULL before the call to gcdev_unregister(), this can be done. I did modification that leads to the following sequence: --- 8< --- ... gcdev_unregister(gdev); gpiochip_free_hogs(gc); /* Numb the device, cancelling all outstanding operations */ gdev->chip = NULL; gpiochip_irqchip_remove(gc); acpi_gpiochip_remove(gc); of_gpiochip_remove(gc); gpiochip_remove_pin_ranges(gc); ... --- 8< --- I can call gcdev_unregister() right after gdev->chip = NULL. The needed things is to have free_irq() (from the gcdev_unregister()) called before calling gpiochip_irqchip_remove(). And so, why not: --- 8< --- ... gpiochip_free_hogs(gc); /* Numb the device, cancelling all outstanding operations */ gdev->chip = NULL; gcdev_unregister(gdev); gpiochip_irqchip_remove(gc); acpi_gpiochip_remove(gc); of_gpiochip_remove(gc); gpiochip_remove_pin_ranges(gc); ... --- 8< --- > > There is also a race here with linereq_set_config(). That can be prevented > by holding the lr->config_mutex - assuming the notifier is not being called > from atomic context. I missed that one and indeed, I probably can take the mutex. With the mutex holded, no more race condition with linereq_set_config() and so the IRQ cannot be re-requested. > > You also have a race with the line being freed that could pull the > lr out from under you, so a use after free problem. I probably missed something but I don't see this use after free. Can you give me some details/pointers ? > I'd rather live with the warning :(. > Fixing that requires rethinking the lifecycle management for the > linereq/lineevent. Well, currently the warning is a big one with a dump_stack included. It will be interesting to have it fixed. The need to fix it is to have free_irq() called before gpiochip_irqchip_remove(); Is there really no way to have this correct sequence without rethinking all the lifecycle management ? Also, after the warning related to the IRQ, the following one is present: --- 8< --- [ 9593.527961] gpio gpiochip9: REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED [ 9593.535602] ------------[ cut here ]------------ [ 9593.540244] WARNING: CPU: 0 PID: 309 at drivers/gpio/gpiolib.c:2352 gpiod_free.part.0+0x20/0x48 ... [ 9593.725016] Call trace: [ 9593.727468] gpiod_free.part.0+0x20/0x48 [ 9593.731404] gpiod_free+0x14/0x24 [ 9593.734728] lineevent_free+0x40/0x74 [ 9593.738402] lineevent_release+0x14/0x24 [ 9593.742335] __fput+0x70/0x2bc [ 9593.745403] __fput_sync+0x50/0x5c [ 9593.748817] __arm64_sys_close+0x38/0x7c [ 9593.752751] invoke_syscall+0x48/0x114 ... [ 9593.815299] ---[ end trace 0000000000000000 ]--- [ 9593.820616] hotplug-manager dock-hotplug-manager: remove overlay 0 (ovcs id 1) gpiomon: error waiting for events: No such device # --- 8< --- Best regards, Hervé
On Tue, Feb 20, 2024 at 07:26:57PM +0100, Herve Codina wrote: > Hi Kent, > > On Tue, 20 Feb 2024 22:29:59 +0800 > Kent Gibson <warthog618@gmail.com> wrote: > ... > > I can call gcdev_unregister() right after gdev->chip = NULL. > The needed things is to have free_irq() (from the gcdev_unregister()) called > before calling gpiochip_irqchip_remove(). > > And so, why not: > --- 8< --- > ... > gpiochip_free_hogs(gc); > /* Numb the device, cancelling all outstanding operations */ > gdev->chip = NULL; > gcdev_unregister(gdev); > gpiochip_irqchip_remove(gc); > acpi_gpiochip_remove(gc); > of_gpiochip_remove(gc); > gpiochip_remove_pin_ranges(gc); > ... > --- 8< --- > Exactly - why not. Though adding some comments to the code as to why that ordering is required, as per the numbing, would be helpful for maintainability. > > > > There is also a race here with linereq_set_config(). That can be prevented > > by holding the lr->config_mutex - assuming the notifier is not being called > > from atomic context. > > I missed that one and indeed, I probably can take the mutex. With the mutex > holded, no more race condition with linereq_set_config() and so the IRQ cannot > be re-requested. > > > > > You also have a race with the line being freed that could pull the > > lr out from under you, so a use after free problem. > > I probably missed something but I don't see this use after free. > Can you give me some details/pointers ? > What is to prevent userspace releasing the request and freeing the linereq while you use it? The use after free is anywhere that is possible. AIUI, from the userspace side that is prevented by the file handle not being closed, and so linereq_release() not being called, while ioctls are in flight. But as you are calling in from the kernel side there is nothing in place to prevent userspace freeing the linereq while you are using it. > > > I'd rather live with the warning :(. > > Fixing that requires rethinking the lifecycle management for the > > linereq/lineevent. > > Well, currently the warning is a big one with a dump_stack included. > It will be interesting to have it fixed. > > The need to fix it is to have free_irq() called before > gpiochip_irqchip_remove(); > > Is there really no way to have this correct sequence without rethinking all > the lifecycle management ? > Not that I am aware of. You need to protect against the linereq being freed while you are using it, which is by definition is lifecycle management. Though it isn't necessarily __all__ the lifecycle management. > Also, after the warning related to the IRQ, the following one is present: > --- 8< --- > [ 9593.527961] gpio gpiochip9: REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED > [ 9593.535602] ------------[ cut here ]------------ > [ 9593.540244] WARNING: CPU: 0 PID: 309 at drivers/gpio/gpiolib.c:2352 gpiod_free.part.0+0x20/0x48 > ... > [ 9593.725016] Call trace: > [ 9593.727468] gpiod_free.part.0+0x20/0x48 > [ 9593.731404] gpiod_free+0x14/0x24 > [ 9593.734728] lineevent_free+0x40/0x74 > [ 9593.738402] lineevent_release+0x14/0x24 > [ 9593.742335] __fput+0x70/0x2bc > [ 9593.745403] __fput_sync+0x50/0x5c > [ 9593.748817] __arm64_sys_close+0x38/0x7c > [ 9593.752751] invoke_syscall+0x48/0x114 > ... > [ 9593.815299] ---[ end trace 0000000000000000 ]--- > [ 9593.820616] hotplug-manager dock-hotplug-manager: remove overlay 0 (ovcs id 1) > gpiomon: error waiting for events: No such device > # > --- 8< --- > My understanding is that we now handle that case - that is what the gdev->device_notifier chain is for, and gpiolib and gpiolib-cdev now perform a controlled cleanupi - so that warning is obsolete and should be removed from gpiochip_remove(). IIRC, previously you would've gotten a panic shortly after that warning. Now you get gpiomon noticing that the device has been removed. Btw, where I mention linereq here, the same applies to lineevent and linehandle - the uAPI v1 equivalents of linereq. Cheers, Kent.
On Wed, Feb 21, 2024 at 08:25:55AM +0800, Kent Gibson wrote: > On Tue, Feb 20, 2024 at 07:26:57PM +0100, Herve Codina wrote: > > Hi Kent, > > > > > > I probably missed something but I don't see this use after free. > > Can you give me some details/pointers ? > > > > What is to prevent userspace releasing the request and freeing the > linereq while you use it? The use after free is anywhere that is > possible. > To answer my own question - the notifier call chain itself will prevent that - linereq_free() will get blocked on the notifier chain semaphore until the notifier call returns. So there is no use after free problem. My bad - sorry for the added confusion. Cheers, Kent.
On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: ... > > } > > > > +static int linereq_unregistered_notify(struct notifier_block *nb, > > + unsigned long action, void *data) > > +{ > > + struct linereq *lr = container_of(nb, struct linereq, > > + device_unregistered_nb); > > + int i; > > + > > + for (i = 0; i < lr->num_lines; i++) { > > + if (lr->lines[i].desc) > > + edge_detector_stop(&lr->lines[i]); > > + } > > + > > Firstly, the re-ordering in the previous patch creates a race, > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > there is now a window between the notifier being called and that numbing, > during which userspace may call linereq_set_config() and re-request > the irq. > > There is also a race here with linereq_set_config(). That can be prevented > by holding the lr->config_mutex - assuming the notifier is not being called > from atomic context. > It occurs to me that the fixed reordering in patch 1 would place the notifier call AFTER the NULLing of the ioctls, so there will no longer be any chance of a race with linereq_set_config() - so holding the config_mutex semaphore is not necessary. In which case this patch is fine - it is only patch 1 that requires updating. Cheers, Kent.
On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote: > On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: > > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > > ... > > > > } > > > > > > +static int linereq_unregistered_notify(struct notifier_block *nb, > > > + unsigned long action, void *data) > > > +{ > > > + struct linereq *lr = container_of(nb, struct linereq, > > > + device_unregistered_nb); > > > + int i; > > > + > > > + for (i = 0; i < lr->num_lines; i++) { > > > + if (lr->lines[i].desc) > > > + edge_detector_stop(&lr->lines[i]); > > > + } > > > + > > > > Firstly, the re-ordering in the previous patch creates a race, > > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > > there is now a window between the notifier being called and that numbing, > > during which userspace may call linereq_set_config() and re-request > > the irq. > > > > There is also a race here with linereq_set_config(). That can be prevented > > by holding the lr->config_mutex - assuming the notifier is not being called > > from atomic context. > > > > It occurs to me that the fixed reordering in patch 1 would place > the notifier call AFTER the NULLing of the ioctls, so there will no longer > be any chance of a race with linereq_set_config() - so holding the > config_mutex semaphore is not necessary. > NULLing -> numbing The gdev->chip is NULLed, so the ioctls are numbed. And I need to let the coffee soak in before sending. > In which case this patch is fine - it is only patch 1 that requires > updating. > > Cheers, > Kent.
On Thu, 22 Feb 2024 02:05:30 +0100, Kent Gibson <warthog618@gmail.com> said: > On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote: >> On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: >> > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: >> >> ... >> >> > > } >> > > >> > > +static int linereq_unregistered_notify(struct notifier_block *nb, >> > > + unsigned long action, void *data) >> > > +{ >> > > + struct linereq *lr = container_of(nb, struct linereq, >> > > + device_unregistered_nb); >> > > + int i; >> > > + >> > > + for (i = 0; i < lr->num_lines; i++) { >> > > + if (lr->lines[i].desc) >> > > + edge_detector_stop(&lr->lines[i]); >> > > + } >> > > + >> > >> > Firstly, the re-ordering in the previous patch creates a race, >> > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so >> > there is now a window between the notifier being called and that numbing, >> > during which userspace may call linereq_set_config() and re-request >> > the irq. >> > >> > There is also a race here with linereq_set_config(). That can be prevented >> > by holding the lr->config_mutex - assuming the notifier is not being called >> > from atomic context. >> > >> >> It occurs to me that the fixed reordering in patch 1 would place >> the notifier call AFTER the NULLing of the ioctls, so there will no longer >> be any chance of a race with linereq_set_config() - so holding the >> config_mutex semaphore is not necessary. >> > > NULLing -> numbing > > The gdev->chip is NULLed, so the ioctls are numbed. > And I need to let the coffee soak in before sending. > >> In which case this patch is fine - it is only patch 1 that requires >> updating. >> >> Cheers, >> Kent. > The fix for the user-space issue may be more-or-less correct but the problem is deeper and this won't fix it for in-kernel users. Herve: please consider the following DT snippet: gpio0 { compatible = "foo"; gpio-controller; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <1>; ngpios = <8>; }; consumer { compatible = "bar"; interrupts-extended = <&gpio0 0>; }; If you unbind the "gpio0" device after the consumer requested the interrupt, you'll get the same splat. And device links will not help you here (on that note: Saravana: is there anything we could do about it? Have you even considered making the irqchip subsystem use the driver model in any way? Is it even feasible?). I would prefer this to be fixed at a lower lever than the GPIOLIB character device. Bartosz
Hi Bartosz, On Thu, 22 Feb 2024 00:31:08 -0800 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Thu, 22 Feb 2024 02:05:30 +0100, Kent Gibson <warthog618@gmail.com> said: > > On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote: > >> On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: > >> > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > >> > >> ... > >> > >> > > } > >> > > > >> > > +static int linereq_unregistered_notify(struct notifier_block *nb, > >> > > + unsigned long action, void *data) > >> > > +{ > >> > > + struct linereq *lr = container_of(nb, struct linereq, > >> > > + device_unregistered_nb); > >> > > + int i; > >> > > + > >> > > + for (i = 0; i < lr->num_lines; i++) { > >> > > + if (lr->lines[i].desc) > >> > > + edge_detector_stop(&lr->lines[i]); > >> > > + } > >> > > + > >> > > >> > Firstly, the re-ordering in the previous patch creates a race, > >> > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > >> > there is now a window between the notifier being called and that numbing, > >> > during which userspace may call linereq_set_config() and re-request > >> > the irq. > >> > > >> > There is also a race here with linereq_set_config(). That can be prevented > >> > by holding the lr->config_mutex - assuming the notifier is not being called > >> > from atomic context. > >> > > >> > >> It occurs to me that the fixed reordering in patch 1 would place > >> the notifier call AFTER the NULLing of the ioctls, so there will no longer > >> be any chance of a race with linereq_set_config() - so holding the > >> config_mutex semaphore is not necessary. > >> > > > > NULLing -> numbing > > > > The gdev->chip is NULLed, so the ioctls are numbed. > > And I need to let the coffee soak in before sending. > > > >> In which case this patch is fine - it is only patch 1 that requires > >> updating. > >> > >> Cheers, > >> Kent. > > > > The fix for the user-space issue may be more-or-less correct but the problem is > deeper and this won't fix it for in-kernel users. > > Herve: please consider the following DT snippet: > > gpio0 { > compatible = "foo"; > > gpio-controller; > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <1>; > ngpios = <8>; > }; > > consumer { > compatible = "bar"; > > interrupts-extended = <&gpio0 0>; > }; > > If you unbind the "gpio0" device after the consumer requested the interrupt, > you'll get the same splat. And device links will not help you here (on that > note: Saravana: is there anything we could do about it? Have you even > considered making the irqchip subsystem use the driver model in any way? Is it > even feasible?). > > I would prefer this to be fixed at a lower lever than the GPIOLIB character > device. I think this use case is covered. When the consumer device related to the consumer DT node is added, a consumer/supplier relationship is created: parse_interrupts() parses the 'interrups-extended' property https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 and so, of_link_to_phandle() creates the consumer/supplier link. https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 We that link present, if the supplier is removed, the consumer is removed before. The consumer should release the interrupt during its remove process (i.e explicit in its .remove() or explicit because of a devm_*() call). At least, it is my understanding. Best regards, Hervé
On Thu, Feb 22, 2024 at 12:36 PM Herve Codina <herve.codina@bootlin.com> wrote: > > Hi Bartosz, > > On Thu, 22 Feb 2024 00:31:08 -0800 > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Thu, 22 Feb 2024 02:05:30 +0100, Kent Gibson <warthog618@gmail.com> said: > > > On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote: > > >> On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: > > >> > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > > >> > > >> ... > > >> > > >> > > } > > >> > > > > >> > > +static int linereq_unregistered_notify(struct notifier_block *nb, > > >> > > + unsigned long action, void *data) > > >> > > +{ > > >> > > + struct linereq *lr = container_of(nb, struct linereq, > > >> > > + device_unregistered_nb); > > >> > > + int i; > > >> > > + > > >> > > + for (i = 0; i < lr->num_lines; i++) { > > >> > > + if (lr->lines[i].desc) > > >> > > + edge_detector_stop(&lr->lines[i]); > > >> > > + } > > >> > > + > > >> > > > >> > Firstly, the re-ordering in the previous patch creates a race, > > >> > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > > >> > there is now a window between the notifier being called and that numbing, > > >> > during which userspace may call linereq_set_config() and re-request > > >> > the irq. > > >> > > > >> > There is also a race here with linereq_set_config(). That can be prevented > > >> > by holding the lr->config_mutex - assuming the notifier is not being called > > >> > from atomic context. > > >> > > > >> > > >> It occurs to me that the fixed reordering in patch 1 would place > > >> the notifier call AFTER the NULLing of the ioctls, so there will no longer > > >> be any chance of a race with linereq_set_config() - so holding the > > >> config_mutex semaphore is not necessary. > > >> > > > > > > NULLing -> numbing > > > > > > The gdev->chip is NULLed, so the ioctls are numbed. > > > And I need to let the coffee soak in before sending. > > > > > >> In which case this patch is fine - it is only patch 1 that requires > > >> updating. > > >> > > >> Cheers, > > >> Kent. > > > > > > > The fix for the user-space issue may be more-or-less correct but the problem is > > deeper and this won't fix it for in-kernel users. > > > > Herve: please consider the following DT snippet: > > > > gpio0 { > > compatible = "foo"; > > > > gpio-controller; > > #gpio-cells = <2>; > > interrupt-controller; > > #interrupt-cells = <1>; > > ngpios = <8>; > > }; > > > > consumer { > > compatible = "bar"; > > > > interrupts-extended = <&gpio0 0>; > > }; > > > > If you unbind the "gpio0" device after the consumer requested the interrupt, > > you'll get the same splat. And device links will not help you here (on that > > note: Saravana: is there anything we could do about it? Have you even > > considered making the irqchip subsystem use the driver model in any way? Is it > > even feasible?). > > > > I would prefer this to be fixed at a lower lever than the GPIOLIB character > > device. > > I think this use case is covered. > When the consumer device related to the consumer DT node is added, a > consumer/supplier relationship is created: > parse_interrupts() parses the 'interrups-extended' property > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > and so, of_link_to_phandle() creates the consumer/supplier link. > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > We that link present, if the supplier is removed, the consumer is removed > before. > The consumer should release the interrupt during its remove process (i.e > explicit in its .remove() or explicit because of a devm_*() call). > > At least, it is my understanding. Well, then it doesn't work, because I literally just tried it before sending my previous email. Please try it yourself, you'll see. Also: an interrupt controller may not even have a device consuming its DT node (see IRQCHIP_DECLARE()), what happens then? Bart > > Best regards, > Hervé
On Thu, Feb 22, 2024 at 4:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Thu, Feb 22, 2024 at 12:36 PM Herve Codina <herve.codina@bootlin.com> wrote: > > > > Hi Bartosz, > > > > On Thu, 22 Feb 2024 00:31:08 -0800 > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > On Thu, 22 Feb 2024 02:05:30 +0100, Kent Gibson <warthog618@gmail.com> said: > > > > On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote: > > > >> On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote: > > > >> > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote: > > > >> > > > >> ... > > > >> > > > >> > > } > > > >> > > > > > >> > > +static int linereq_unregistered_notify(struct notifier_block *nb, > > > >> > > + unsigned long action, void *data) > > > >> > > +{ > > > >> > > + struct linereq *lr = container_of(nb, struct linereq, > > > >> > > + device_unregistered_nb); > > > >> > > + int i; > > > >> > > + > > > >> > > + for (i = 0; i < lr->num_lines; i++) { > > > >> > > + if (lr->lines[i].desc) > > > >> > > + edge_detector_stop(&lr->lines[i]); > > > >> > > + } > > > >> > > + > > > >> > > > > >> > Firstly, the re-ordering in the previous patch creates a race, > > > >> > as the NULLing of the gdev->chip serves to numb the cdev ioctls, so > > > >> > there is now a window between the notifier being called and that numbing, > > > >> > during which userspace may call linereq_set_config() and re-request > > > >> > the irq. > > > >> > > > > >> > There is also a race here with linereq_set_config(). That can be prevented > > > >> > by holding the lr->config_mutex - assuming the notifier is not being called > > > >> > from atomic context. > > > >> > > > > >> > > > >> It occurs to me that the fixed reordering in patch 1 would place > > > >> the notifier call AFTER the NULLing of the ioctls, so there will no longer > > > >> be any chance of a race with linereq_set_config() - so holding the > > > >> config_mutex semaphore is not necessary. > > > >> > > > > > > > > NULLing -> numbing > > > > > > > > The gdev->chip is NULLed, so the ioctls are numbed. > > > > And I need to let the coffee soak in before sending. > > > > > > > >> In which case this patch is fine - it is only patch 1 that requires > > > >> updating. > > > >> > > > >> Cheers, > > > >> Kent. > > > > > > > > > > The fix for the user-space issue may be more-or-less correct but the problem is > > > deeper and this won't fix it for in-kernel users. > > > > > > Herve: please consider the following DT snippet: > > > > > > gpio0 { > > > compatible = "foo"; > > > > > > gpio-controller; > > > #gpio-cells = <2>; > > > interrupt-controller; > > > #interrupt-cells = <1>; > > > ngpios = <8>; > > > }; > > > > > > consumer { > > > compatible = "bar"; > > > > > > interrupts-extended = <&gpio0 0>; > > > }; > > > > > > If you unbind the "gpio0" device after the consumer requested the interrupt, > > > you'll get the same splat. And device links will not help you here (on that > > > note: Saravana: is there anything we could do about it? Have you even > > > considered making the irqchip subsystem use the driver model in any way? Is it > > > even feasible?). I did add support to irqchip to use the driver model. See IRQCHIP_PLATFORM_DRIVER_BEGIN() and uses of it. So this makes sure the probe ordering is correct. But when I added that support, there was some pushback on making the modules removable[1]. But that's why you'll see that the IRQCHIP_PLATFORM_DRIVER_BEGIN() macro set .suppress_bind_attrs = true. Do you have a way to unregister an interrupt controller in your example? If so, how do you unregister it? It shouldn't be too hard to extend those macros to add removal support. We could add a IRQCHIP_MATCH2() that also takes in an exit() function op that gets called on device unbind. [1] - https://lore.kernel.org/lkml/86sghas7so.wl-maz@kernel.org/#t > > > > > > I would prefer this to be fixed at a lower lever than the GPIOLIB character > > > device. > > > > I think this use case is covered. > > When the consumer device related to the consumer DT node is added, a > > consumer/supplier relationship is created: > > parse_interrupts() parses the 'interrups-extended' property > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > and so, of_link_to_phandle() creates the consumer/supplier link. > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > > > We that link present, if the supplier is removed, the consumer is removed > > before. > > The consumer should release the interrupt during its remove process (i.e > > explicit in its .remove() or explicit because of a devm_*() call). > > > > At least, it is my understanding. > > Well, then it doesn't work, because I literally just tried it before > sending my previous email. For your gpio0 device, can you see why __device_release_driver() doesn't end up calling device_links_unbind_consumers()? Also, can you look at /sys/class/devlink/<bus:gpio0-devicename>--<consumer device name> folders and see what the status file says before you try to unbind the gpio0 device? It should say "active". > Please try it yourself, you'll see. > > Also: an interrupt controller may not even have a device consuming its > DT node (see IRQCHIP_DECLARE()), what happens then? Yeah, we are screwed in those cases. Ideally we are rejecting all submissions for irqchip drivers that use IRQCHIP_DECLARE(). -Saravana
Hi Bartosz On Thu, 22 Feb 2024 15:51:15 -0800 Saravana Kannan <saravanak@google.com> wrote: > On Thu, Feb 22, 2024 at 4:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > ... > > > > > > > > The fix for the user-space issue may be more-or-less correct but the problem is > > > > deeper and this won't fix it for in-kernel users. > > > > > > > > Herve: please consider the following DT snippet: > > > > > > > > gpio0 { > > > > compatible = "foo"; > > > > > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > interrupt-controller; > > > > #interrupt-cells = <1>; > > > > ngpios = <8>; > > > > }; > > > > > > > > consumer { > > > > compatible = "bar"; > > > > > > > > interrupts-extended = <&gpio0 0>; > > > > }; > > > > > > > > If you unbind the "gpio0" device after the consumer requested the interrupt, > > > > you'll get the same splat. And device links will not help you here (on that > > > > note: Saravana: is there anything we could do about it? Have you even > > > > considered making the irqchip subsystem use the driver model in any way? Is it > > > > even feasible?). > > I did add support to irqchip to use the driver model. See > IRQCHIP_PLATFORM_DRIVER_BEGIN() and uses of it. So this makes sure > the probe ordering is correct. > > But when I added that support, there was some pushback on making the > modules removable[1]. But that's why you'll see that the > IRQCHIP_PLATFORM_DRIVER_BEGIN() macro set .suppress_bind_attrs = true. > > Do you have a way to unregister an interrupt controller in your > example? If so, how do you unregister it? It shouldn't be too hard to > extend those macros to add removal support. We could add a > IRQCHIP_MATCH2() that also takes in an exit() function op that gets > called on device unbind. > > [1] - https://lore.kernel.org/lkml/86sghas7so.wl-maz@kernel.org/#t > > > > > > > > > I would prefer this to be fixed at a lower lever than the GPIOLIB character > > > > device. > > > > > > I think this use case is covered. > > > When the consumer device related to the consumer DT node is added, a > > > consumer/supplier relationship is created: > > > parse_interrupts() parses the 'interrups-extended' property > > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > > and so, of_link_to_phandle() creates the consumer/supplier link. > > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > > > > > We that link present, if the supplier is removed, the consumer is removed > > > before. > > > The consumer should release the interrupt during its remove process (i.e > > > explicit in its .remove() or explicit because of a devm_*() call). > > > > > > At least, it is my understanding. > > > > Well, then it doesn't work, because I literally just tried it before > > sending my previous email. > > For your gpio0 device, can you see why __device_release_driver() > doesn't end up calling device_links_unbind_consumers()? > > Also, can you look at > /sys/class/devlink/<bus:gpio0-devicename>--<consumer device name> > folders and see what the status file says before you try to unbind the > gpio0 device? It should say "active". > > > Please try it yourself, you'll see. > > > > Also: an interrupt controller may not even have a device consuming its > > DT node (see IRQCHIP_DECLARE()), what happens then? > > Yeah, we are screwed in those cases. Ideally we are rejecting all > submissions for irqchip drivers that use IRQCHIP_DECLARE(). > I have the feeling that this issue related to your gpio0 driver unbind is out of the scope of this series. Let move forward with the user-space fix (cdev) related to this series. I will sent the v2 to cover the cdev case. Regards, Hervé
On Fri, Feb 23, 2024 at 12:51 AM Saravana Kannan <saravanak@google.com> wrote: > [snip] > > > > > > > > If you unbind the "gpio0" device after the consumer requested the interrupt, > > > > you'll get the same splat. And device links will not help you here (on that > > > > note: Saravana: is there anything we could do about it? Have you even > > > > considered making the irqchip subsystem use the driver model in any way? Is it > > > > even feasible?). > > I did add support to irqchip to use the driver model. See > IRQCHIP_PLATFORM_DRIVER_BEGIN() and uses of it. So this makes sure > the probe ordering is correct. > > But when I added that support, there was some pushback on making the > modules removable[1]. But that's why you'll see that the > IRQCHIP_PLATFORM_DRIVER_BEGIN() macro set .suppress_bind_attrs = true. > > Do you have a way to unregister an interrupt controller in your > example? If so, how do you unregister it? It shouldn't be too hard to > extend those macros to add removal support. We could add a > IRQCHIP_MATCH2() that also takes in an exit() function op that gets > called on device unbind. > The provider in my example is a GPIO chip that registers its own IRQ domain. The domain is destroyed when the GPIO controller is unregistered but interrupts can be requested orthogonally to the GPIO subsystem and we don't have the knowledge about any interrupt users as the .to_irq() callback is never called. Let me know if anything can be done in this case. I used the gpio-sim testing module for it (instantiated over DT) but any such GPIO chip that is also an interrupt-controller would behave the same. > [1] - https://lore.kernel.org/lkml/86sghas7so.wl-maz@kernel.org/#t > > > > > > > > > I would prefer this to be fixed at a lower lever than the GPIOLIB character > > > > device. > > > > > > I think this use case is covered. > > > When the consumer device related to the consumer DT node is added, a > > > consumer/supplier relationship is created: > > > parse_interrupts() parses the 'interrups-extended' property > > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > > and so, of_link_to_phandle() creates the consumer/supplier link. > > > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#L1316 > > > > > > We that link present, if the supplier is removed, the consumer is removed > > > before. > > > The consumer should release the interrupt during its remove process (i.e > > > explicit in its .remove() or explicit because of a devm_*() call). > > > > > > At least, it is my understanding. > > > > Well, then it doesn't work, because I literally just tried it before > > sending my previous email. > > For your gpio0 device, can you see why __device_release_driver() > doesn't end up calling device_links_unbind_consumers()? > It never gets into the while (device_links_busy(dev)) loop. > Also, can you look at > /sys/class/devlink/<bus:gpio0-devicename>--<consumer device name> > folders and see what the status file says before you try to unbind the > gpio0 device? It should say "active". > It says "available". If you have some board supported upstream you could use for testing, I think I could prepare you a DT snippet for testing. Bart [snip]
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 2a88736629ef..aec4a4c8490a 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -688,17 +688,6 @@ static void line_set_debounce_period(struct line *line, GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \ GPIO_V2_LINE_EDGE_FLAGS) -static int linereq_unregistered_notify(struct notifier_block *nb, - unsigned long action, void *data) -{ - struct linereq *lr = container_of(nb, struct linereq, - device_unregistered_nb); - - wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR); - - return NOTIFY_OK; -} - static void linereq_put_event(struct linereq *lr, struct gpio_v2_line_event *le) { @@ -1189,6 +1178,23 @@ static int edge_detector_update(struct line *line, return edge_detector_setup(line, lc, line_idx, edflags); } +static int linereq_unregistered_notify(struct notifier_block *nb, + unsigned long action, void *data) +{ + struct linereq *lr = container_of(nb, struct linereq, + device_unregistered_nb); + int i; + + for (i = 0; i < lr->num_lines; i++) { + if (lr->lines[i].desc) + edge_detector_stop(&lr->lines[i]); + } + + wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR); + + return NOTIFY_OK; +} + static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc, unsigned int line_idx) { @@ -1898,6 +1904,11 @@ static int lineevent_unregistered_notify(struct notifier_block *nb, struct lineevent_state *le = container_of(nb, struct lineevent_state, device_unregistered_nb); + if (le->irq) { + free_irq(le->irq, le); + le->irq = 0; + } + wake_up_poll(&le->wait, EPOLLIN | EPOLLERR); return NOTIFY_OK;
When gpio chip device is removed while some related gpio are used by the user-space, the following warning can appear: remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c ... Call trace: remove_proc_entry+0x190/0x19c unregister_irq_proc+0xd0/0x104 free_desc+0x4c/0xc4 irq_free_descs+0x6c/0x90 irq_dispose_mapping+0x104/0x14c gpiochip_irqchip_remove+0xcc/0x1a4 gpiochip_remove+0x48/0x100 ... Indeed, the gpio cdev uses an IRQ but this IRQ is not released when the gpio chip device is removed. Release IRQs used in the device removal notifier functions. Also move one of these function definition in order to avoid a forward declaration (move after the edge_detector_stop() definition). Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)