Message ID | 20140517191821.GD16662@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-König wrote: > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > index fc4dd7cedc11..6bd7c3f37ac0 100644 > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera( > > pdev = platform_device_alloc("mx3-camera", 0); > if (!pdev) > - goto err; > + return ERR_PTR(-ENOMEM); > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > if (!pdev->dev.dma_mask) Emil, do this one, please and not the second suggestion. Direct returns are more readable. Otherwise, you wonder what the goto is for and where it will take you and be annoyed to discover it is a waste of time, no-op goto. Also you will wonder if platform_device_put() accepts NULL pointers. Thirdly there is a small ugliness that the error code is not preserved. What is the point of setting the error code to -ENOMEM only to discard it? Let's look at that error handling again. err: <-- the name is not descriptive. the location is bad. kfree(pdev->dev.dma_mask); <- null dereference. platform_device_put(pdev); <- ok return ERR_PTR(-ENODEV); <- should be "return ERR_PTR(ret);" 3 out of 4 of the lines are bad. regards, dan carpenter
Hello Uwe, I was to quick to resend the patch, sorry. On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-König wrote: > Hello Emil, > > On Sat, May 17, 2014 at 08:40:33PM +0200, Emil Goode wrote: > > If we fail to allocate struct platform_device pdev we > > dereference it after the goto label err. > > > > I have rearranged the error handling a bit to fix the issue > > and also make it more clear. > > > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > > --- > > v3: Made subject line more specific. > > v2: Changed to return -ENOMEM instead of ret where possible and > > updated the subject line. > > > > arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > Considering that you can fix the issue also by just doing: > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > index fc4dd7cedc11..6bd7c3f37ac0 100644 > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera( > > pdev = platform_device_alloc("mx3-camera", 0); > if (!pdev) > - goto err; > + return ERR_PTR(-ENOMEM); > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > if (!pdev->dev.dma_mask) > > or > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > index fc4dd7cedc11..c609f3667200 100644 > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > @@ -96,7 +96,8 @@ struct platform_device *__init imx_alloc_mx3_camera( > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > if (ret) { > err: > - kfree(pdev->dev.dma_mask); > + if (pdev) > + kfree(pdev->dev.dma_mask); > platform_device_put(pdev); > return ERR_PTR(-ENODEV); > } > > I would prefer one of them as it is easier to justify and for the next > cycle convert the function to platform_device_register_full. Agreed, that makes sense considering the second patch that would convert to platform_device_register_full(). > > Also you should point out in the commit log that the issue was found by > coccinelle. Ok, will do that. Thank you! Best regards, Emil Goode
On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote: > Let's look at that error handling again. > > err: <-- the name is not descriptive. the location is bad. > kfree(pdev->dev.dma_mask); <- null dereference. > platform_device_put(pdev); <- ok > return ERR_PTR(-ENODEV); <- should be "return ERR_PTR(ret);" > > 3 out of 4 of the lines are bad. 2 out of 4. kfree(NULL) is perfectly legal.
Hello Dan, On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote: > On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-König wrote: > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > index fc4dd7cedc11..6bd7c3f37ac0 100644 > > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > > pdev = platform_device_alloc("mx3-camera", 0); > > if (!pdev) > > - goto err; > > + return ERR_PTR(-ENOMEM); > > > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > if (!pdev->dev.dma_mask) > > Emil, do this one, please and not the second suggestion. Yes, I would pick this one to. > > Direct returns are more readable. Otherwise, you wonder what the goto > is for and where it will take you and be annoyed to discover it is a > waste of time, no-op goto. Also you will wonder if platform_device_put() > accepts NULL pointers. Thirdly there is a small ugliness that the error > code is not preserved. What is the point of setting the error code to > -ENOMEM only to discard it? > > Let's look at that error handling again. > > err: <-- the name is not descriptive. the location is bad. > kfree(pdev->dev.dma_mask); <- null dereference. > platform_device_put(pdev); <- ok > return ERR_PTR(-ENODEV); <- should be "return ERR_PTR(ret);" > > 3 out of 4 of the lines are bad. I agree that it's not very pretty. Now that Uwe solved the issue regarding converting the function to platform_device_register_full(), I will look into sending a second patch that would remove these lines. Thank you! Best regards, Emil Goode
On Sat, May 17, 2014 at 11:35:55PM +0100, Russell King - ARM Linux wrote: > On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote: > > Let's look at that error handling again. > > > > err: <-- the name is not descriptive. the location is bad. > > kfree(pdev->dev.dma_mask); <- null dereference. > > platform_device_put(pdev); <- ok > > return ERR_PTR(-ENODEV); <- should be "return ERR_PTR(ret);" > > > > 3 out of 4 of the lines are bad. > > 2 out of 4. kfree(NULL) is perfectly legal. pdev was NULL though... The bug is *caused* by trying to use the same "err" label to do all error handling. This is a very common anti-patern, but if you follow canonical kernel style then your error handling is less buggy. regards, dan carpenter
Hello Russell, On Sat, May 17, 2014 at 11:35:55PM +0100, Russell King - ARM Linux wrote: > On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote: > > Let's look at that error handling again. > > > > err: <-- the name is not descriptive. the location is bad. > > kfree(pdev->dev.dma_mask); <- null dereference. > > platform_device_put(pdev); <- ok > > return ERR_PTR(-ENODEV); <- should be "return ERR_PTR(ret);" > > > > 3 out of 4 of the lines are bad. > > 2 out of 4. kfree(NULL) is perfectly legal. I believe pdev could potentially be NULL, so it's the dereference that is the problem. Best regards, Emil Goode
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index fc4dd7cedc11..6bd7c3f37ac0 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera( pdev = platform_device_alloc("mx3-camera", 0); if (!pdev) - goto err; + return ERR_PTR(-ENOMEM); pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); if (!pdev->dev.dma_mask) or diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index fc4dd7cedc11..c609f3667200 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -96,7 +96,8 @@ struct platform_device *__init imx_alloc_mx3_camera( ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); if (ret) { err: - kfree(pdev->dev.dma_mask); + if (pdev) + kfree(pdev->dev.dma_mask); platform_device_put(pdev); return ERR_PTR(-ENODEV); }