* [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
@ 2020-03-15 15:50 Christophe JAILLET
2020-03-16 7:20 ` Peter Ujfalusi
2020-03-18 7:03 ` Peter Ujfalusi
0 siblings, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2020-03-15 15:50 UTC (permalink / raw)
To: vkoul, dan.j.williams, peter.ujfalusi, grygorii.strashko
Cc: dmaengine, linux-kernel, kernel-janitors, Christophe JAILLET
All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
This not correct because this function has a 'channel->flows_ready--;' at
the end, but 'flows_ready' has not been incremented here, when we branch to
the error handling path.
In order to keep a correct value in 'flows_ready', un-roll
'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
at the correct places when an error is detected.
Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Not sure that the last point of the description is correct. Maybe, the
'xudma_rflow_put / return -ENODEV;' should be kept in order not to
override 'flow->udma_rflow'.
---
drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
index dbccdc7c0ed5..890573eb1625 100644
--- a/drivers/dma/ti/k3-udma-glue.c
+++ b/drivers/dma/ti/k3-udma-glue.c
@@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
if (IS_ERR(flow->udma_rflow)) {
ret = PTR_ERR(flow->udma_rflow);
dev_err(dev, "UDMAX rflow get err %d\n", ret);
- goto err;
+ goto err_return;
}
if (flow->udma_rflow_id != xudma_rflow_get_id(flow->udma_rflow)) {
- xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_rflow_put;
}
/* request and cfg rings */
@@ -592,7 +592,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
if (!flow->ringrx) {
ret = -ENODEV;
dev_err(dev, "Failed to get RX ring\n");
- goto err;
+ goto err_rflow_put;
}
flow->ringrxfdq = k3_ringacc_request_ring(rx_chn->common.ringacc,
@@ -600,19 +600,19 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
if (!flow->ringrxfdq) {
ret = -ENODEV;
dev_err(dev, "Failed to get RXFDQ ring\n");
- goto err;
+ goto err_ringrx_free;
}
ret = k3_ringacc_ring_cfg(flow->ringrx, &flow_cfg->rx_cfg);
if (ret) {
dev_err(dev, "Failed to cfg ringrx %d\n", ret);
- goto err;
+ goto err_ringrxfdq_free;
}
ret = k3_ringacc_ring_cfg(flow->ringrxfdq, &flow_cfg->rxfdq_cfg);
if (ret) {
dev_err(dev, "Failed to cfg ringrxfdq %d\n", ret);
- goto err;
+ goto err_ringrxfdq_free;
}
if (rx_chn->remote) {
@@ -662,7 +662,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
if (ret) {
dev_err(dev, "flow%d config failed: %d\n", flow->udma_rflow_id,
ret);
- goto err;
+ goto err_ringrxfdq_free;
}
rx_chn->flows_ready++;
@@ -670,8 +670,18 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
flow->udma_rflow_id, rx_chn->flows_ready);
return 0;
-err:
- k3_udma_glue_release_rx_flow(rx_chn, flow_idx);
+
+err_ringrxfdq_free:
+ k3_ringacc_ring_free(flow->ringrxfdq);
+
+err_ringrx_free:
+ k3_ringacc_ring_free(flow->ringrx);
+
+err_rflow_put:
+ xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
+ flow->udma_rflow = NULL;
+
+err_return:
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
2020-03-15 15:50 [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()' Christophe JAILLET
@ 2020-03-16 7:20 ` Peter Ujfalusi
2020-03-17 7:50 ` Grygorii Strashko
2020-03-18 7:03 ` Peter Ujfalusi
1 sibling, 1 reply; 6+ messages in thread
From: Peter Ujfalusi @ 2020-03-16 7:20 UTC (permalink / raw)
To: Christophe JAILLET, vkoul, dan.j.williams, grygorii.strashko
Cc: dmaengine, linux-kernel, kernel-janitors
Hi Christophe,
On 15/03/2020 17.50, Christophe JAILLET wrote:
> All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
> function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
>
> This not correct because this function has a 'channel->flows_ready--;' at
> the end, but 'flows_ready' has not been incremented here, when we branch to
> the error handling path.
>
> In order to keep a correct value in 'flows_ready', un-roll
> 'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
> at the correct places when an error is detected.
Good catch!
> Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
Even better catch ;)
> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Not sure that the last point of the description is correct. Maybe, the
> 'xudma_rflow_put / return -ENODEV;' should be kept in order not to
> override 'flow->udma_rflow'.
> ---
> drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> index dbccdc7c0ed5..890573eb1625 100644
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c
> @@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (IS_ERR(flow->udma_rflow)) {
> ret = PTR_ERR(flow->udma_rflow);
> dev_err(dev, "UDMAX rflow get err %d\n", ret);
> - goto err;
> + goto err_return;
return err; ?
> }
Optionally you could have moved the
rx_chn->flows_ready++;
here and
>
> if (flow->udma_rflow_id != xudma_rflow_get_id(flow->udma_rflow)) {
> - xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err_rflow_put;
goto err;
> }
>
> /* request and cfg rings */
> @@ -592,7 +592,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (!flow->ringrx) {
> ret = -ENODEV;
> dev_err(dev, "Failed to get RX ring\n");
> - goto err;
> + goto err_rflow_put;
> }
>
> flow->ringrxfdq = k3_ringacc_request_ring(rx_chn->common.ringacc,
> @@ -600,19 +600,19 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (!flow->ringrxfdq) {
> ret = -ENODEV;
> dev_err(dev, "Failed to get RXFDQ ring\n");
> - goto err;
> + goto err_ringrx_free;
> }
>
> ret = k3_ringacc_ring_cfg(flow->ringrx, &flow_cfg->rx_cfg);
> if (ret) {
> dev_err(dev, "Failed to cfg ringrx %d\n", ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> ret = k3_ringacc_ring_cfg(flow->ringrxfdq, &flow_cfg->rxfdq_cfg);
> if (ret) {
> dev_err(dev, "Failed to cfg ringrxfdq %d\n", ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> if (rx_chn->remote) {
> @@ -662,7 +662,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (ret) {
> dev_err(dev, "flow%d config failed: %d\n", flow->udma_rflow_id,
> ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> rx_chn->flows_ready++;
> @@ -670,8 +670,18 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> flow->udma_rflow_id, rx_chn->flows_ready);
>
> return 0;
> -err:
> - k3_udma_glue_release_rx_flow(rx_chn, flow_idx);
> +
> +err_ringrxfdq_free:
> + k3_ringacc_ring_free(flow->ringrxfdq);
> +
> +err_ringrx_free:
> + k3_ringacc_ring_free(flow->ringrx);
> +
> +err_rflow_put:
> + xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
> + flow->udma_rflow = NULL;
> +
> +err_return:
You could have kept the single err label and just copy the
release_rx_flow() without the rx_chn->flows_ready--;
I don't have anything against multiple labels as such, but a single one
might be easier to follow?
and you don't need the err_return, just return in place when you would
jump to it.
> return ret;
> }
>
>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
2020-03-16 7:20 ` Peter Ujfalusi
@ 2020-03-17 7:50 ` Grygorii Strashko
2020-03-17 12:42 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Grygorii Strashko @ 2020-03-17 7:50 UTC (permalink / raw)
To: Peter Ujfalusi, Christophe JAILLET, vkoul, dan.j.williams
Cc: dmaengine, linux-kernel, kernel-janitors
Hi Christophe,
On 16/03/2020 09:20, Peter Ujfalusi wrote:
> Hi Christophe,
>
> On 15/03/2020 17.50, Christophe JAILLET wrote:
>> All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
>> function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
>>
>> This not correct because this function has a 'channel->flows_ready--;' at
>> the end, but 'flows_ready' has not been incremented here, when we branch to
>> the error handling path.
>>
>> In order to keep a correct value in 'flows_ready', un-roll
>> 'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
>> at the correct places when an error is detected.
>
> Good catch!
>
>> Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
>
> Even better catch ;)
>
>> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Not sure that the last point of the description is correct. Maybe, the
>> 'xudma_rflow_put / return -ENODEV;' should be kept in order not to
>> override 'flow->udma_rflow'.
>> ---
>> drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
>> index dbccdc7c0ed5..890573eb1625 100644
>> --- a/drivers/dma/ti/k3-udma-glue.c
>> +++ b/drivers/dma/ti/k3-udma-glue.c
>> @@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>> if (IS_ERR(flow->udma_rflow)) {
>> ret = PTR_ERR(flow->udma_rflow);
>> dev_err(dev, "UDMAX rflow get err %d\n", ret);
>> - goto err;
>> + goto err_return;
>
> return err; ?
>
>> }
>
> Optionally you could have moved the
> rx_chn->flows_ready++;
> here and
Thank you for your patch.
I tend to agree with Peter here - just may be with comment that it will be dec in
k3_udma_glue_release_rx_flow().
All clean ups were moved in standalone function intentionally to avoid
code duplication in err and normal channel release path, and avoid common errors
when normal path is fixed, but err path missed.
>
>>
>> if (flow->udma_rflow_id != xudma_rflow_get_id(flow->udma_rflow)) {
>> - xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
>> - return -ENODEV;
>> + ret = -ENODEV;
>> + goto err_rflow_put;
>
> goto err;
>
>> }
>>
>> /* request and cfg rings */
>> @@ -592,7 +592,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>> if (!flow->ringrx) {
>> ret = -ENODEV;
>> dev_err(dev, "Failed to get RX ring\n");
>> - goto err;
>> + goto err_rflow_put;
>> }
>>
>> flow->ringrxfdq = k3_ringacc_request_ring(rx_chn->common.ringacc,
>> @@ -600,19 +600,19 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>> if (!flow->ringrxfdq) {
>> ret = -ENODEV;
>> dev_err(dev, "Failed to get RXFDQ ring\n");
>> - goto err;
>> + goto err_ringrx_free;
>> }
>>
>> ret = k3_ringacc_ring_cfg(flow->ringrx, &flow_cfg->rx_cfg);
>> if (ret) {
>> dev_err(dev, "Failed to cfg ringrx %d\n", ret);
>> - goto err;
>> + goto err_ringrxfdq_free;
>> }
>>
>> ret = k3_ringacc_ring_cfg(flow->ringrxfdq, &flow_cfg->rxfdq_cfg);
>> if (ret) {
>> dev_err(dev, "Failed to cfg ringrxfdq %d\n", ret);
>> - goto err;
>> + goto err_ringrxfdq_free;
>> }
>>
>> if (rx_chn->remote) {
>> @@ -662,7 +662,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>> if (ret) {
>> dev_err(dev, "flow%d config failed: %d\n", flow->udma_rflow_id,
>> ret);
>> - goto err;
>> + goto err_ringrxfdq_free;
>> }
>>
>> rx_chn->flows_ready++;
>> @@ -670,8 +670,18 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>> flow->udma_rflow_id, rx_chn->flows_ready);
>>
>> return 0;
>> -err:
>> - k3_udma_glue_release_rx_flow(rx_chn, flow_idx);
>> +
>> +err_ringrxfdq_free:
>> + k3_ringacc_ring_free(flow->ringrxfdq);
>> +
>> +err_ringrx_free:
>> + k3_ringacc_ring_free(flow->ringrx);
>> +
>> +err_rflow_put:
>> + xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
>> + flow->udma_rflow = NULL;
>> +
>> +err_return:
>
> You could have kept the single err label and just copy the
> release_rx_flow() without the rx_chn->flows_ready--;
>
> I don't have anything against multiple labels as such, but a single one
> might be easier to follow?
>
> and you don't need the err_return, just return in place when you would
> jump to it.
>
>> return ret;
>> }
>>
>>
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
--
Best regards,
grygorii
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
2020-03-17 7:50 ` Grygorii Strashko
@ 2020-03-17 12:42 ` Dan Carpenter
2020-03-17 12:53 ` Grygorii Strashko
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-03-17 12:42 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Peter Ujfalusi, Christophe JAILLET, vkoul, dan.j.williams,
dmaengine, linux-kernel, kernel-janitors
On Tue, Mar 17, 2020 at 09:50:52AM +0200, Grygorii Strashko wrote:
> Hi Christophe,
>
> On 16/03/2020 09:20, Peter Ujfalusi wrote:
> > Hi Christophe,
> >
> > On 15/03/2020 17.50, Christophe JAILLET wrote:
> > > All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
> > > function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
> > >
> > > This not correct because this function has a 'channel->flows_ready--;' at
> > > the end, but 'flows_ready' has not been incremented here, when we branch to
> > > the error handling path.
> > >
> > > In order to keep a correct value in 'flows_ready', un-roll
> > > 'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
> > > at the correct places when an error is detected.
> >
> > Good catch!
> >
> > > Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
> >
> > Even better catch ;)
> >
> > > Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Not sure that the last point of the description is correct. Maybe, the
> > > 'xudma_rflow_put / return -ENODEV;' should be kept in order not to
> > > override 'flow->udma_rflow'.
> > > ---
> > > drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
> > > 1 file changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> > > index dbccdc7c0ed5..890573eb1625 100644
> > > --- a/drivers/dma/ti/k3-udma-glue.c
> > > +++ b/drivers/dma/ti/k3-udma-glue.c
> > > @@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> > > if (IS_ERR(flow->udma_rflow)) {
> > > ret = PTR_ERR(flow->udma_rflow);
> > > dev_err(dev, "UDMAX rflow get err %d\n", ret);
> > > - goto err;
> > > + goto err_return;
> >
> > return err; ?
> >
> > > }
> >
> > Optionally you could have moved the
> > rx_chn->flows_ready++;
> > here and
>
> Thank you for your patch.
>
> I tend to agree with Peter here - just may be with comment that it will be dec in
> k3_udma_glue_release_rx_flow().
> All clean ups were moved in standalone function intentionally to avoid
> code duplication in err and normal channel release path, and avoid common errors
> when normal path is fixed, but err path missed.
A standalone function to free everything is *always* going to be buggy.
This patch is the classic bug where when you "free everything", you end
up undoing things that haven't been done.
The best way to do error handling is to 1) Free the most recently
allocated resource and 2) Use label names which say what the goto does.
With multiple labels like "goto err_rflow_put;" the review only needs to
ask, what was the most recent allocation? In the case, it was
"udma_rflow" and the "goto err_rflow_put" puts it. That's very simple
and correct. There is no need to scroll to the bottom of the function.
When it comes to line count, if we only free successfully allocated
resources then it means we can remove all the if statements from the
k3_udma_glue_release_rx_flow() so the line count ends up being similar
either way.
The other problem with "common cleanup functions" is that when people
want to audit it, instead of looking at the gotos, reviewers have to
open up two terminal windows and go through it line by line. Currently
static analysis tools are not able to parse common clean functions.
Christophe's patch doesn't just fix the bug he observed, it also fixed
at least one other double free bug. It's quite hard to spot the second
bug, but Christophe fixed it automatically by following the rules.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
2020-03-17 12:42 ` Dan Carpenter
@ 2020-03-17 12:53 ` Grygorii Strashko
0 siblings, 0 replies; 6+ messages in thread
From: Grygorii Strashko @ 2020-03-17 12:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: Peter Ujfalusi, Christophe JAILLET, vkoul, dan.j.williams,
dmaengine, linux-kernel, kernel-janitors
On 17/03/2020 14:42, Dan Carpenter wrote:
> On Tue, Mar 17, 2020 at 09:50:52AM +0200, Grygorii Strashko wrote:
>> Hi Christophe,
>>
>> On 16/03/2020 09:20, Peter Ujfalusi wrote:
>>> Hi Christophe,
>>>
>>> On 15/03/2020 17.50, Christophe JAILLET wrote:
>>>> All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
>>>> function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
>>>>
>>>> This not correct because this function has a 'channel->flows_ready--;' at
>>>> the end, but 'flows_ready' has not been incremented here, when we branch to
>>>> the error handling path.
>>>>
>>>> In order to keep a correct value in 'flows_ready', un-roll
>>>> 'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
>>>> at the correct places when an error is detected.
>>>
>>> Good catch!
>>>
>>>> Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
>>>
>>> Even better catch ;)
>>>
>>>> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> Not sure that the last point of the description is correct. Maybe, the
>>>> 'xudma_rflow_put / return -ENODEV;' should be kept in order not to
>>>> override 'flow->udma_rflow'.
>>>> ---
>>>> drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
>>>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
>>>> index dbccdc7c0ed5..890573eb1625 100644
>>>> --- a/drivers/dma/ti/k3-udma-glue.c
>>>> +++ b/drivers/dma/ti/k3-udma-glue.c
>>>> @@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>>>> if (IS_ERR(flow->udma_rflow)) {
>>>> ret = PTR_ERR(flow->udma_rflow);
>>>> dev_err(dev, "UDMAX rflow get err %d\n", ret);
>>>> - goto err;
>>>> + goto err_return;
>>>
>>> return err; ?
>>>
>>>> }
>>>
>>> Optionally you could have moved the
>>> rx_chn->flows_ready++;
>>> here and
>>
>> Thank you for your patch.
>>
>> I tend to agree with Peter here - just may be with comment that it will be dec in
>> k3_udma_glue_release_rx_flow().
>> All clean ups were moved in standalone function intentionally to avoid
>> code duplication in err and normal channel release path, and avoid common errors
>> when normal path is fixed, but err path missed.
>
> A standalone function to free everything is *always* going to be buggy.
> This patch is the classic bug where when you "free everything", you end
> up undoing things that haven't been done.
>
> The best way to do error handling is to 1) Free the most recently
> allocated resource and 2) Use label names which say what the goto does.
>
> With multiple labels like "goto err_rflow_put;" the review only needs to
> ask, what was the most recent allocation? In the case, it was
> "udma_rflow" and the "goto err_rflow_put" puts it. That's very simple
> and correct. There is no need to scroll to the bottom of the function.
>
> When it comes to line count, if we only free successfully allocated
> resources then it means we can remove all the if statements from the
> k3_udma_glue_release_rx_flow() so the line count ends up being similar
> either way.
>
> The other problem with "common cleanup functions" is that when people
> want to audit it, instead of looking at the gotos, reviewers have to
> open up two terminal windows and go through it line by line. Currently
> static analysis tools are not able to parse common clean functions.
>
> Christophe's patch doesn't just fix the bug he observed, it also fixed
> at least one other double free bug. It's quite hard to spot the second
> bug, but Christophe fixed it automatically by following the rules.
>
fair enough. Thank you.
Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
--
Best regards,
grygorii
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()'
2020-03-15 15:50 [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()' Christophe JAILLET
2020-03-16 7:20 ` Peter Ujfalusi
@ 2020-03-18 7:03 ` Peter Ujfalusi
1 sibling, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2020-03-18 7:03 UTC (permalink / raw)
To: Christophe JAILLET, vkoul, dan.j.williams, grygorii.strashko
Cc: dmaengine, linux-kernel, kernel-janitors
Hi Christophe,
On 15/03/2020 17.50, Christophe JAILLET wrote:
> All but one error handling paths in the 'k3_udma_glue_cfg_rx_flow()'
> function 'goto err' and call 'k3_udma_glue_release_rx_flow()'.
>
> This not correct because this function has a 'channel->flows_ready--;' at
> the end, but 'flows_ready' has not been incremented here, when we branch to
> the error handling path.
>
> In order to keep a correct value in 'flows_ready', un-roll
> 'k3_udma_glue_release_rx_flow()', simplify it, add some labels and branch
> at the correct places when an error is detected.
>
> Doing so, we also NULLify 'flow->udma_rflow' in a path that was lacking it.
>
> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine user")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Can you change the subject to:
dmaengine: ti: k3-udma-glue: ...
> ---
> Not sure that the last point of the description is correct. Maybe, the
> 'xudma_rflow_put / return -ENODEV;' should be kept in order not to
> override 'flow->udma_rflow'.
> ---
> drivers/dma/ti/k3-udma-glue.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> index dbccdc7c0ed5..890573eb1625 100644
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c
> @@ -578,12 +578,12 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (IS_ERR(flow->udma_rflow)) {
> ret = PTR_ERR(flow->udma_rflow);
> dev_err(dev, "UDMAX rflow get err %d\n", ret);
> - goto err;
> + goto err_return;
return ret;
> }
>
> if (flow->udma_rflow_id != xudma_rflow_get_id(flow->udma_rflow)) {
> - xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err_rflow_put;
> }
>
> /* request and cfg rings */
> @@ -592,7 +592,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (!flow->ringrx) {
> ret = -ENODEV;
> dev_err(dev, "Failed to get RX ring\n");
> - goto err;
> + goto err_rflow_put;
> }
>
> flow->ringrxfdq = k3_ringacc_request_ring(rx_chn->common.ringacc,
> @@ -600,19 +600,19 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (!flow->ringrxfdq) {
> ret = -ENODEV;
> dev_err(dev, "Failed to get RXFDQ ring\n");
> - goto err;
> + goto err_ringrx_free;
> }
>
> ret = k3_ringacc_ring_cfg(flow->ringrx, &flow_cfg->rx_cfg);
> if (ret) {
> dev_err(dev, "Failed to cfg ringrx %d\n", ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> ret = k3_ringacc_ring_cfg(flow->ringrxfdq, &flow_cfg->rxfdq_cfg);
> if (ret) {
> dev_err(dev, "Failed to cfg ringrxfdq %d\n", ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> if (rx_chn->remote) {
> @@ -662,7 +662,7 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> if (ret) {
> dev_err(dev, "flow%d config failed: %d\n", flow->udma_rflow_id,
> ret);
> - goto err;
> + goto err_ringrxfdq_free;
> }
>
> rx_chn->flows_ready++;
> @@ -670,8 +670,18 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
> flow->udma_rflow_id, rx_chn->flows_ready);
>
> return 0;
> -err:
> - k3_udma_glue_release_rx_flow(rx_chn, flow_idx);
> +
> +err_ringrxfdq_free:
> + k3_ringacc_ring_free(flow->ringrxfdq);
> +
> +err_ringrx_free:
> + k3_ringacc_ring_free(flow->ringrx);
> +
> +err_rflow_put:
> + xudma_rflow_put(rx_chn->common.udmax, flow->udma_rflow);
> + flow->udma_rflow = NULL;
> +
> +err_return:
and you don't need this label.
> return ret;
> }
>
>
With these changes:
Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-18 7:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-15 15:50 [PATCH] dmaengine: ti: k3-udma: Fix an error handling path in 'k3_udma_glue_cfg_rx_flow()' Christophe JAILLET
2020-03-16 7:20 ` Peter Ujfalusi
2020-03-17 7:50 ` Grygorii Strashko
2020-03-17 12:42 ` Dan Carpenter
2020-03-17 12:53 ` Grygorii Strashko
2020-03-18 7:03 ` Peter Ujfalusi
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).