Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Sameer Pujar]

On 3/13/2019 4:52 PM, Marc Zyngier wrote:
> First things first:
>
> - Where is the cover letter?
> - This series should be flagged as v2, as it not the same as the one you
> sent last week.
I had the dilemma whether to name this series as v2 or not, thought the 
commits
in the series are different and v2 may not be necessary.
Also felt commit messages are descriptive enough and all belong to 
irq-gic-pm,
hence did not send cover letter.
If you suggest so, I will send a cover letter next patch version(v2)
>
> On 13/03/2019 11:02, Sameer Pujar wrote:
>> This is a preparatory patch for using irq-gic-pm driver as module and thus
>> implement remove() call for the driver. Details of remove() are as below,
>>
>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
>>   * Function gic_teardown() is exported from gic driver and called in remove
>>     to perform io unmap.
>>   * pm_clk_destroy() to free clock resources
>>   * irq is unmapped and freed with irq_dispose_mapping()
>>
> Let's be clear, I have no desire to export any GIC symbol at all. Why
> should we do this? This "driver" is the tiniest thing, and making it
> modular doesn't get us anything.
>
> So what's the rational for doing so?
Reason for this was, the driver gets used for AGIC block and audio is not
boot critical and hence module option was preferred.

Thanks,
Sameer.

>
> Thanks,
>
> 	M.

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

On 13/03/2019 13:50, Sameer Pujar wrote:
> 
> On 3/13/2019 4:52 PM, Marc Zyngier wrote:
>> First things first:
>>
>> - Where is the cover letter?
>> - This series should be flagged as v2, as it not the same as the one you
>> sent last week.
> I had the dilemma whether to name this series as v2 or not, thought the 
> commits
> in the series are different and v2 may not be necessary.

This is an iteration on the same theme. Please always bump up the
counter. Better do it more often than not.

> Also felt commit messages are descriptive enough and all belong to 
> irq-gic-pm,
> hence did not send cover letter.
> If you suggest so, I will send a cover letter next patch version(v2)

You should always send a cover letter if you have more than a single patch.

>>
>> On 13/03/2019 11:02, Sameer Pujar wrote:
>>> This is a preparatory patch for using irq-gic-pm driver as module and thus
>>> implement remove() call for the driver. Details of remove() are as below,
>>>
>>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
>>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
>>>   * Function gic_teardown() is exported from gic driver and called in remove
>>>     to perform io unmap.
>>>   * pm_clk_destroy() to free clock resources
>>>   * irq is unmapped and freed with irq_dispose_mapping()
>>>
>> Let's be clear, I have no desire to export any GIC symbol at all. Why
>> should we do this? This "driver" is the tiniest thing, and making it
>> modular doesn't get us anything.
>>
>> So what's the rational for doing so?
> Reason for this was, the driver gets used for AGIC block and audio is not
> boot critical and hence module option was preferred.

Sure, but look at the result:

- you remove your gic-pm module
- the MMIO mapping disappears
- the GIC data structures *are still live*
- a driver does a disable_irq() on an interrupt routed to this block
(because nothing has taken the interrupts away, as far as the kernel is
concerned)
- ...
- profit! (or kernel panic, your choice)

Even better if something else in the system has mapped anything that
ends up in the same vmalloc range. Congratulations, you have now
corrupted unsuspecting memory. This reminds me of the e1000 corruption
bug. Great stuff.

So for the whole thing, NAK. You don't pull an irqchip from under the
kernel's feet.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:
> On 13/03/2019 13:50, Sameer Pujar wrote:
> > 
> > On 3/13/2019 4:52 PM, Marc Zyngier wrote:
> >> First things first:
> >>
> >> - Where is the cover letter?
> >> - This series should be flagged as v2, as it not the same as the one you
> >> sent last week.
> > I had the dilemma whether to name this series as v2 or not, thought the 
> > commits
> > in the series are different and v2 may not be necessary.
> 
> This is an iteration on the same theme. Please always bump up the
> counter. Better do it more often than not.
> 
> > Also felt commit messages are descriptive enough and all belong to 
> > irq-gic-pm,
> > hence did not send cover letter.
> > If you suggest so, I will send a cover letter next patch version(v2)
> 
> You should always send a cover letter if you have more than a single patch.
> 
> >>
> >> On 13/03/2019 11:02, Sameer Pujar wrote:
> >>> This is a preparatory patch for using irq-gic-pm driver as module and thus
> >>> implement remove() call for the driver. Details of remove() are as below,
> >>>
> >>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
> >>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
> >>>   * Function gic_teardown() is exported from gic driver and called in remove
> >>>     to perform io unmap.
> >>>   * pm_clk_destroy() to free clock resources
> >>>   * irq is unmapped and freed with irq_dispose_mapping()
> >>>
> >> Let's be clear, I have no desire to export any GIC symbol at all. Why
> >> should we do this? This "driver" is the tiniest thing, and making it
> >> modular doesn't get us anything.
> >>
> >> So what's the rational for doing so?
> > Reason for this was, the driver gets used for AGIC block and audio is not
> > boot critical and hence module option was preferred.
> 
> Sure, but look at the result:
> 
> - you remove your gic-pm module
> - the MMIO mapping disappears
> - the GIC data structures *are still live*
> - a driver does a disable_irq() on an interrupt routed to this block
> (because nothing has taken the interrupts away, as far as the kernel is
> concerned)
> - ...
> - profit! (or kernel panic, your choice)

I suppose we could use device links to model the dependency, but perhaps
it's not worth it for something like the GIC. The AGIC is somewhat of an
outlier because it serves a fairly encapsulated part of the system, so
it would be manageable to make it unloadable.

> Even better if something else in the system has mapped anything that
> ends up in the same vmalloc range. Congratulations, you have now
> corrupted unsuspecting memory. This reminds me of the e1000 corruption
> bug. Great stuff.

Can you elaborate on this point? How would unloading a driver cause
memory corruption in another driver's mapped memory? I've never heard of
this before, so I want to make sure I understand what to look out for in
the future.

> So for the whole thing, NAK. You don't pull an irqchip from under the
> kernel's feet.

Maybe we should also set the driver's .suppress_bind_attrs = true while
at it, to prevent anyone from trying to force unbind the driver via
sysfs?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

Hi Thierry,

On 13/03/2019 16:34, Thierry Reding wrote:
> On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:

[...]

>> Sure, but look at the result:
>>
>> - you remove your gic-pm module
>> - the MMIO mapping disappears
>> - the GIC data structures *are still live*
>> - a driver does a disable_irq() on an interrupt routed to this block
>> (because nothing has taken the interrupts away, as far as the kernel is
>> concerned)
>> - ...
>> - profit! (or kernel panic, your choice)
> 
> I suppose we could use device links to model the dependency, but perhaps
> it's not worth it for something like the GIC. The AGIC is somewhat of an
> outlier because it serves a fairly encapsulated part of the system, so
> it would be manageable to make it unloadable.

The problem is that the GIC is required so early that it cannot be a
real device (devices require sysfs, which requires the VFS, which has
indirect dependencies in the scheduler, which needs the timer, which
needs interrupts -- I've been pondering turning that upside down for
years, but life is too short).

> 
>> Even better if something else in the system has mapped anything that
>> ends up in the same vmalloc range. Congratulations, you have now
>> corrupted unsuspecting memory. This reminds me of the e1000 corruption
>> bug. Great stuff.
> 
> Can you elaborate on this point? How would unloading a driver cause
> memory corruption in another driver's mapped memory? I've never heard of
> this before, so I want to make sure I understand what to look out for in
> the future.

The MMIO ranges get their VA as part of the vmalloc space. If you free
that space (iounmap), you allow it to be reused by another mapping if
the hole you've created fits the new ioremap. Now the GIC driver (which,
remember, still has references to that VA space all over the place)
unexpectedly pokes into another driver's MMIO space.

But it can be worse. You could have loaded a module instead, which also
ends up in the vmalloc region. And now the GIC driver is writing to the
module code or data. Or anything that gets allocated using vmalloc.

> 
>> So for the whole thing, NAK. You don't pull an irqchip from under the
>> kernel's feet.
> 
> Maybe we should also set the driver's .suppress_bind_attrs = true while
> at it, to prevent anyone from trying to force unbind the driver via
> sysfs?

Would that prevent a 'modprobe -r'? With the current approach, anything
that allows the driver to be removed is potentially fatal.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...