* [PATCH] dma: mv_xor: fix kernel crash on probe error
@ 2013-12-12 21:11 Aaro Koskinen
2013-12-12 21:15 ` Jason Cooper
2013-12-12 22:42 ` Russell King - ARM Linux
0 siblings, 2 replies; 7+ messages in thread
From: Aaro Koskinen @ 2013-12-12 21:11 UTC (permalink / raw)
To: linux-arm-kernel
If the non-DT channel add path fails, the kernel will crash as the
channel is not set to NULL and it will try to release the channel using
the error value. Fix that.
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
drivers/dma/mv_xor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 7807f0ef4e20..2cb35a62c7f0 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1227,6 +1227,7 @@ static int mv_xor_probe(struct platform_device *pdev)
cd->cap_mask, irq);
if (IS_ERR(xordev->channels[i])) {
ret = PTR_ERR(xordev->channels[i]);
+ xordev->channels[i] = NULL;
goto err_channel_add;
}
}
--
1.8.5.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 21:11 [PATCH] dma: mv_xor: fix kernel crash on probe error Aaro Koskinen
@ 2013-12-12 21:15 ` Jason Cooper
2013-12-12 22:42 ` Russell King - ARM Linux
1 sibling, 0 replies; 7+ messages in thread
From: Jason Cooper @ 2013-12-12 21:15 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 12, 2013 at 11:11:55PM +0200, Aaro Koskinen wrote:
> If the non-DT channel add path fails, the kernel will crash as the
> channel is not set to NULL and it will try to release the channel using
> the error value. Fix that.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
> drivers/dma/mv_xor.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Jason Cooper <jason@lakedaemon.net>
thx,
Jason.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 21:11 [PATCH] dma: mv_xor: fix kernel crash on probe error Aaro Koskinen
2013-12-12 21:15 ` Jason Cooper
@ 2013-12-12 22:42 ` Russell King - ARM Linux
2013-12-12 23:36 ` Dan Williams
1 sibling, 1 reply; 7+ messages in thread
From: Russell King - ARM Linux @ 2013-12-12 22:42 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 12, 2013 at 11:11:55PM +0200, Aaro Koskinen wrote:
> If the non-DT channel add path fails, the kernel will crash as the
> channel is not set to NULL and it will try to release the channel using
> the error value. Fix that.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
> drivers/dma/mv_xor.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 7807f0ef4e20..2cb35a62c7f0 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1227,6 +1227,7 @@ static int mv_xor_probe(struct platform_device *pdev)
> cd->cap_mask, irq);
> if (IS_ERR(xordev->channels[i])) {
> ret = PTR_ERR(xordev->channels[i]);
> + xordev->channels[i] = NULL;
> goto err_channel_add;
> }
> }
Yes, I found this too, and although this is _a_ fix, it's not my
preferred. I'd much prefer this instead - avoid writing invalid
channels to xordev->channels[i] in the first place... Slightly
larger patch but IMHO more correct.
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 7807f0ef4e20..a7e91090443e 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1176,6 +1176,7 @@ static int mv_xor_probe(struct platform_device *pdev)
int i = 0;
for_each_child_of_node(pdev->dev.of_node, np) {
+ struct mv_xor_chan *chan;
dma_cap_mask_t cap_mask;
int irq;
@@ -1193,21 +1194,21 @@ static int mv_xor_probe(struct platform_device *pdev)
goto err_channel_add;
}
- xordev->channels[i] =
- mv_xor_channel_add(xordev, pdev, i,
- cap_mask, irq);
- if (IS_ERR(xordev->channels[i])) {
- ret = PTR_ERR(xordev->channels[i]);
- xordev->channels[i] = NULL;
+ chan = mv_xor_channel_add(xordev, pdev, i,
+ cap_mask, irq);
+ if (IS_ERR(chan)) {
+ ret = PTR_ERR(chan);
irq_dispose_mapping(irq);
goto err_channel_add;
}
+ xordev->channels[i] = chan;
i++;
}
} else if (pdata && pdata->channels) {
for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
struct mv_xor_channel_data *cd;
+ struct mv_xor_chan *chan;
int irq;
cd = &pdata->channels[i];
@@ -1222,13 +1223,14 @@ static int mv_xor_probe(struct platform_device *pdev)
goto err_channel_add;
}
- xordev->channels[i] =
- mv_xor_channel_add(xordev, pdev, i,
- cd->cap_mask, irq);
- if (IS_ERR(xordev->channels[i])) {
- ret = PTR_ERR(xordev->channels[i]);
+ chan = mv_xor_channel_add(xordev, pdev, i,
+ cd->cap_mask, irq);
+ if (IS_ERR(chan)) {
+ ret = PTR_ERR(chan);
goto err_channel_add;
}
+
+ xordev->channels[i] = chan;
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 22:42 ` Russell King - ARM Linux
@ 2013-12-12 23:36 ` Dan Williams
2013-12-12 23:44 ` Aaro Koskinen
0 siblings, 1 reply; 7+ messages in thread
From: Dan Williams @ 2013-12-12 23:36 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 12, 2013 at 2:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Dec 12, 2013 at 11:11:55PM +0200, Aaro Koskinen wrote:
>> If the non-DT channel add path fails, the kernel will crash as the
>> channel is not set to NULL and it will try to release the channel using
>> the error value. Fix that.
>>
>> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
>> ---
>> drivers/dma/mv_xor.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
>> index 7807f0ef4e20..2cb35a62c7f0 100644
>> --- a/drivers/dma/mv_xor.c
>> +++ b/drivers/dma/mv_xor.c
>> @@ -1227,6 +1227,7 @@ static int mv_xor_probe(struct platform_device *pdev)
>> cd->cap_mask, irq);
>> if (IS_ERR(xordev->channels[i])) {
>> ret = PTR_ERR(xordev->channels[i]);
>> + xordev->channels[i] = NULL;
>> goto err_channel_add;
>> }
>> }
>
> Yes, I found this too, and although this is _a_ fix, it's not my
> preferred. I'd much prefer this instead - avoid writing invalid
> channels to xordev->channels[i] in the first place... Slightly
> larger patch but IMHO more correct.
>
Looks good to me. Aaro?
Can I have one with a sign-off?
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 23:36 ` Dan Williams
@ 2013-12-12 23:44 ` Aaro Koskinen
2013-12-12 23:59 ` Russell King - ARM Linux
0 siblings, 1 reply; 7+ messages in thread
From: Aaro Koskinen @ 2013-12-12 23:44 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 12, 2013 at 03:36:13PM -0800, Dan Williams wrote:
> On Thu, Dec 12, 2013 at 2:42 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Dec 12, 2013 at 11:11:55PM +0200, Aaro Koskinen wrote:
> >> If the non-DT channel add path fails, the kernel will crash as the
> >> channel is not set to NULL and it will try to release the channel using
> >> the error value. Fix that.
> >>
> >> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> >> ---
> >> drivers/dma/mv_xor.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> >> index 7807f0ef4e20..2cb35a62c7f0 100644
> >> --- a/drivers/dma/mv_xor.c
> >> +++ b/drivers/dma/mv_xor.c
> >> @@ -1227,6 +1227,7 @@ static int mv_xor_probe(struct platform_device *pdev)
> >> cd->cap_mask, irq);
> >> if (IS_ERR(xordev->channels[i])) {
> >> ret = PTR_ERR(xordev->channels[i]);
> >> + xordev->channels[i] = NULL;
> >> goto err_channel_add;
> >> }
> >> }
> >
> > Yes, I found this too, and although this is _a_ fix, it's not my
> > preferred. I'd much prefer this instead - avoid writing invalid
> > channels to xordev->channels[i] in the first place... Slightly
> > larger patch but IMHO more correct.
> >
>
> Looks good to me. Aaro?
I'm also fine this version. Just to be sure, I tested the error path on
my Marvell OpenRD client, so you can put my Tested-by to the final patch.
A.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 23:44 ` Aaro Koskinen
@ 2013-12-12 23:59 ` Russell King - ARM Linux
2013-12-18 16:35 ` Vinod Koul
0 siblings, 1 reply; 7+ messages in thread
From: Russell King - ARM Linux @ 2013-12-12 23:59 UTC (permalink / raw)
To: linux-arm-kernel
From: Russell King <rmk+kernel@arm.linux.org.uk>
Subject: [PATCH] dmaengine: mv_xor: fix oops when channels fail to initialise
When a channel fails to initialise, we error out and clean up any
previously unregistered channels by walking the entire xordev->channels
array. Unfortunately, there are paths which end up storing an error
pointer in this array, which we then try and dereference in the cleanup
code, which causes an oops.
Fix this by avoiding writing invalid pointers to this array in the first
place.
Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
drivers/dma/mv_xor.c | 24 +++++++++++++-----------
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 7807f0ef4e20..a7e91090443e 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1176,6 +1176,7 @@ static int mv_xor_probe(struct platform_device *pdev)
int i = 0;
for_each_child_of_node(pdev->dev.of_node, np) {
+ struct mv_xor_chan *chan;
dma_cap_mask_t cap_mask;
int irq;
@@ -1193,21 +1194,21 @@ static int mv_xor_probe(struct platform_device *pdev)
goto err_channel_add;
}
- xordev->channels[i] =
- mv_xor_channel_add(xordev, pdev, i,
- cap_mask, irq);
- if (IS_ERR(xordev->channels[i])) {
- ret = PTR_ERR(xordev->channels[i]);
- xordev->channels[i] = NULL;
+ chan = mv_xor_channel_add(xordev, pdev, i,
+ cap_mask, irq);
+ if (IS_ERR(chan)) {
+ ret = PTR_ERR(chan);
irq_dispose_mapping(irq);
goto err_channel_add;
}
+ xordev->channels[i] = chan;
i++;
}
} else if (pdata && pdata->channels) {
for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
struct mv_xor_channel_data *cd;
+ struct mv_xor_chan *chan;
int irq;
cd = &pdata->channels[i];
@@ -1222,13 +1223,14 @@ static int mv_xor_probe(struct platform_device *pdev)
goto err_channel_add;
}
- xordev->channels[i] =
- mv_xor_channel_add(xordev, pdev, i,
- cd->cap_mask, irq);
- if (IS_ERR(xordev->channels[i])) {
- ret = PTR_ERR(xordev->channels[i]);
+ chan = mv_xor_channel_add(xordev, pdev, i,
+ cd->cap_mask, irq);
+ if (IS_ERR(chan)) {
+ ret = PTR_ERR(chan);
goto err_channel_add;
}
+
+ xordev->channels[i] = chan;
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] dma: mv_xor: fix kernel crash on probe error
2013-12-12 23:59 ` Russell King - ARM Linux
@ 2013-12-18 16:35 ` Vinod Koul
0 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2013-12-18 16:35 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 12, 2013 at 11:59:08PM +0000, Russell King - ARM Linux wrote:
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: [PATCH] dmaengine: mv_xor: fix oops when channels fail to initialise
>
> When a channel fails to initialise, we error out and clean up any
> previously unregistered channels by walking the entire xordev->channels
> array. Unfortunately, there are paths which end up storing an error
> pointer in this array, which we then try and dereference in the cleanup
> code, which causes an oops.
>
> Fix this by avoiding writing invalid pointers to this array in the first
> place.
>
> Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Vinod Koul <vinod.koul@intel.com>
--
~Vinod
> ---
> drivers/dma/mv_xor.c | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 7807f0ef4e20..a7e91090443e 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1176,6 +1176,7 @@ static int mv_xor_probe(struct platform_device *pdev)
> int i = 0;
>
> for_each_child_of_node(pdev->dev.of_node, np) {
> + struct mv_xor_chan *chan;
> dma_cap_mask_t cap_mask;
> int irq;
>
> @@ -1193,21 +1194,21 @@ static int mv_xor_probe(struct platform_device *pdev)
> goto err_channel_add;
> }
>
> - xordev->channels[i] =
> - mv_xor_channel_add(xordev, pdev, i,
> - cap_mask, irq);
> - if (IS_ERR(xordev->channels[i])) {
> - ret = PTR_ERR(xordev->channels[i]);
> - xordev->channels[i] = NULL;
> + chan = mv_xor_channel_add(xordev, pdev, i,
> + cap_mask, irq);
> + if (IS_ERR(chan)) {
> + ret = PTR_ERR(chan);
> irq_dispose_mapping(irq);
> goto err_channel_add;
> }
>
> + xordev->channels[i] = chan;
> i++;
> }
> } else if (pdata && pdata->channels) {
> for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
> struct mv_xor_channel_data *cd;
> + struct mv_xor_chan *chan;
> int irq;
>
> cd = &pdata->channels[i];
> @@ -1222,13 +1223,14 @@ static int mv_xor_probe(struct platform_device *pdev)
> goto err_channel_add;
> }
>
> - xordev->channels[i] =
> - mv_xor_channel_add(xordev, pdev, i,
> - cd->cap_mask, irq);
> - if (IS_ERR(xordev->channels[i])) {
> - ret = PTR_ERR(xordev->channels[i]);
> + chan = mv_xor_channel_add(xordev, pdev, i,
> + cd->cap_mask, irq);
> + if (IS_ERR(chan)) {
> + ret = PTR_ERR(chan);
> goto err_channel_add;
> }
> +
> + xordev->channels[i] = chan;
> }
> }
>
--
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-18 16:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-12 21:11 [PATCH] dma: mv_xor: fix kernel crash on probe error Aaro Koskinen
2013-12-12 21:15 ` Jason Cooper
2013-12-12 22:42 ` Russell King - ARM Linux
2013-12-12 23:36 ` Dan Williams
2013-12-12 23:44 ` Aaro Koskinen
2013-12-12 23:59 ` Russell King - ARM Linux
2013-12-18 16:35 ` Vinod Koul
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).