2014-12-08 15:03:23

by Yingjoe Chen

[permalink] [raw]
Subject: [PATCH] irqchip: mediatek: Fix error return code detection

This fix an error handling bug reported by Beniamino, this is based on
mtk intpol patches [1]

Joe.C

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html

====================
of_io_request_and_map() return ERR_PTR wrapped error code instead of
NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
this.

Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;

chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}

--
1.8.1.1.dirty


2014-12-08 20:30:59

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] irqchip: mediatek: Fix error return code detection

On Mon, 8 Dec 2014, Yingjoe Chen wrote:

> This fix an error handling bug reported by Beniamino, this is based on
> mtk intpol patches [1]
>
> Joe.C
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html
>
> ====================
> of_io_request_and_map() return ERR_PTR wrapped error code instead of
> NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
> this.
>
> Signed-off-by: Yingjoe Chen <[email protected]>

This is not a proper changelog. Let me write up a proper one:

Subject: irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

Beniamino reported a kernel oops caused by an invalid DT file for the
mediatek interrupt polarity extension.

The reason is that the interrupt polarity support for mediatek chips
merily checks for at NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.

Use IS_ERR() and return the error value via PTR_ERR().

Reported-and-tested-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>

Can you see the difference?

Thanks,

tglx

2014-12-09 06:11:36

by Yingjoe Chen

[permalink] [raw]
Subject: Re: [PATCH] irqchip: mediatek: Fix error return code detection

On Mon, 2014-12-08 at 21:30 +0100, Thomas Gleixner wrote:
> On Mon, 8 Dec 2014, Yingjoe Chen wrote:
>
> > This fix an error handling bug reported by Beniamino, this is based on
> > mtk intpol patches [1]
> >
> > Joe.C
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305808.html
> >
> > ====================
> > of_io_request_and_map() return ERR_PTR wrapped error code instead of
> > NULL when fail, fix code in mtk_sysirq_of_init() to correctly handle
> > this.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
>
> This is not a proper changelog. Let me write up a proper one:
>
> Subject: irqchip: mediatek: Use IS_ERR() instead of NULL pointer check
>
> Beniamino reported a kernel oops caused by an invalid DT file for the
> mediatek interrupt polarity extension.
>
> The reason is that the interrupt polarity support for mediatek chips
> merily checks for at NULL pointer instead of a casted error return
> value in mtk_sysirq_of_init() so any other casted error value passes
> the NULL pointer check and causes a kernel panic when dereferenced.
>
> Use IS_ERR() and return the error value via PTR_ERR().
>
> Reported-and-tested-by: Beniamino Galvani <[email protected]>
> Signed-off-by: Yingjoe Chen <[email protected]>
>
> Can you see the difference?
>
> Thanks,
>
> tglx

Thanks for the review.

This changelog describe issue and why we need this change more clearly.
I'll prepare a new patch with this changelog, Thanks.

Joe.C

2014-12-10 09:55:22

by Yingjoe Chen

[permalink] [raw]
Subject: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

Beniamino reported a kernel oops caused by an invalid DT file for the
mediatek interrupt polarity extension.

The reason is that the interrupt polarity support for mediatek chips
merely checks for NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.

Use IS_ERR() and return the error value via PTR_ERR().

Reported-and-tested-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;

chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}

--
1.8.1.1.dirty

2014-12-10 12:17:32

by Beniamino Galvani

[permalink] [raw]
Subject: Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> Beniamino reported a kernel oops caused by an invalid DT file for the
> mediatek interrupt polarity extension.
>
> The reason is that the interrupt polarity support for mediatek chips
> merely checks for NULL pointer instead of a casted error return
> value in mtk_sysirq_of_init() so any other casted error value passes
> the NULL pointer check and causes a kernel panic when dereferenced.
>
> Use IS_ERR() and return the error value via PTR_ERR().
>
> Reported-and-tested-by: Beniamino Galvani <[email protected]>

Well, actually I only reported the bug and didn't do any test. Can the
"-and-tested" portion of the tag be dropped while applying the patch?

Beniamino

> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
> index 7e342df..0b0d2c0 100644
> --- a/drivers/irqchip/irq-mtk-sysirq.c
> +++ b/drivers/irqchip/irq-mtk-sysirq.c
> @@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
> return -ENOMEM;
>
> chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
> - if (!chip_data->intpol_base) {
> + if (IS_ERR(chip_data->intpol_base)) {
> pr_err("mtk_sysirq: unable to map sysirq register\n");
> - ret = -ENOMEM;
> + ret = PTR_ERR(chip_data->intpol_base);
> goto out_free;
> }
>
> --
> 1.8.1.1.dirty
>

2014-12-10 12:53:42

by Jason Cooper

[permalink] [raw]
Subject: Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > Beniamino reported a kernel oops caused by an invalid DT file for the
> > mediatek interrupt polarity extension.
> >
> > The reason is that the interrupt polarity support for mediatek chips
> > merely checks for NULL pointer instead of a casted error return
> > value in mtk_sysirq_of_init() so any other casted error value passes
> > the NULL pointer check and causes a kernel panic when dereferenced.
> >
> > Use IS_ERR() and return the error value via PTR_ERR().
> >
> > Reported-and-tested-by: Beniamino Galvani <[email protected]>
>
> Well, actually I only reported the bug and didn't do any test. Can the
> "-and-tested" portion of the tag be dropped while applying the patch?

I'd prefer that it be tested before applying. Would you mind confirming
that the oops is gone with this patch applied?

thx,

Jason.

2014-12-10 13:56:01

by Yingjoe Chen

[permalink] [raw]
Subject: Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

On Wed, 2014-12-10 at 07:53 -0500, Jason Cooper wrote:
> On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> > On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > > Beniamino reported a kernel oops caused by an invalid DT file for the
> > > mediatek interrupt polarity extension.
> > >
> > > The reason is that the interrupt polarity support for mediatek chips
> > > merely checks for NULL pointer instead of a casted error return
> > > value in mtk_sysirq_of_init() so any other casted error value passes
> > > the NULL pointer check and causes a kernel panic when dereferenced.
> > >
> > > Use IS_ERR() and return the error value via PTR_ERR().
> > >
> > > Reported-and-tested-by: Beniamino Galvani <[email protected]>
> >
> > Well, actually I only reported the bug and didn't do any test. Can the
> > "-and-tested" portion of the tag be dropped while applying the patch?
>
> I'd prefer that it be tested before applying. Would you mind confirming
> that the oops is gone with this patch applied?

Hi,

I'm not sure if that count, but I tested with incorrect DTS node
with/without the patch, to make sure the oops is gone with the patch.

Please note this driver is necessary to boot and kernel uart driver,
even without the oops you still can't boot to shell and you'll need
earlyprintk to see the error message.

Joe.C

2014-12-10 20:10:56

by Beniamino Galvani

[permalink] [raw]
Subject: Re: [PATCH v2] irqchip: mediatek: Use IS_ERR() instead of NULL pointer check

On Wed, Dec 10, 2014 at 07:53:13AM -0500, Jason Cooper wrote:
> On Wed, Dec 10, 2014 at 01:14:43PM +0100, Beniamino Galvani wrote:
> > On Wed, Dec 10, 2014 at 05:55:02PM +0800, Yingjoe Chen wrote:
> > > Beniamino reported a kernel oops caused by an invalid DT file for the
> > > mediatek interrupt polarity extension.
> > >
> > > The reason is that the interrupt polarity support for mediatek chips
> > > merely checks for NULL pointer instead of a casted error return
> > > value in mtk_sysirq_of_init() so any other casted error value passes
> > > the NULL pointer check and causes a kernel panic when dereferenced.
> > >
> > > Use IS_ERR() and return the error value via PTR_ERR().
> > >
> > > Reported-and-tested-by: Beniamino Galvani <[email protected]>
> >
> > Well, actually I only reported the bug and didn't do any test. Can the
> > "-and-tested" portion of the tag be dropped while applying the patch?
>
> I'd prefer that it be tested before applying. Would you mind confirming
> that the oops is gone with this patch applied?

Probably the commit message is misleading about this, but I don't own
any Mediatek device and never used the driver. I only reported [1] a
possible bug in the driver found through code analysis.

Anyway, I suppose that the tests done by Yingjoe are enough to get the
patch merged.

Beniamino

[1] https://lkml.org/lkml/2014/11/29/105

2014-12-12 15:16:08

by Yingjoe Chen

[permalink] [raw]
Subject: [PATCH v3] irqchip: mtk-sysirq: Use IS_ERR() instead of NULL pointer check

Hi,

Small fix to the commit message and the reported by tag, the code stay
the same.

Thanks

Joe.C

=============================================
Beniamino noticed a bug that an invalid DT file for the mediatek
interrupt polarity extension will cause kernel oops.

The reason is that the interrupt polarity support for mediatek chips
merely checks for NULL pointer instead of a casted error return
value in mtk_sysirq_of_init() so any other casted error value passes
the NULL pointer check and causes a kernel panic when dereferenced.

Use IS_ERR() and return the error value via PTR_ERR().

Reported-by: Beniamino Galvani <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/irqchip/irq-mtk-sysirq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 7e342df..0b0d2c0 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -137,9 +137,9 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
return -ENOMEM;

chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
- if (!chip_data->intpol_base) {
+ if (IS_ERR(chip_data->intpol_base)) {
pr_err("mtk_sysirq: unable to map sysirq register\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(chip_data->intpol_base);
goto out_free;
}

--
1.8.1.1.dirty