Re: [PATCH 2/2] genirq: proc: fix a procfs entry leak

[Bartosz Golaszewski]

On Tue, Sep 12, 2023 at 8:17 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 06 2023 at 16:54, Bartosz Golaszewski wrote:
> > On Wed, Aug 30, 2023 at 12:29 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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
>