* Re: [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gcu_probe()'
@ 2020-04-29 12:25 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2020-04-29 12:25 UTC (permalink / raw)
To: Christophe JAILLET
Cc: linux-fbdev, eric.y.miao, arnd, b.zolnierkie, gregkh,
kernel-janitors, linux-kernel, dri-devel, zhenzhong.duan, daniel,
mpe, tglx
On Wed, Apr 29, 2020 at 06:34:38AM +0200, Christophe JAILLET wrote:
> If an error occurs in the loop where we call 'pxa3xx_gcu_add_buffer()',
> any resource already allocated should be freed.
>
> In order to fix it, add a call to 'pxa3xx_gcu_free_buffers()' in the error
> handling path, as already done in the remove function.
>
> Fixes: 364dbdf3b6c3 ("video: add driver for PXA3xx 2D graphics accelerator")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/video/fbdev/pxa3xx-gcu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
> index 4279e13a3b58..68d9c7a681d4 100644
> --- a/drivers/video/fbdev/pxa3xx-gcu.c
> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
> @@ -675,6 +675,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>
> err_disable_clk:
> clk_disable_unprepare(priv->clk);
> + pxa3xx_gcu_free_buffers(dev, priv);
The error handling in this function makes no sense and is buggy. It
should be that it unwinds in the reverse order from the allocation. The
goto should be "goto free_most_recently_allocated_resource;". Since the
unwind is done in the wrong order it causes a couple bugs.
These buffers are the last thing which we allocated so they should be
the first thing which we free. In this case, calling
pxa3xx_gcu_free_buffers() before the buffers are allocated is confusing
but harmless. The clk_disable_unprepare() is done on some paths where
the clock was not enabled and that will trigger a WARN() so that's a
bug. Syzcaller will complain and if you have reboot on WARN then it's
annoying.
The second bug is that we don't deregister the misc device or release
the DMA memory on failure when we allocate the buffers in the loop.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gc
@ 2020-04-29 12:25 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2020-04-29 12:25 UTC (permalink / raw)
To: Christophe JAILLET
Cc: linux-fbdev, eric.y.miao, arnd, b.zolnierkie, gregkh,
kernel-janitors, linux-kernel, dri-devel, zhenzhong.duan, daniel,
mpe, tglx
On Wed, Apr 29, 2020 at 06:34:38AM +0200, Christophe JAILLET wrote:
> If an error occurs in the loop where we call 'pxa3xx_gcu_add_buffer()',
> any resource already allocated should be freed.
>
> In order to fix it, add a call to 'pxa3xx_gcu_free_buffers()' in the error
> handling path, as already done in the remove function.
>
> Fixes: 364dbdf3b6c3 ("video: add driver for PXA3xx 2D graphics accelerator")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/video/fbdev/pxa3xx-gcu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
> index 4279e13a3b58..68d9c7a681d4 100644
> --- a/drivers/video/fbdev/pxa3xx-gcu.c
> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
> @@ -675,6 +675,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>
> err_disable_clk:
> clk_disable_unprepare(priv->clk);
> + pxa3xx_gcu_free_buffers(dev, priv);
The error handling in this function makes no sense and is buggy. It
should be that it unwinds in the reverse order from the allocation. The
goto should be "goto free_most_recently_allocated_resource;". Since the
unwind is done in the wrong order it causes a couple bugs.
These buffers are the last thing which we allocated so they should be
the first thing which we free. In this case, calling
pxa3xx_gcu_free_buffers() before the buffers are allocated is confusing
but harmless. The clk_disable_unprepare() is done on some paths where
the clock was not enabled and that will trigger a WARN() so that's a
bug. Syzcaller will complain and if you have reboot on WARN then it's
annoying.
The second bug is that we don't deregister the misc device or release
the DMA memory on failure when we allocate the buffers in the loop.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gcu_probe()'
2020-04-29 12:25 ` [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gc Dan Carpenter
(?)
@ 2020-04-29 17:42 ` Christophe JAILLET
-1 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2020-04-29 17:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: b.zolnierkie, gregkh, mpe, zhenzhong.duan, arnd, tglx,
eric.y.miao, daniel, dri-devel, linux-fbdev, linux-kernel,
kernel-janitors
Le 29/04/2020 à 14:25, Dan Carpenter a écrit :
> On Wed, Apr 29, 2020 at 06:34:38AM +0200, Christophe JAILLET wrote:
>> If an error occurs in the loop where we call 'pxa3xx_gcu_add_buffer()',
>> any resource already allocated should be freed.
>>
>> In order to fix it, add a call to 'pxa3xx_gcu_free_buffers()' in the error
>> handling path, as already done in the remove function.
>>
>> Fixes: 364dbdf3b6c3 ("video: add driver for PXA3xx 2D graphics accelerator")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> drivers/video/fbdev/pxa3xx-gcu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
>> index 4279e13a3b58..68d9c7a681d4 100644
>> --- a/drivers/video/fbdev/pxa3xx-gcu.c
>> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
>> @@ -675,6 +675,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>>
>> err_disable_clk:
>> clk_disable_unprepare(priv->clk);
>> + pxa3xx_gcu_free_buffers(dev, priv);
> The error handling in this function makes no sense and is buggy. It
> should be that it unwinds in the reverse order from the allocation. The
> goto should be "goto free_most_recently_allocated_resource;". Since the
> unwind is done in the wrong order it causes a couple bugs.
>
> These buffers are the last thing which we allocated so they should be
> the first thing which we free. In this case, calling
> pxa3xx_gcu_free_buffers() before the buffers are allocated is confusing
> but harmless. The clk_disable_unprepare() is done on some paths where
> the clock was not enabled and that will trigger a WARN() so that's a
> bug. Syzcaller will complain and if you have reboot on WARN then it's
> annoying.
>
> The second bug is that we don't deregister the misc device or release
> the DMA memory on failure when we allocate the buffers in the loop.
>
> regards,
> dan carpenter
>
Agreed. I've been a little too fast on this one.
I'll update it.
Thx for the review.
CJ
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gcu_probe()'
@ 2020-04-29 17:42 ` Christophe JAILLET
0 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2020-04-29 17:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, eric.y.miao, arnd, b.zolnierkie, gregkh,
kernel-janitors, linux-kernel, dri-devel, zhenzhong.duan, daniel,
mpe, tglx
Le 29/04/2020 à 14:25, Dan Carpenter a écrit :
> On Wed, Apr 29, 2020 at 06:34:38AM +0200, Christophe JAILLET wrote:
>> If an error occurs in the loop where we call 'pxa3xx_gcu_add_buffer()',
>> any resource already allocated should be freed.
>>
>> In order to fix it, add a call to 'pxa3xx_gcu_free_buffers()' in the error
>> handling path, as already done in the remove function.
>>
>> Fixes: 364dbdf3b6c3 ("video: add driver for PXA3xx 2D graphics accelerator")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> drivers/video/fbdev/pxa3xx-gcu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
>> index 4279e13a3b58..68d9c7a681d4 100644
>> --- a/drivers/video/fbdev/pxa3xx-gcu.c
>> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
>> @@ -675,6 +675,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>>
>> err_disable_clk:
>> clk_disable_unprepare(priv->clk);
>> + pxa3xx_gcu_free_buffers(dev, priv);
> The error handling in this function makes no sense and is buggy. It
> should be that it unwinds in the reverse order from the allocation. The
> goto should be "goto free_most_recently_allocated_resource;". Since the
> unwind is done in the wrong order it causes a couple bugs.
>
> These buffers are the last thing which we allocated so they should be
> the first thing which we free. In this case, calling
> pxa3xx_gcu_free_buffers() before the buffers are allocated is confusing
> but harmless. The clk_disable_unprepare() is done on some paths where
> the clock was not enabled and that will trigger a WARN() so that's a
> bug. Syzcaller will complain and if you have reboot on WARN then it's
> annoying.
>
> The second bug is that we don't deregister the misc device or release
> the DMA memory on failure when we allocate the buffers in the loop.
>
> regards,
> dan carpenter
>
Agreed. I've been a little too fast on this one.
I'll update it.
Thx for the review.
CJ
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] video: fbdev: pxa3xx_gcu: Fix some resource leak in an error handling path in 'pxa3xx_gc
@ 2020-04-29 17:42 ` Christophe JAILLET
0 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2020-04-29 17:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, eric.y.miao, arnd, b.zolnierkie, gregkh,
kernel-janitors, linux-kernel, dri-devel, zhenzhong.duan, daniel,
mpe, tglx
Le 29/04/2020 à 14:25, Dan Carpenter a écrit :
> On Wed, Apr 29, 2020 at 06:34:38AM +0200, Christophe JAILLET wrote:
>> If an error occurs in the loop where we call 'pxa3xx_gcu_add_buffer()',
>> any resource already allocated should be freed.
>>
>> In order to fix it, add a call to 'pxa3xx_gcu_free_buffers()' in the error
>> handling path, as already done in the remove function.
>>
>> Fixes: 364dbdf3b6c3 ("video: add driver for PXA3xx 2D graphics accelerator")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> drivers/video/fbdev/pxa3xx-gcu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
>> index 4279e13a3b58..68d9c7a681d4 100644
>> --- a/drivers/video/fbdev/pxa3xx-gcu.c
>> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
>> @@ -675,6 +675,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>>
>> err_disable_clk:
>> clk_disable_unprepare(priv->clk);
>> + pxa3xx_gcu_free_buffers(dev, priv);
> The error handling in this function makes no sense and is buggy. It
> should be that it unwinds in the reverse order from the allocation. The
> goto should be "goto free_most_recently_allocated_resource;". Since the
> unwind is done in the wrong order it causes a couple bugs.
>
> These buffers are the last thing which we allocated so they should be
> the first thing which we free. In this case, calling
> pxa3xx_gcu_free_buffers() before the buffers are allocated is confusing
> but harmless. The clk_disable_unprepare() is done on some paths where
> the clock was not enabled and that will trigger a WARN() so that's a
> bug. Syzcaller will complain and if you have reboot on WARN then it's
> annoying.
>
> The second bug is that we don't deregister the misc device or release
> the DMA memory on failure when we allocate the buffers in the loop.
>
> regards,
> dan carpenter
>
Agreed. I've been a little too fast on this one.
I'll update it.
Thx for the review.
CJ
^ permalink raw reply [flat|nested] 9+ messages in thread