From: Bartosz Golaszewski <[email protected]>
When we remove the procfs entry for an irq desc that's still in use, we
leak the procfs entries created per handler. We need to go through the
irqaction chain and remove all entries before finally removing the irq's
top procfs directory.
First patch drops an unused argument from unregister_handler_proc(), the
second fixes the actual leak.
Bartosz Golaszewski (2):
genirq: proc: drop unused argument from unregister_handler_proc()
genirq: proc: fix a procfs entry leak
kernel/irq/internals.h | 5 ++---
kernel/irq/manage.c | 6 +++---
kernel/irq/proc.c | 17 ++++++++++++++++-
3 files changed, 21 insertions(+), 7 deletions(-)
--
2.39.2
From: Bartosz Golaszewski <[email protected]>
When removing the proc entry for a desc that still has active users, we
will leak the irqaction entries. Let's remove them in
unregister_irq_proc().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
kernel/irq/proc.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 83ed403991c6..b284604a091a 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -390,6 +390,15 @@ void register_irq_proc(unsigned int irq, struct irq_desc *desc)
mutex_unlock(®ister_lock);
}
+static void unregister_action_proc(struct irqaction *action)
+{
+ if (!action)
+ return;
+
+ unregister_action_proc(action->secondary);
+ unregister_handler_proc(action);
+}
+
void unregister_irq_proc(unsigned int irq, struct irq_desc *desc)
{
char name [MAX_NAMELEN];
@@ -408,6 +417,12 @@ void unregister_irq_proc(unsigned int irq, struct irq_desc *desc)
#endif
remove_proc_entry("spurious", desc->dir);
+ /*
+ * If at this point, this irq desc is still requested, we need to
+ * remove the proc handler entries or we'll leak them.
+ */
+ unregister_action_proc(desc->action);
+
sprintf(name, "%u", irq);
remove_proc_entry(name, root_irq_dir);
}
--
2.39.2
From: Bartosz Golaszewski <[email protected]>
The irq argument is unused. Drop it.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
kernel/irq/internals.h | 5 ++---
kernel/irq/manage.c | 6 +++---
kernel/irq/proc.c | 2 +-
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index bdd35bb9c735..eee0e27e6750 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -125,14 +125,13 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action);
extern void register_irq_proc(unsigned int irq, struct irq_desc *desc);
extern void unregister_irq_proc(unsigned int irq, struct irq_desc *desc);
extern void register_handler_proc(unsigned int irq, struct irqaction *action);
-extern void unregister_handler_proc(unsigned int irq, struct irqaction *action);
+extern void unregister_handler_proc(struct irqaction *action);
#else
static inline void register_irq_proc(unsigned int irq, struct irq_desc *desc) { }
static inline void unregister_irq_proc(unsigned int irq, struct irq_desc *desc) { }
static inline void register_handler_proc(unsigned int irq,
struct irqaction *action) { }
-static inline void unregister_handler_proc(unsigned int irq,
- struct irqaction *action) { }
+static inline void unregister_handler_proc(struct irqaction *action) { }
#endif
extern bool irq_can_set_affinity_usr(unsigned int irq);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index d2742af0f0fd..7ed8a151ded8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1937,7 +1937,7 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
*/
chip_bus_sync_unlock(desc);
- unregister_handler_proc(irq, action);
+ unregister_handler_proc(action);
/*
* Make sure it's not being used on another CPU and if the chip
@@ -2056,7 +2056,7 @@ static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc)
if (!WARN_ON(desc->action == NULL)) {
irq_pm_remove_action(desc, desc->action);
devname = desc->action->name;
- unregister_handler_proc(irq, desc->action);
+ unregister_handler_proc(desc->action);
kfree(desc->action);
desc->action = NULL;
@@ -2487,7 +2487,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
raw_spin_unlock_irqrestore(&desc->lock, flags);
- unregister_handler_proc(irq, action);
+ unregister_handler_proc(action);
irq_chip_pm_put(&desc->irq_data);
module_put(desc->owner);
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af..83ed403991c6 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -414,7 +414,7 @@ void unregister_irq_proc(unsigned int irq, struct irq_desc *desc)
#undef MAX_NAMELEN
-void unregister_handler_proc(unsigned int irq, struct irqaction *action)
+void unregister_handler_proc(struct irqaction *action)
{
proc_remove(action->dir);
}
--
2.39.2
On Wed, Sep 06 2023 at 16:54, Bartosz Golaszewski wrote:
> On Wed, Aug 30, 2023 at 12:29 AM Thomas Gleixner <[email protected]> wrote:
>> usb disconnect
>> ...
>> cp2112_remove()
>> i2c_del_adapter()
>> i2c_unregister_device(client)
>> ...
>> device_unregister()
>> device_del()
>> bus_notify() // Mechanism #1
>> i2c_device_remove()
>> if (dev->remove)
>> dev->remove()
>> ...
>> device_unbind_cleanup()
>> devres_release_all() // Mechanism #2
>>
>> gpiochip_remove()
>>
>> There are very well notifications to the drivers about unplug of a
>> device. Otherwise this would end up in a complete disaster and a lot
>> more stale data and state than just a procfs file or a requested
>> interrupt.
>
> I'm not sure how either of the two helps here. #2 just releases
> managed resources owned by cp2112. It can remove the domain with an
> appropriate devm action but it won't do anything for the users of
> interrupts. #1 is a bus notification emitted when the I2C adapter
> exposed by cp2112 has been deleted.
No. The domain is not yet gone at the point where the I2C bus
notification happens. Look at the above invocation chain.
The removal of the attached I2C devices happens _before_ the domain is
removed. Anything else does not make sense at all.
So the cleanup of those devices should free the interrupt, in the same
way it frees other resources, no?
i2c_device_remove()
if (driver->remove)
driver->remove() // Driver specific cleanup
// Devres cleanup operating on the to be removed I2C device
devres_release_group(&client->dev, client->devres_group_id);
So again:
cp2112_remove()
i2c_del_adapter() // Cleans up all I2C users
gpiochip_remove() // Removes the interrupt domain.
So you do not need any magic bus notififications and whatever. It's all
there already.
> This one in particular doesn't help us, the domain is long gone by now
> but if I get what you mean correctly, you'd want the driver to call
> request_irq() and then set up a notifier block for the
> BUS_NOTIFY_UNBIND_DRIVER notification of the provider of that
> interrupt? Doesn't that break like half a dozen of abstraction layers?
> Because now the device driver which is the GPIO consumer needs to know
> where it gets its interrupts from?
Again. It does not. The point is that the device is removed in the
hotplug event chain, which cleans up the associated resources.
devm_request_irq() already takes care of that.
> You would think that plug-and-play works well in the kernel and it's
> true for certain parts but it really isn't the case for subsystems
> that were not considered as very plug-and-play until people started
> putting them on a stick. Some devices that are not typically
> hot-pluggable - like serial - have been used with USB for so long that
> they do handle unplugging very well. But as soon as you put i2c on
> USB, you'll see what a mess it is. If you have an I2C device on a USB
> I2C expander and it's being used, when you pull the plug, you'll see
> that the kernel thread removing the device will block on a call to
> wait_for_completion() until all users release their i2c adapter
> references. They (the users) are not however notified in any generic
> way about the provider of their resources being gone.
So why aren't you fixing this and instead trying to implement force
unplug mechanisms which require a pile of unholy hacks all over the
place?
>> All hotpluggable consumers have at least one mechanism to mop up the
>> resources they allocated. There are a lot of resources in the kernel
>> which do not clean themself up magically.
>>
>
> Yeah, hotpluggable consumers are fine. The problem here is
> hotpluggable *providers* with consumers who don't know that.
Then these consumers have to be fixed and made aware of the new world order
of hotplug, no?
>> Your idea of tracking request_irq()/free_irq() at some subsystem level
>> is not going to work either simply because it requires that such muck is
>> sprinkled all over the place.
>>
> I was thinking more about tracking it at the irq domain level so that
> when a domain is destroyed with interrupts requested, these interrupts
> are freed. I admit I still don't have enough in-depth knowledge about
> linux interrupts to understand why it can't work, I need to spend
> more time on this. I'll be back.
There is no need for special tracking. The core can figure out today
whether an interrupt which is mapped by the domain is requested or
not. That's not the problem at all.
The problems are the life time rules, references, concurrency etc. They
are not magically going away by some new form of tracking.
It's amazing that you insist on solving the problem at the wrong end.
The real problem is that there are device drivers and subsystems which
are not prepared for hotplug, right?
As interrupts are only a small part of the overall problem, I'm
absolutely not seeing how adding heuristics all over the place is a
sensible design principle.
What's so problematic about teaching the affected subsystems and drivers
that hotplug exists?
Thanks,
tglx
On Tue, Sep 12, 2023 at 8:17 PM Thomas Gleixner <[email protected]> wrote:
>
> On Wed, Sep 06 2023 at 16:54, Bartosz Golaszewski wrote:
> > On Wed, Aug 30, 2023 at 12:29 AM Thomas Gleixner <[email protected]> wrote:
> >> usb disconnect
> >> ...
> >> cp2112_remove()
> >> i2c_del_adapter()
> >> i2c_unregister_device(client)
> >> ...
> >> device_unregister()
> >> device_del()
> >> bus_notify() // Mechanism #1
> >> i2c_device_remove()
> >> if (dev->remove)
> >> dev->remove()
> >> ...
> >> device_unbind_cleanup()
> >> devres_release_all() // Mechanism #2
> >>
> >> gpiochip_remove()
> >>
> >> There are very well notifications to the drivers about unplug of a
> >> device. Otherwise this would end up in a complete disaster and a lot
> >> more stale data and state than just a procfs file or a requested
> >> interrupt.
> >
> > I'm not sure how either of the two helps here. #2 just releases
> > managed resources owned by cp2112. It can remove the domain with an
> > appropriate devm action but it won't do anything for the users of
> > interrupts. #1 is a bus notification emitted when the I2C adapter
> > exposed by cp2112 has been deleted.
>
> No. The domain is not yet gone at the point where the I2C bus
> notification happens. Look at the above invocation chain.
>
> The removal of the attached I2C devices happens _before_ the domain is
> removed. Anything else does not make sense at all.
>
> So the cleanup of those devices should free the interrupt, in the same
> way it frees other resources, no?
>
> i2c_device_remove()
> if (driver->remove)
> driver->remove() // Driver specific cleanup
>
> // Devres cleanup operating on the to be removed I2C device
> devres_release_group(&client->dev, client->devres_group_id);
>
> So again:
>
> cp2112_remove()
> i2c_del_adapter() // Cleans up all I2C users
>
> gpiochip_remove() // Removes the interrupt domain.
>
> So you do not need any magic bus notififications and whatever. It's all
> there already.
>
You're only talking about a situation in which the users of the
interrupts from the cp2112 GPIO chip's are I2C devices on its I2C
adapter. We can have consumers of those interrupts elsewhere. We can
have user-space watching interrupts on GPIOs (see below). They won't
get removed before the cp2112 GPIO chip, so their remove paths freeing
interrupts will not be triggered as you describe it.
> > This one in particular doesn't help us, the domain is long gone by now
> > but if I get what you mean correctly, you'd want the driver to call
> > request_irq() and then set up a notifier block for the
> > BUS_NOTIFY_UNBIND_DRIVER notification of the provider of that
> > interrupt? Doesn't that break like half a dozen of abstraction layers?
> > Because now the device driver which is the GPIO consumer needs to know
> > where it gets its interrupts from?
>
> Again. It does not. The point is that the device is removed in the
> hotplug event chain, which cleans up the associated resources.
> devm_request_irq() already takes care of that.
>
That's not always necessary. Imagine you have an interrupt handler set
up on a GPIO that is now gone. Your driver may do lots of other things
and remain functional even though this interrupt will never fire.
> > You would think that plug-and-play works well in the kernel and it's
> > true for certain parts but it really isn't the case for subsystems
> > that were not considered as very plug-and-play until people started
> > putting them on a stick. Some devices that are not typically
> > hot-pluggable - like serial - have been used with USB for so long that
> > they do handle unplugging very well. But as soon as you put i2c on
> > USB, you'll see what a mess it is. If you have an I2C device on a USB
> > I2C expander and it's being used, when you pull the plug, you'll see
> > that the kernel thread removing the device will block on a call to
> > wait_for_completion() until all users release their i2c adapter
> > references. They (the users) are not however notified in any generic
> > way about the provider of their resources being gone.
>
> So why aren't you fixing this and instead trying to implement force
> unplug mechanisms which require a pile of unholy hacks all over the
> place?
>
That's not what I'm suggesting. I've described the general model I'm
postulating. If this patch is an unholy hack, it's because I didn't
know better. Now I do, I've abandoned it two weeks ago.
> >> All hotpluggable consumers have at least one mechanism to mop up the
> >> resources they allocated. There are a lot of resources in the kernel
> >> which do not clean themself up magically.
> >>
> >
> > Yeah, hotpluggable consumers are fine. The problem here is
> > hotpluggable *providers* with consumers who don't know that.
>
> Then these consumers have to be fixed and made aware of the new world order
> of hotplug, no?
>
I've asked that question in my previous email. What do you think we
should do when a provider of a resource we're using in a driver is
gone? Let's assume, the consumer device will not get removed in the
hot-unplug chain - which is perfectly reasonable. I'm arguing that it
should receive an error code on the next call to that provider. The
alternatives I see are: force-unbind the device or notify it by some
other unspecified means and have it do what exactly?
> >> Your idea of tracking request_irq()/free_irq() at some subsystem level
> >> is not going to work either simply because it requires that such muck is
> >> sprinkled all over the place.
> >>
> > I was thinking more about tracking it at the irq domain level so that
> > when a domain is destroyed with interrupts requested, these interrupts
> > are freed. I admit I still don't have enough in-depth knowledge about
> > linux interrupts to understand why it can't work, I need to spend
> > more time on this. I'll be back.
>
> There is no need for special tracking. The core can figure out today
> whether an interrupt which is mapped by the domain is requested or
> not. That's not the problem at all.
>
> The problems are the life time rules, references, concurrency etc. They
> are not magically going away by some new form of tracking.
>
> It's amazing that you insist on solving the problem at the wrong end.
>
Is it really the wrong end though? Let me give you an analogy with a
driver consuming a resource replaced by a user-space process. Let's
take a user process requesting some kernel resource by opening a
character device file. The handle the process gets will now be the
file descriptor number. The resource can be a GPIO (or even an
interrupt on that GPIO - as user-space can watch interrupts via the
GPIO character device).
Let's now assume the GPIO is on a USB expander. We now unplug it.
Should the user-space get informed about this fact with some
side-channel other than the descriptor? Or sent a signal/killed
(analogy to the removal of the device in the hot-unplug path)? Should
we set up some entirely different notification mechanism? No, the only
sane thing to do is: next time the process calls into the kernel via a
system call referencing that descriptor, it should return an error -
typically -ENODEV. If a poll() is in process, it should be woken up
with EPOLLERR. The process should then call close() on that
descriptor, putting its reference to the resource. If it doesn't, then
all it'll see will be errors. The process however can keep on living
and doing other stuff.
What should happen in the kernel is: on the unplug event we clean
everything up, leaving just the user-facing, reference-counted outer
shell. Once the last reference to struct file is put, it'll be
released. Of course not everyone does it and so user-space can crash
the kernel just by opening a character device exposed by vulnerable
subsystems, unbinding the device over sysfs and calling ioctl() or
otherwise.
My point is: the same rule should apply to in-kernel consumers. When
they request a resource, they get a reference to it. The resource is
managed by its provider. If the provider is going down, it frees the
resource. The consumer tries to use it -> it gets an error. I'm not
convinced by the life-time rules argument. The consumer is not
CREATING a resource. It's REQUESTING it for usage. IMO this means it
REFERENCES it, not OWNS it. And so is only responsible for putting the
reference.
Bartosz
> The real problem is that there are device drivers and subsystems which
> are not prepared for hotplug, right?
>
> As interrupts are only a small part of the overall problem, I'm
> absolutely not seeing how adding heuristics all over the place is a
> sensible design principle.
>
> What's so problematic about teaching the affected subsystems and drivers
> that hotplug exists?
>
> Thanks,
>
> tglx
>