[PATCH 1/1] gic: its: Make sure a LPI is discarded before free.

[PATCH 1/1] gic: its: Make sure a LPI is discarded before free.

[Zhao Yuanyuan]

Its device will be removed after all events be freed.
Undisarded events can lead to unpredictable behaviar.

Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index db20e99..4fee008 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq,
                                                                virq + i);
                u32 event = its_get_event_id(data);

+               /* Discard irq before free */
+               if (irqd_is_activated(d))
+                       its_send_discard(its_dev, event);
+
                /* Mark interrupt index as unused */
                clear_bit(event, its_dev->event_map.lpi_map);

--
1.8.3.1




This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you.

[RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Zhao Yuanyuan]

Its device will be removed after all events be freed.
Undisarded events can lead to unpredictable behaviar.

Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index db20e99..4fee008 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq,
 								virq + i);
 		u32 event = its_get_event_id(data);
 
+		/* Discard irq before free */
+		if (irqd_is_activated(d))
+			its_send_discard(its_dev, event);
+
 		/* Mark interrupt index as unused */
 		clear_bit(event, its_dev->event_map.lpi_map);
 
-- 
1.8.3.1

Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Marc Zyngier]

On Wed, 9 Jan 2019 11:53:27 +0800
Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> wrote:

Hi Zhao,

> Its device will be removed after all events be freed.
> Undisarded events can lead to unpredictable behaviar.
> 
> Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index db20e99..4fee008 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq,
>  								virq + i);
>  		u32 event = its_get_event_id(data);
>  
> +		/* Discard irq before free */
> +		if (irqd_is_activated(d))
> +			its_send_discard(its_dev, event);
> +
>  		/* Mark interrupt index as unused */
>  		clear_bit(event, its_dev->event_map.lpi_map);
>  

But we already do send a discard on deactivate, which logically happens
before we free the domain. So what are you fixing here?

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

RE: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Zhao, Yuanyuan]

Hi Marc:

Thank you for your reply. 

As you said, APIs such as free_irq will deactivate irq
before free it. But deactivation is not forced by every API,
for example irq_dispose_mapping.  So I think it's better to check 
that irq was deactivated as expected.

BRs,
Yuanyuan


> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: 2019年1月9日 15:43
> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, Joey
> <yu.zheng@hxt-semitech.com>; Wang, Dongsheng <dongsheng.wang@hxt-
> semitech.com>
> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.
> 
> On Wed, 9 Jan 2019 11:53:27 +0800
> Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> wrote:
> 
> Hi Zhao,
> 
> > Its device will be removed after all events be freed.
> > Undisarded events can lead to unpredictable behaviar.
> >
> > Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c
> > b/drivers/irqchip/irq-gic-v3-its.c
> > index db20e99..4fee008 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct
> irq_domain *domain, unsigned int virq,
> >  								virq + i);
> >  		u32 event = its_get_event_id(data);
> >
> > +		/* Discard irq before free */
> > +		if (irqd_is_activated(d))
> > +			its_send_discard(its_dev, event);
> > +
> >  		/* Mark interrupt index as unused */
> >  		clear_bit(event, its_dev->event_map.lpi_map);
> >
> 
> But we already do send a discard on deactivate, which logically happens
> before we free the domain. So what are you fixing here?
> 
> Thanks,
> 
> 	M.
> --
> Without deviation from the norm, progress is not possible.

Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Marc Zyngier]

On 09/01/2019 09:29, Zhao, Yuanyuan wrote:
> Hi Marc:
> 
> Thank you for your reply. 
> 
> As you said, APIs such as free_irq will deactivate irq
> before free it. But deactivation is not forced by every API,
> for example irq_dispose_mapping.  So I think it's better to check 
> that irq was deactivated as expected.

In general, we should fix the problem at the core API level instead of
hacking individual drivers.

But more to the point, irq_dispose_mapping is not supposed to do
anything with the an active irq, as it doesn't have the required
information to safely remove it.

So calling irq_dispose_mapping on an interrupt that still has registered
actions is a bug, and I'm not convinced we want to cater for such a
case. Do you have a concrete example of some kernel code expecting this
behaviour?

Thanks,

	M.

> 
> BRs,
> Yuanyuan
> 
> 
>> -----Original Message-----
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
>> Sent: 2019年1月9日 15:43
>> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
>> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, Joey
>> <yu.zheng@hxt-semitech.com>; Wang, Dongsheng <dongsheng.wang@hxt-
>> semitech.com>
>> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.
>>
>> On Wed, 9 Jan 2019 11:53:27 +0800
>> Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> wrote:
>>
>> Hi Zhao,
>>
>>> Its device will be removed after all events be freed.
>>> Undisarded events can lead to unpredictable behaviar.
>>>
>>> Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
>>> ---
>>>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index db20e99..4fee008 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct
>> irq_domain *domain, unsigned int virq,
>>>  								virq + i);
>>>  		u32 event = its_get_event_id(data);
>>>
>>> +		/* Discard irq before free */
>>> +		if (irqd_is_activated(d))
>>> +			its_send_discard(its_dev, event);
>>> +
>>>  		/* Mark interrupt index as unused */
>>>  		clear_bit(event, its_dev->event_map.lpi_map);
>>>
>>
>> But we already do send a discard on deactivate, which logically happens
>> before we free the domain. So what are you fixing here?
>>
>> Thanks,
>>
>> 	M.
>> --
>> Without deviation from the norm, progress is not possible.


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

RE: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Zhao, Yuanyuan]

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: 2019年1月9日 17:52
> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, Joey
> <yu.zheng@hxt-semitech.com>; Wang, Dongsheng <dongsheng.wang@hxt-
> semitech.com>
> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.
> 
> On 09/01/2019 09:29, Zhao, Yuanyuan wrote:
> > Hi Marc:
> >
> > Thank you for your reply.
> >
> > As you said, APIs such as free_irq will deactivate irq before free it.
> > But deactivation is not forced by every API, for example
> > irq_dispose_mapping.  So I think it's better to check that irq was
> > deactivated as expected.
> 
> In general, we should fix the problem at the core API level instead of hacking
> individual drivers.
> 
> But more to the point, irq_dispose_mapping is not supposed to do anything
> with the an active irq, as it doesn't have the required information to safely
> remove it.
> 
> So calling irq_dispose_mapping on an interrupt that still has registered
> actions is a bug, and I'm not convinced we want to cater for such a case. Do
> you have a concrete example of some kernel code expecting this behaviour?
> 
> Thanks,
> 
> 	M.
> 

Most driver use free_irq after register actions, I found this problem by a test case. 
But if this problem happen and the same DeviceID & EventID are reused, 
the freed ITT will be visit which cause delayed kernel panic,
the prev INTs are triggered unexpected, but the new INTs lost.

So I think this check spend less, but gains more.

BRs,
Yuanyuan.


> >
> > BRs,
> > Yuanyuan
> >
> >
> >> -----Original Message-----
> >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> >> Sent: 2019年1月9日 15:43
> >> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> >> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux-
> >> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng,
> >> Joey <yu.zheng@hxt-semitech.com>; Wang, Dongsheng
> >> <dongsheng.wang@hxt- semitech.com>
> >> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.
> >>
> >> On Wed, 9 Jan 2019 11:53:27 +0800
> >> Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com> wrote:
> >>
> >> Hi Zhao,
> >>
> >>> Its device will be removed after all events be freed.
> >>> Undisarded events can lead to unpredictable behaviar.
> >>>
> >>> Signed-off-by: Zhao Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
> >>> ---
> >>>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> >>> b/drivers/irqchip/irq-gic-v3-its.c
> >>> index db20e99..4fee008 100644
> >>> --- a/drivers/irqchip/irq-gic-v3-its.c
> >>> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >>> @@ -2572,6 +2572,10 @@ static void its_irq_domain_free(struct
> >> irq_domain *domain, unsigned int virq,
> >>>  								virq + i);
> >>>  		u32 event = its_get_event_id(data);
> >>>
> >>> +		/* Discard irq before free */
> >>> +		if (irqd_is_activated(d))
> >>> +			its_send_discard(its_dev, event);
> >>> +
> >>>  		/* Mark interrupt index as unused */
> >>>  		clear_bit(event, its_dev->event_map.lpi_map);
> >>>
> >>
> >> But we already do send a discard on deactivate, which logically
> >> happens before we free the domain. So what are you fixing here?
> >>
> >> Thanks,
> >>
> >> 	M.
> >> --
> >> Without deviation from the norm, progress is not possible.
> 
> 
> --
> Jazz is not dead. It just smells funny...

Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.

[Marc Zyngier]

On 10/01/2019 09:42, Zhao, Yuanyuan wrote:
> 
> 
>> -----Original Message-----
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
>> Sent: 2019年1月9日 17:52
>> To: Zhao, Yuanyuan <yuanyuan.zhao@hxt-semitech.com>
>> Cc: tglx@linutronix.de; jason@lakedaemon.net; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zheng, Joey
>> <yu.zheng@hxt-semitech.com>; Wang, Dongsheng <dongsheng.wang@hxt-
>> semitech.com>
>> Subject: Re: [RESEND 1/1] gic: its: Make sure a LPI is discarded before free.
>>
>> On 09/01/2019 09:29, Zhao, Yuanyuan wrote:
>>> Hi Marc:
>>>
>>> Thank you for your reply.
>>>
>>> As you said, APIs such as free_irq will deactivate irq before free it.
>>> But deactivation is not forced by every API, for example
>>> irq_dispose_mapping.  So I think it's better to check that irq was
>>> deactivated as expected.
>>
>> In general, we should fix the problem at the core API level instead of hacking
>> individual drivers.
>>
>> But more to the point, irq_dispose_mapping is not supposed to do anything
>> with the an active irq, as it doesn't have the required information to safely
>> remove it.
>>
>> So calling irq_dispose_mapping on an interrupt that still has registered
>> actions is a bug, and I'm not convinced we want to cater for such a case. Do
>> you have a concrete example of some kernel code expecting this behaviour?
>>
>> Thanks,
>>
>> 	M.
>>
> 
> Most driver use free_irq after register actions, I found this problem by a test case. 

Right, so that's not a fix at all. Not following the API is the bug.

> But if this problem happen and the same DeviceID & EventID are reused, 
> the freed ITT will be visit which cause delayed kernel panic,
> the prev INTs are triggered unexpected, but the new INTs lost.

If you're disposing of the mapping and yet have not freed the interrupt,
then anything can happen. The kernel doesn't try to protect yourself
from your own mistakes.

> So I think this check spend less, but gains more.

I beg to differ. This brings nothing but a very weak guarantee that only
applies to a particular class of HW, makes a mess of the API, and papers
over grave bugs that should be fixed.

Thanks,

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