Message ID | 20210209090036.30832-1-kishon@ti.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | dmaengine: ti: k3-udma: Fix NULL pointer dereference error | expand |
Hi Kishon, On 2/9/21 11:00 AM, Kishon Vijay Abraham I wrote: > bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is > already allocated by checking if it has a NON NULL value. For the > error cases, bchan/rchan/tchan/rflow will have error value > and bcdma_get_*() and udma_get_*() considers this as already allocated > (PASS) since the error values are NON NULL. This results in > NULL pointer dereference error while de-referencing > bchan/rchan/tchan/rflow. I think this can happen when a channel request fails and we get a second request coming and faces with the not cleanup up tchan/rchan/bchan/rflow from the previous failure. Interesting that I have not faced with this, but it is a valid oversight from me. > Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation > actually fails. > > Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA") > Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA") Will this patch apply at any of these? 25dcb5dd7b7c does not have BCDMA (bchan) 017794739702 does not contain PKTDMA (tflow) > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/dma/ti/k3-udma.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c > index 298460438bb4..aa4ef583ff83 100644 > --- a/drivers/dma/ti/k3-udma.c > +++ b/drivers/dma/ti/k3-udma.c > @@ -1330,6 +1330,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) > { > struct udma_dev *ud = uc->ud; > enum udma_tp_level tpl; > + int ret; > > if (uc->bchan) { > dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n", > @@ -1347,8 +1348,11 @@ static int bcdma_get_bchan(struct udma_chan *uc) > tpl = ud->bchan_tpl.levels - 1; > > uc->bchan = __udma_reserve_bchan(ud, tpl, -1); > - if (IS_ERR(uc->bchan)) > - return PTR_ERR(uc->bchan); > + if (IS_ERR(uc->bchan)) { > + ret = PTR_ERR(uc->bchan); > + uc->bchan = NULL; > + return ret; > + } > > uc->tchan = uc->bchan; > > @@ -1358,6 +1362,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) > static int udma_get_tchan(struct udma_chan *uc) > { > struct udma_dev *ud = uc->ud; > + int ret; > > if (uc->tchan) { > dev_dbg(ud->dev, "chan%d: already have tchan%d allocated\n", > @@ -1372,8 +1377,11 @@ static int udma_get_tchan(struct udma_chan *uc) > */ > uc->tchan = __udma_reserve_tchan(ud, uc->config.channel_tpl, > uc->config.mapped_channel_id); > - if (IS_ERR(uc->tchan)) > - return PTR_ERR(uc->tchan); > + if (IS_ERR(uc->tchan)) { > + ret = PTR_ERR(uc->tchan); > + uc->tchan = NULL; > + return ret; > + } > > if (ud->tflow_cnt) { > int tflow_id; > @@ -1403,6 +1411,7 @@ static int udma_get_tchan(struct udma_chan *uc) > static int udma_get_rchan(struct udma_chan *uc) > { > struct udma_dev *ud = uc->ud; > + int ret; > > if (uc->rchan) { > dev_dbg(ud->dev, "chan%d: already have rchan%d allocated\n", > @@ -1417,8 +1426,13 @@ static int udma_get_rchan(struct udma_chan *uc) > */ > uc->rchan = __udma_reserve_rchan(ud, uc->config.channel_tpl, > uc->config.mapped_channel_id); > + if (IS_ERR(uc->rchan)) { > + ret = PTR_ERR(uc->rchan); > + uc->rchan = NULL; > + return ret; > + } > > - return PTR_ERR_OR_ZERO(uc->rchan); > + return 0; > } > > static int udma_get_chan_pair(struct udma_chan *uc) > @@ -1472,6 +1486,7 @@ static int udma_get_chan_pair(struct udma_chan *uc) > static int udma_get_rflow(struct udma_chan *uc, int flow_id) > { > struct udma_dev *ud = uc->ud; > + int ret; > > if (!uc->rchan) { > dev_err(ud->dev, "chan%d: does not have rchan??\n", uc->id); > @@ -1485,6 +1500,11 @@ static int udma_get_rflow(struct udma_chan *uc, int flow_id) > } > > uc->rflow = __udma_get_rflow(ud, flow_id); > + if (IS_ERR(uc->rflow)) { > + ret = PTR_ERR(uc->rflow); > + uc->rflow = NULL; > + return ret; > + } > > return PTR_ERR_OR_ZERO(uc->rflow); return 0; > } >
Hi Peter, On 09/02/21 5:53 pm, Péter Ujfalusi wrote: > Hi Kishon, > > On 2/9/21 11:00 AM, Kishon Vijay Abraham I wrote: >> bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is >> already allocated by checking if it has a NON NULL value. For the >> error cases, bchan/rchan/tchan/rflow will have error value >> and bcdma_get_*() and udma_get_*() considers this as already allocated >> (PASS) since the error values are NON NULL. This results in >> NULL pointer dereference error while de-referencing >> bchan/rchan/tchan/rflow. > > I think this can happen when a channel request fails and we get a second > request coming and faces with the not cleanup up tchan/rchan/bchan/rflow > from the previous failure. > Interesting that I have not faced with this, but it is a valid oversight > from me. Thank you for reviewing. Got into this issue when all the PCIe endpoint functions were requesting for a MEMCOPY channel (total 22 endpoint functions) specifically in bcdma_get_bchan() where the scenario you mentioned above happened. Vignesh asked me to fix it for all udma_get_*(). > >> Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation >> actually fails. >> >> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA") >> Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA") > > Will this patch apply at any of these? > 25dcb5dd7b7c does not have BCDMA (bchan) > 017794739702 does not contain PKTDMA (tflow) I can probably split this patch 017794739702 for bchan and 25dcb5dd7b7c for bchan/rchan/tchan/rflow > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/dma/ti/k3-udma.c | 30 +++++++++++++++++++++++++----- >> 1 file changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c >> index 298460438bb4..aa4ef583ff83 100644 >> --- a/drivers/dma/ti/k3-udma.c >> +++ b/drivers/dma/ti/k3-udma.c >> @@ -1330,6 +1330,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) >> { >> struct udma_dev *ud = uc->ud; >> enum udma_tp_level tpl; >> + int ret; >> >> if (uc->bchan) { >> dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n", >> @@ -1347,8 +1348,11 @@ static int bcdma_get_bchan(struct udma_chan *uc) >> tpl = ud->bchan_tpl.levels - 1; >> >> uc->bchan = __udma_reserve_bchan(ud, tpl, -1); >> - if (IS_ERR(uc->bchan)) >> - return PTR_ERR(uc->bchan); >> + if (IS_ERR(uc->bchan)) { >> + ret = PTR_ERR(uc->bchan); >> + uc->bchan = NULL; >> + return ret; >> + } >> >> uc->tchan = uc->bchan; >> >> @@ -1358,6 +1362,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) >> static int udma_get_tchan(struct udma_chan *uc) >> { >> struct udma_dev *ud = uc->ud; >> + int ret; >> >> if (uc->tchan) { >> dev_dbg(ud->dev, "chan%d: already have tchan%d allocated\n", >> @@ -1372,8 +1377,11 @@ static int udma_get_tchan(struct udma_chan *uc) >> */ >> uc->tchan = __udma_reserve_tchan(ud, uc->config.channel_tpl, >> uc->config.mapped_channel_id); >> - if (IS_ERR(uc->tchan)) >> - return PTR_ERR(uc->tchan); >> + if (IS_ERR(uc->tchan)) { >> + ret = PTR_ERR(uc->tchan); >> + uc->tchan = NULL; >> + return ret; >> + } >> >> if (ud->tflow_cnt) { >> int tflow_id; >> @@ -1403,6 +1411,7 @@ static int udma_get_tchan(struct udma_chan *uc) >> static int udma_get_rchan(struct udma_chan *uc) >> { >> struct udma_dev *ud = uc->ud; >> + int ret; >> >> if (uc->rchan) { >> dev_dbg(ud->dev, "chan%d: already have rchan%d allocated\n", >> @@ -1417,8 +1426,13 @@ static int udma_get_rchan(struct udma_chan *uc) >> */ >> uc->rchan = __udma_reserve_rchan(ud, uc->config.channel_tpl, >> uc->config.mapped_channel_id); >> + if (IS_ERR(uc->rchan)) { >> + ret = PTR_ERR(uc->rchan); >> + uc->rchan = NULL; >> + return ret; >> + } >> >> - return PTR_ERR_OR_ZERO(uc->rchan); >> + return 0; >> } >> >> static int udma_get_chan_pair(struct udma_chan *uc) >> @@ -1472,6 +1486,7 @@ static int udma_get_chan_pair(struct udma_chan *uc) >> static int udma_get_rflow(struct udma_chan *uc, int flow_id) >> { >> struct udma_dev *ud = uc->ud; >> + int ret; >> >> if (!uc->rchan) { >> dev_err(ud->dev, "chan%d: does not have rchan??\n", uc->id); >> @@ -1485,6 +1500,11 @@ static int udma_get_rflow(struct udma_chan *uc, int flow_id) >> } >> >> uc->rflow = __udma_get_rflow(ud, flow_id); >> + if (IS_ERR(uc->rflow)) { >> + ret = PTR_ERR(uc->rflow); >> + uc->rflow = NULL; >> + return ret; >> + } >> >> return PTR_ERR_OR_ZERO(uc->rflow); > > return 0; Will fix this. Thanks Kishon
Hi Kishon, On 2/9/21 2:45 PM, Kishon Vijay Abraham I wrote: > Hi Peter, > > On 09/02/21 5:53 pm, Péter Ujfalusi wrote: >> Hi Kishon, >> >> On 2/9/21 11:00 AM, Kishon Vijay Abraham I wrote: >>> bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is >>> already allocated by checking if it has a NON NULL value. For the >>> error cases, bchan/rchan/tchan/rflow will have error value >>> and bcdma_get_*() and udma_get_*() considers this as already allocated >>> (PASS) since the error values are NON NULL. This results in >>> NULL pointer dereference error while de-referencing >>> bchan/rchan/tchan/rflow. >> >> I think this can happen when a channel request fails and we get a second >> request coming and faces with the not cleanup up tchan/rchan/bchan/rflow >> from the previous failure. >> Interesting that I have not faced with this, but it is a valid oversight >> from me. > > Thank you for reviewing. > > Got into this issue when all the PCIe endpoint functions were requesting > for a MEMCOPY channel (total 22 endpoint functions) specifically in > bcdma_get_bchan() where the scenario you mentioned above happened. I see, do we even have 22 bchan allocated for Linux out from the 40? ;) > Vignesh asked me to fix it for all udma_get_*(). Yes, that is the right thing to do, thank you! >> >>> Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation >>> actually fails. >>> >>> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA") >>> Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA") >> >> Will this patch apply at any of these? >> 25dcb5dd7b7c does not have BCDMA (bchan) >> 017794739702 does not contain PKTDMA (tflow) > > I can probably split this patch > 017794739702 for bchan and 25dcb5dd7b7c for bchan/rchan/tchan/rflow the tflow support for PKTDMA makes the tchan fix a bit problematic for backporting, but it might worth a try to split to bcdma and rchan/tchan/rflow patch.
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c index 298460438bb4..aa4ef583ff83 100644 --- a/drivers/dma/ti/k3-udma.c +++ b/drivers/dma/ti/k3-udma.c @@ -1330,6 +1330,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) { struct udma_dev *ud = uc->ud; enum udma_tp_level tpl; + int ret; if (uc->bchan) { dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n", @@ -1347,8 +1348,11 @@ static int bcdma_get_bchan(struct udma_chan *uc) tpl = ud->bchan_tpl.levels - 1; uc->bchan = __udma_reserve_bchan(ud, tpl, -1); - if (IS_ERR(uc->bchan)) - return PTR_ERR(uc->bchan); + if (IS_ERR(uc->bchan)) { + ret = PTR_ERR(uc->bchan); + uc->bchan = NULL; + return ret; + } uc->tchan = uc->bchan; @@ -1358,6 +1362,7 @@ static int bcdma_get_bchan(struct udma_chan *uc) static int udma_get_tchan(struct udma_chan *uc) { struct udma_dev *ud = uc->ud; + int ret; if (uc->tchan) { dev_dbg(ud->dev, "chan%d: already have tchan%d allocated\n", @@ -1372,8 +1377,11 @@ static int udma_get_tchan(struct udma_chan *uc) */ uc->tchan = __udma_reserve_tchan(ud, uc->config.channel_tpl, uc->config.mapped_channel_id); - if (IS_ERR(uc->tchan)) - return PTR_ERR(uc->tchan); + if (IS_ERR(uc->tchan)) { + ret = PTR_ERR(uc->tchan); + uc->tchan = NULL; + return ret; + } if (ud->tflow_cnt) { int tflow_id; @@ -1403,6 +1411,7 @@ static int udma_get_tchan(struct udma_chan *uc) static int udma_get_rchan(struct udma_chan *uc) { struct udma_dev *ud = uc->ud; + int ret; if (uc->rchan) { dev_dbg(ud->dev, "chan%d: already have rchan%d allocated\n", @@ -1417,8 +1426,13 @@ static int udma_get_rchan(struct udma_chan *uc) */ uc->rchan = __udma_reserve_rchan(ud, uc->config.channel_tpl, uc->config.mapped_channel_id); + if (IS_ERR(uc->rchan)) { + ret = PTR_ERR(uc->rchan); + uc->rchan = NULL; + return ret; + } - return PTR_ERR_OR_ZERO(uc->rchan); + return 0; } static int udma_get_chan_pair(struct udma_chan *uc) @@ -1472,6 +1486,7 @@ static int udma_get_chan_pair(struct udma_chan *uc) static int udma_get_rflow(struct udma_chan *uc, int flow_id) { struct udma_dev *ud = uc->ud; + int ret; if (!uc->rchan) { dev_err(ud->dev, "chan%d: does not have rchan??\n", uc->id); @@ -1485,6 +1500,11 @@ static int udma_get_rflow(struct udma_chan *uc, int flow_id) } uc->rflow = __udma_get_rflow(ud, flow_id); + if (IS_ERR(uc->rflow)) { + ret = PTR_ERR(uc->rflow); + uc->rflow = NULL; + return ret; + } return PTR_ERR_OR_ZERO(uc->rflow); }
bcdma_get_*() and udma_get_*() checks if bchan/rchan/tchan/rflow is already allocated by checking if it has a NON NULL value. For the error cases, bchan/rchan/tchan/rflow will have error value and bcdma_get_*() and udma_get_*() considers this as already allocated (PASS) since the error values are NON NULL. This results in NULL pointer dereference error while de-referencing bchan/rchan/tchan/rflow. Reset the value of bchan/rchan/tchan/rflow to NULL if the allocation actually fails. Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA") Fixes: 25dcb5dd7b7c ("dmaengine: ti: New driver for K3 UDMA") Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/dma/ti/k3-udma.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)