* [PATCH] net: ethernet: ti: davinci_cpdma: fix access to uninitialized variable in cpdma_chan_set_descs()
@ 2016-12-28 23:42 Grygorii Strashko
2016-12-29 1:49 ` Ivan Khoronzhuk
0 siblings, 1 reply; 3+ messages in thread
From: Grygorii Strashko @ 2016-12-28 23:42 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N
Cc: Sekhar Nori, linux-kernel, linux-omap, Ivan Khoronzhuk,
Grygorii Strashko
Now below code sequence causes "Unable to handle kernel NULL pointer
dereference.." exception and system crash during CPSW CPDMA initialization:
cpsw_probe
|-cpdma_chan_create (TX channel)
|-cpdma_chan_split_pool
|-cpdma_chan_set_descs(for TX channels)
|-cpdma_chan_set_descs(for RX channels) [1]
- and -
static void cpdma_chan_set_descs(struct cpdma_ctlr *ctlr,
int rx, int desc_num,
int per_ch_desc)
{
struct cpdma_chan *chan, *most_chan = NULL;
...
for (i = min; i < max; i++) {
chan = ctlr->channels[i];
if (!chan)
continue;
...
if (most_dnum < chan->desc_num) {
most_dnum = chan->desc_num;
most_chan = chan;
}
}
/* use remains */
most_chan->desc_num += desc_cnt; [2]
}
So, most_chan value will never be reassigned when cpdma_chan_set_descs() is
called second time [1], because there are no RX channels yet and system
will crash at [2].
Hence, fix the issue by checking most_chan for NULL before accessing it.
Fixes: 0fc6432cc78d ("net: ethernet: ti: davinci_cpdma: add weight function for channels")
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/davinci_cpdma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 36518fc..b349d572 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -708,7 +708,8 @@ static void cpdma_chan_set_descs(struct cpdma_ctlr *ctlr,
}
}
/* use remains */
- most_chan->desc_num += desc_cnt;
+ if (most_chan)
+ most_chan->desc_num += desc_cnt;
}
/**
--
2.10.1.dirty
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix access to uninitialized variable in cpdma_chan_set_descs()
2016-12-28 23:42 [PATCH] net: ethernet: ti: davinci_cpdma: fix access to uninitialized variable in cpdma_chan_set_descs() Grygorii Strashko
@ 2016-12-29 1:49 ` Ivan Khoronzhuk
2016-12-29 16:04 ` Grygorii Strashko
0 siblings, 1 reply; 3+ messages in thread
From: Ivan Khoronzhuk @ 2016-12-29 1:49 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap
On Wed, Dec 28, 2016 at 05:42:13PM -0600, Grygorii Strashko wrote:
Grygorii,
> Now below code sequence causes "Unable to handle kernel NULL pointer
> dereference.." exception and system crash during CPSW CPDMA initialization:
>
> cpsw_probe
> |-cpdma_chan_create (TX channel)
> |-cpdma_chan_split_pool
> |-cpdma_chan_set_descs(for TX channels)
> |-cpdma_chan_set_descs(for RX channels) [1]
>
> - and -
> static void cpdma_chan_set_descs(struct cpdma_ctlr *ctlr,
> int rx, int desc_num,
> int per_ch_desc)
> {
> struct cpdma_chan *chan, *most_chan = NULL;
>
> ...
>
> for (i = min; i < max; i++) {
> chan = ctlr->channels[i];
> if (!chan)
> continue;
> ...
>
> if (most_dnum < chan->desc_num) {
> most_dnum = chan->desc_num;
> most_chan = chan;
> }
> }
> /* use remains */
> most_chan->desc_num += desc_cnt; [2]
> }
>
> So, most_chan value will never be reassigned when cpdma_chan_set_descs() is
> called second time [1], because there are no RX channels yet and system
> will crash at [2].
How did you get this?
I just remember as I fixed it before sending patchset.
Maybe it was some experiment with it.
I just wonder and want to find actual reason what's happening.
Look bellow:
cpsw_probe
|-cpdma_chan_create (TX channel)
|-cpdma_chan_split_pool
|-cpdma_chan_set_descs(for TX channels)
|-cpdma_chan_set_descs(for RX channels) [1]
|-cpdma_chan_set_descs(for RX channels) in case you'be described has to be
called with rx_desc_num = 0, because all descs are assigned already for tx
channel. And, if desc_num = 0, cpdma_chan_set_descs just exits and no issues.
So, could you please explain how you get this, in what circumstances.
>
> Hence, fix the issue by checking most_chan for NULL before accessing it.
>
> Fixes: 0fc6432cc78d ("net: ethernet: ti: davinci_cpdma: add weight function for channels")
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/net/ethernet/ti/davinci_cpdma.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 36518fc..b349d572 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -708,7 +708,8 @@ static void cpdma_chan_set_descs(struct cpdma_ctlr *ctlr,
> }
> }
> /* use remains */
> - most_chan->desc_num += desc_cnt;
> + if (most_chan)
> + most_chan->desc_num += desc_cnt;
> }
>
> /**
> --
> 2.10.1.dirty
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix access to uninitialized variable in cpdma_chan_set_descs()
2016-12-29 1:49 ` Ivan Khoronzhuk
@ 2016-12-29 16:04 ` Grygorii Strashko
0 siblings, 0 replies; 3+ messages in thread
From: Grygorii Strashko @ 2016-12-29 16:04 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap
Hi Ivan,
On 12/28/2016 07:49 PM, Ivan Khoronzhuk wrote:
> On Wed, Dec 28, 2016 at 05:42:13PM -0600, Grygorii Strashko wrote:
>> Now below code sequence causes "Unable to handle kernel NULL pointer
>> dereference.." exception and system crash during CPSW CPDMA initialization:
>>
>> cpsw_probe
>> |-cpdma_chan_create (TX channel)
>> |-cpdma_chan_split_pool
>> |-cpdma_chan_set_descs(for TX channels)
>> |-cpdma_chan_set_descs(for RX channels) [1]
>>
>> - and -
>> static void cpdma_chan_set_descs(struct cpdma_ctlr *ctlr,
>> int rx, int desc_num,
>> int per_ch_desc)
>> {
>> struct cpdma_chan *chan, *most_chan = NULL;
>>
>> ...
>>
>> for (i = min; i < max; i++) {
>> chan = ctlr->channels[i];
>> if (!chan)
>> continue;
>> ...
>>
>> if (most_dnum < chan->desc_num) {
>> most_dnum = chan->desc_num;
>> most_chan = chan;
>> }
>> }
>> /* use remains */
>> most_chan->desc_num += desc_cnt; [2]
>> }
>>
>> So, most_chan value will never be reassigned when cpdma_chan_set_descs() is
>> called second time [1], because there are no RX channels yet and system
>> will crash at [2].
>
> How did you get this?
> I just remember as I fixed it before sending patchset.
>
> Maybe it was some experiment with it.
> I just wonder and want to find actual reason what's happening.
>
> Look bellow:
>
> cpsw_probe
> |-cpdma_chan_create (TX channel)
> |-cpdma_chan_split_pool
> |-cpdma_chan_set_descs(for TX channels)
> |-cpdma_chan_set_descs(for RX channels) [1]
>
> |-cpdma_chan_set_descs(for RX channels) in case you'be described has to be
> called with rx_desc_num = 0, because all descs are assigned already for tx
> channel. And, if desc_num = 0, cpdma_chan_set_descs just exits and no issues.
> So, could you please explain how you get this, in what circumstances.
You are right. I've hit this issue while working on other feature which allows
to split pool between RX and TX path and as part of it cpdma_chan_set_descs()
is called with different set of arguments. I probably will just squash it in my changes or
or send as part of my series.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-12-29 16:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-28 23:42 [PATCH] net: ethernet: ti: davinci_cpdma: fix access to uninitialized variable in cpdma_chan_set_descs() Grygorii Strashko
2016-12-29 1:49 ` Ivan Khoronzhuk
2016-12-29 16:04 ` Grygorii Strashko
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).