| Message ID | 20220223070223.26845-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc: 8xx: fix a return value error in mpc8xx_pic_init | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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;
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;
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
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
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
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
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
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
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;
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(+)