* [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
@ 2022-02-23 7:02 Hangyu Hua
2022-03-07 1:41 ` Hangyu Hua
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Hangyu Hua @ 2022-02-23 7:02 UTC (permalink / raw)
To: christophe.leroy, mpe, benh, paulus, peng.hao2, wen.yang99
Cc: linuxppc-dev, linux-kernel, Hangyu Hua
mpc8xx_pic_init() should return -ENOMEM instead of 0 when
irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
executing even if mpc8xx_pic_host is NULL.
Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
arch/powerpc/platforms/8xx/pic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
index f2ba837249d6..04a6abf14c29 100644
--- a/arch/powerpc/platforms/8xx/pic.c
+++ b/arch/powerpc/platforms/8xx/pic.c
@@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
if (mpc8xx_pic_host == NULL) {
printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
ret = -ENOMEM;
+ goto out;
}
ret = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-02-23 7:02 [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init Hangyu Hua
@ 2022-03-07 1:41 ` Hangyu Hua
2022-03-08 15:00 ` Christophe Leroy
2022-03-09 3:24 ` Michael Ellerman
2022-03-12 10:29 ` Michael Ellerman
2 siblings, 1 reply; 9+ messages in thread
From: Hangyu Hua @ 2022-03-07 1:41 UTC (permalink / raw)
To: christophe.leroy, mpe, benh, paulus, peng.hao2, wen.yang99
Cc: linuxppc-dev, linux-kernel
Ping?
On 2022/2/23 15:02, Hangyu Hua wrote:
> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
> executing even if mpc8xx_pic_host is NULL.
>
> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
> arch/powerpc/platforms/8xx/pic.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
> index f2ba837249d6..04a6abf14c29 100644
> --- a/arch/powerpc/platforms/8xx/pic.c
> +++ b/arch/powerpc/platforms/8xx/pic.c
> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
> if (mpc8xx_pic_host == NULL) {
> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
> ret = -ENOMEM;
> + goto out;
> }
>
> ret = 0;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-03-07 1:41 ` Hangyu Hua
@ 2022-03-08 15:00 ` Christophe Leroy
0 siblings, 0 replies; 9+ messages in thread
From: Christophe Leroy @ 2022-03-08 15:00 UTC (permalink / raw)
To: Hangyu Hua, mpe@ellerman.id.au, benh@kernel.crashing.org,
paulus@samba.org, peng.hao2@zte.com.cn, wen.yang99@zte.com.cn
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Le 07/03/2022 à 02:41, Hangyu Hua a écrit :
> Ping?
>
> On 2022/2/23 15:02, Hangyu Hua wrote:
>> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
>> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to
>> continue
>> executing even if mpc8xx_pic_host is NULL.
>>
>> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference
>> leak")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/platforms/8xx/pic.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/platforms/8xx/pic.c
>> b/arch/powerpc/platforms/8xx/pic.c
>> index f2ba837249d6..04a6abf14c29 100644
>> --- a/arch/powerpc/platforms/8xx/pic.c
>> +++ b/arch/powerpc/platforms/8xx/pic.c
>> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
>> if (mpc8xx_pic_host == NULL) {
>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>> ret = -ENOMEM;
>> + goto out;
>> }
>> ret = 0;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-02-23 7:02 [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init Hangyu Hua
2022-03-07 1:41 ` Hangyu Hua
@ 2022-03-09 3:24 ` Michael Ellerman
2022-03-09 7:21 ` Christophe Leroy
2022-03-12 10:29 ` Michael Ellerman
2 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2022-03-09 3:24 UTC (permalink / raw)
To: Hangyu Hua, christophe.leroy, benh, paulus, peng.hao2, wen.yang99
Cc: linuxppc-dev, linux-kernel, Hangyu Hua
Hangyu Hua <hbh25y@gmail.com> writes:
> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
> executing even if mpc8xx_pic_host is NULL.
>
> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
> arch/powerpc/platforms/8xx/pic.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
> index f2ba837249d6..04a6abf14c29 100644
> --- a/arch/powerpc/platforms/8xx/pic.c
> +++ b/arch/powerpc/platforms/8xx/pic.c
> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
Expanding the context:
siu_reg = ioremap(res.start, resource_size(&res));
if (siu_reg == NULL) {
ret = -EINVAL;
goto out;
}
mpc8xx_pic_host = irq_domain_add_linear(np, 64, &mpc8xx_pic_host_ops, NULL);
> if (mpc8xx_pic_host == NULL) {
> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
> ret = -ENOMEM;
> + goto out;
> }
>
> ret = 0;
>
out:
of_node_put(np);
return ret;
}
Proper error cleanup should also undo the ioremap() if
irq_domain_add_linear() fails.
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-03-09 3:24 ` Michael Ellerman
@ 2022-03-09 7:21 ` Christophe Leroy
2022-03-09 10:46 ` Michael Ellerman
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2022-03-09 7:21 UTC (permalink / raw)
To: Michael Ellerman, Hangyu Hua, benh@kernel.crashing.org,
paulus@samba.org, peng.hao2@zte.com.cn, wen.yang99@zte.com.cn
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Le 09/03/2022 à 04:24, Michael Ellerman a écrit :
> Hangyu Hua <hbh25y@gmail.com> writes:
>> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
>> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
>> executing even if mpc8xx_pic_host is NULL.
>>
>> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> ---
>> arch/powerpc/platforms/8xx/pic.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
>> index f2ba837249d6..04a6abf14c29 100644
>> --- a/arch/powerpc/platforms/8xx/pic.c
>> +++ b/arch/powerpc/platforms/8xx/pic.c
>> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
>
> Expanding the context:
>
> siu_reg = ioremap(res.start, resource_size(&res));
> if (siu_reg == NULL) {
> ret = -EINVAL;
> goto out;
> }
>
> mpc8xx_pic_host = irq_domain_add_linear(np, 64, &mpc8xx_pic_host_ops, NULL);
>> if (mpc8xx_pic_host == NULL) {
>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>> ret = -ENOMEM;
>> + goto out;
>> }
>>
>> ret = 0;
>>
> out:
> of_node_put(np);
> return ret;
> }
>
> Proper error cleanup should also undo the ioremap() if
> irq_domain_add_linear() fails.
Uh ...
If siu_reg is NULL, you get a serious problem when __do_irq() calls
mpc8xx_get_irq()
unsigned int mpc8xx_get_irq(void)
{
int irq;
/* For MPC8xx, read the SIVEC register and shift the bits down
* to get the irq number.
*/
irq = in_be32(&siu_reg->sc_sivec) >> 26;
if (irq == PIC_VEC_SPURRIOUS)
return 0;
return irq_linear_revmap(mpc8xx_pic_host, irq);
}
So I'll leave siu_reg as is even if irq_domain_add_linear() fails.
Whatever, if we do something about that it should be done in another
patch I think.
Christophe
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-03-09 7:21 ` Christophe Leroy
@ 2022-03-09 10:46 ` Michael Ellerman
2022-03-09 11:13 ` Christophe Leroy
0 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2022-03-09 10:46 UTC (permalink / raw)
To: Christophe Leroy, Hangyu Hua, benh@kernel.crashing.org,
paulus@samba.org, peng.hao2@zte.com.cn, wen.yang99@zte.com.cn
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 09/03/2022 à 04:24, Michael Ellerman a écrit :
>> Hangyu Hua <hbh25y@gmail.com> writes:
>>> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
>>> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
>>> executing even if mpc8xx_pic_host is NULL.
>>>
>>> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>> ---
>>> arch/powerpc/platforms/8xx/pic.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
>>> index f2ba837249d6..04a6abf14c29 100644
>>> --- a/arch/powerpc/platforms/8xx/pic.c
>>> +++ b/arch/powerpc/platforms/8xx/pic.c
>>> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
>>
>> Expanding the context:
>>
>> siu_reg = ioremap(res.start, resource_size(&res));
>> if (siu_reg == NULL) {
>> ret = -EINVAL;
>> goto out;
>> }
>>
>> mpc8xx_pic_host = irq_domain_add_linear(np, 64, &mpc8xx_pic_host_ops, NULL);
>>> if (mpc8xx_pic_host == NULL) {
>>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>>> ret = -ENOMEM;
>>> + goto out;
>>> }
>>>
>>> ret = 0;
>>>
>> out:
>> of_node_put(np);
>> return ret;
>> }
>>
>> Proper error cleanup should also undo the ioremap() if
>> irq_domain_add_linear() fails.
>
> Uh ...
>
> If siu_reg is NULL, you get a serious problem when __do_irq() calls
> mpc8xx_get_irq()
Arguably it shouldn't be assigned to ppc_md.get_irq unless
mpc8xx_pic_init() succeeds. See eg. xics_init().
> unsigned int mpc8xx_get_irq(void)
> {
> int irq;
>
> /* For MPC8xx, read the SIVEC register and shift the bits down
> * to get the irq number.
> */
> irq = in_be32(&siu_reg->sc_sivec) >> 26;
>
> if (irq == PIC_VEC_SPURRIOUS)
> return 0;
>
> return irq_linear_revmap(mpc8xx_pic_host, irq);
>
> }
>
>
> So I'll leave siu_reg as is even if irq_domain_add_linear() fails.
>
> Whatever, if we do something about that it should be done in another
> patch I think.
Yeah OK, that's becoming a bit of a larger cleanup. I'll take this patch
as-is.
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-03-09 10:46 ` Michael Ellerman
@ 2022-03-09 11:13 ` Christophe Leroy
2022-03-09 23:16 ` Michael Ellerman
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2022-03-09 11:13 UTC (permalink / raw)
To: Michael Ellerman, Hangyu Hua, benh@kernel.crashing.org,
paulus@samba.org, peng.hao2@zte.com.cn, wen.yang99@zte.com.cn
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Le 09/03/2022 à 11:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 09/03/2022 à 04:24, Michael Ellerman a écrit :
>>> Hangyu Hua <hbh25y@gmail.com> writes:
>>>> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
>>>> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
>>>> executing even if mpc8xx_pic_host is NULL.
>>>>
>>>> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
>>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>>> ---
>>>> arch/powerpc/platforms/8xx/pic.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
>>>> index f2ba837249d6..04a6abf14c29 100644
>>>> --- a/arch/powerpc/platforms/8xx/pic.c
>>>> +++ b/arch/powerpc/platforms/8xx/pic.c
>>>> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
>>>
>>> Expanding the context:
>>>
>>> siu_reg = ioremap(res.start, resource_size(&res));
>>> if (siu_reg == NULL) {
>>> ret = -EINVAL;
>>> goto out;
>>> }
>>>
>>> mpc8xx_pic_host = irq_domain_add_linear(np, 64, &mpc8xx_pic_host_ops, NULL);
>>>> if (mpc8xx_pic_host == NULL) {
>>>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>>>> ret = -ENOMEM;
>>>> + goto out;
>>>> }
>>>>
>>>> ret = 0;
>>>>
>>> out:
>>> of_node_put(np);
>>> return ret;
>>> }
>>>
>>> Proper error cleanup should also undo the ioremap() if
>>> irq_domain_add_linear() fails.
>>
>> Uh ...
>>
>> If siu_reg is NULL, you get a serious problem when __do_irq() calls
>> mpc8xx_get_irq()
>
> Arguably it shouldn't be assigned to ppc_md.get_irq unless
> mpc8xx_pic_init() succeeds. See eg. xics_init().
I agree with that, but it's a huge work I guess. Most platforms set
.get_irq in ppc_md() at buildtime. See the generic mpic_get_irq() which
has a BUG_ON() in call mpic_primary is NULL. There are 50 platforms with
buildtime assignment.
That would however be a good opportunity to switch get_irq() to a static
call. I'll open a github issue to follow it.
Christophe
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-03-09 11:13 ` Christophe Leroy
@ 2022-03-09 23:16 ` Michael Ellerman
0 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2022-03-09 23:16 UTC (permalink / raw)
To: Christophe Leroy, Hangyu Hua, benh@kernel.crashing.org,
paulus@samba.org, peng.hao2@zte.com.cn, wen.yang99@zte.com.cn
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 09/03/2022 à 11:46, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 09/03/2022 à 04:24, Michael Ellerman a écrit :
>>>> Hangyu Hua <hbh25y@gmail.com> writes:
>>>>> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
>>>>> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
>>>>> executing even if mpc8xx_pic_host is NULL.
>>>>>
>>>>> Fixes: cc76404feaed ("powerpc/8xx: Fix possible device node reference leak")
>>>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>>>> ---
>>>>> arch/powerpc/platforms/8xx/pic.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
>>>>> index f2ba837249d6..04a6abf14c29 100644
>>>>> --- a/arch/powerpc/platforms/8xx/pic.c
>>>>> +++ b/arch/powerpc/platforms/8xx/pic.c
>>>>> @@ -153,6 +153,7 @@ int __init mpc8xx_pic_init(void)
>>>>
>>>> Expanding the context:
>>>>
>>>> siu_reg = ioremap(res.start, resource_size(&res));
>>>> if (siu_reg == NULL) {
>>>> ret = -EINVAL;
>>>> goto out;
>>>> }
>>>>
>>>> mpc8xx_pic_host = irq_domain_add_linear(np, 64, &mpc8xx_pic_host_ops, NULL);
>>>>> if (mpc8xx_pic_host == NULL) {
>>>>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>>>>> ret = -ENOMEM;
>>>>> + goto out;
>>>>> }
>>>>>
>>>>> ret = 0;
>>>>>
>>>> out:
>>>> of_node_put(np);
>>>> return ret;
>>>> }
>>>>
>>>> Proper error cleanup should also undo the ioremap() if
>>>> irq_domain_add_linear() fails.
>>>
>>> Uh ...
>>>
>>> If siu_reg is NULL, you get a serious problem when __do_irq() calls
>>> mpc8xx_get_irq()
>>
>> Arguably it shouldn't be assigned to ppc_md.get_irq unless
>> mpc8xx_pic_init() succeeds. See eg. xics_init().
>
> I agree with that, but it's a huge work I guess. Most platforms set
> .get_irq in ppc_md() at buildtime. See the generic mpic_get_irq() which
> has a BUG_ON() in call mpic_primary is NULL. There are 50 platforms with
> buildtime assignment.
Yes I agree. And __do_irq() will just oops if ppc_md.get_irq() is NULL,
so it's a bit of a mess.
> That would however be a good opportunity to switch get_irq() to a static
> call. I'll open a github issue to follow it.
Thanks.
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
2022-02-23 7:02 [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init Hangyu Hua
2022-03-07 1:41 ` Hangyu Hua
2022-03-09 3:24 ` Michael Ellerman
@ 2022-03-12 10:29 ` Michael Ellerman
2 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2022-03-12 10:29 UTC (permalink / raw)
To: benh, mpe, christophe.leroy, wen.yang99, Hangyu Hua, peng.hao2,
paulus
Cc: linuxppc-dev, linux-kernel
On Wed, 23 Feb 2022 15:02:23 +0800, Hangyu Hua wrote:
> mpc8xx_pic_init() should return -ENOMEM instead of 0 when
> irq_domain_add_linear() return NULL. This cause mpc8xx_pics_init to continue
> executing even if mpc8xx_pic_host is NULL.
>
>
Applied to powerpc/next.
[1/1] powerpc: 8xx: fix a return value error in mpc8xx_pic_init
https://git.kernel.org/powerpc/c/3fd46e551f67f4303c3276a0d6cd20baf2d192c4
cheers
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-12 10:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-23 7:02 [PATCH] powerpc: 8xx: fix a return value error in mpc8xx_pic_init Hangyu Hua
2022-03-07 1:41 ` Hangyu Hua
2022-03-08 15:00 ` Christophe Leroy
2022-03-09 3:24 ` Michael Ellerman
2022-03-09 7:21 ` Christophe Leroy
2022-03-09 10:46 ` Michael Ellerman
2022-03-09 11:13 ` Christophe Leroy
2022-03-09 23:16 ` Michael Ellerman
2022-03-12 10:29 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).