* [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() @ 2019-12-27 2:39 Gen Zhang 2019-12-27 16:01 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 5+ messages in thread From: Gen Zhang @ 2019-12-27 2:39 UTC (permalink / raw) To: nsekhar, bgolaszewski, linux; +Cc: linux-arm-kernel, linux-kernel In evm_led_setup(), the allocation result of platform_device_alloc() and platform_device_add_data() should be checked. Signed-off-by: Gen Zhang <blackgod016574@gmail.com> --- diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c index 9d87d4e..9cd2785 100644 --- a/arch/arm/mach-davinci/board-dm644x-evm.c +++ b/arch/arm/mach-davinci/board-dm644x-evm.c @@ -352,15 +352,20 @@ evm_led_setup(struct i2c_client *client, int gpio, unsigned ngpio, void *c) * device unregistration ... */ evm_led_dev = platform_device_alloc("leds-gpio", 0); - platform_device_add_data(evm_led_dev, + if (!evm_led_dev) + return -ENOMEM; + status = platform_device_add_data(evm_led_dev, &evm_led_data, sizeof evm_led_data); + if (status) + goto err; evm_led_dev->dev.parent = &client->dev; status = platform_device_add(evm_led_dev); - if (status < 0) { - platform_device_put(evm_led_dev); - evm_led_dev = NULL; - } + if (status) + goto err; +err: + platform_device_put(evm_led_dev); + evm_led_dev = NULL; return status; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() 2019-12-27 2:39 [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() Gen Zhang @ 2019-12-27 16:01 ` Russell King - ARM Linux admin 2019-12-28 13:19 ` Gen Zhang 0 siblings, 1 reply; 5+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-27 16:01 UTC (permalink / raw) To: Gen Zhang; +Cc: nsekhar, bgolaszewski, linux-arm-kernel, linux-kernel On Fri, Dec 27, 2019 at 10:39:21AM +0800, Gen Zhang wrote: > In evm_led_setup(), the allocation result of platform_device_alloc() and > platform_device_add_data() should be checked. > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > --- > diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c > index 9d87d4e..9cd2785 100644 > --- a/arch/arm/mach-davinci/board-dm644x-evm.c > +++ b/arch/arm/mach-davinci/board-dm644x-evm.c > @@ -352,15 +352,20 @@ evm_led_setup(struct i2c_client *client, int gpio, unsigned ngpio, void *c) > * device unregistration ... > */ > evm_led_dev = platform_device_alloc("leds-gpio", 0); > - platform_device_add_data(evm_led_dev, > + if (!evm_led_dev) > + return -ENOMEM; > + status = platform_device_add_data(evm_led_dev, > &evm_led_data, sizeof evm_led_data); > + if (status) > + goto err; > > evm_led_dev->dev.parent = &client->dev; > status = platform_device_add(evm_led_dev); > - if (status < 0) { > - platform_device_put(evm_led_dev); > - evm_led_dev = NULL; > - } > + if (status) > + goto err; > +err: > + platform_device_put(evm_led_dev); > + evm_led_dev = NULL; Please look again at the above change very closely. You will want to send an updated patch. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() 2019-12-27 16:01 ` Russell King - ARM Linux admin @ 2019-12-28 13:19 ` Gen Zhang 2019-12-28 13:48 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 5+ messages in thread From: Gen Zhang @ 2019-12-28 13:19 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: nsekhar, bgolaszewski, linux-arm-kernel, linux-kernel On Fri, Dec 27, 2019 at 04:01:42PM +0000, Russell King - ARM Linux admin wrote: > On Fri, Dec 27, 2019 at 10:39:21AM +0800, Gen Zhang wrote: > > In evm_led_setup(), the allocation result of platform_device_alloc() and > > platform_device_add_data() should be checked. > > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > --- > > diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c > > index 9d87d4e..9cd2785 100644 > > --- a/arch/arm/mach-davinci/board-dm644x-evm.c > > +++ b/arch/arm/mach-davinci/board-dm644x-evm.c > > @@ -352,15 +352,20 @@ evm_led_setup(struct i2c_client *client, int gpio, unsigned ngpio, void *c) > > * device unregistration ... > > */ > > evm_led_dev = platform_device_alloc("leds-gpio", 0); > > - platform_device_add_data(evm_led_dev, > > + if (!evm_led_dev) > > + return -ENOMEM; > > + status = platform_device_add_data(evm_led_dev, > > &evm_led_data, sizeof evm_led_data); > > + if (status) > > + goto err; > > > > evm_led_dev->dev.parent = &client->dev; > > status = platform_device_add(evm_led_dev); > > - if (status < 0) { > > - platform_device_put(evm_led_dev); > > - evm_led_dev = NULL; > > - } > > + if (status) > > + goto err; > > +err: > > + platform_device_put(evm_led_dev); > > + evm_led_dev = NULL; > > Please look again at the above change very closely. You will want to > send an updated patch. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Thanks for your reply. You mean the if (state < 0 ) to if (state) or anything else? Please point out directly. Best, Gen ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() 2019-12-28 13:19 ` Gen Zhang @ 2019-12-28 13:48 ` Russell King - ARM Linux admin 2019-12-28 15:40 ` Gen Zhang 0 siblings, 1 reply; 5+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-28 13:48 UTC (permalink / raw) To: Gen Zhang; +Cc: bgolaszewski, nsekhar, linux-kernel, linux-arm-kernel On Sat, Dec 28, 2019 at 09:19:30PM +0800, Gen Zhang wrote: > On Fri, Dec 27, 2019 at 04:01:42PM +0000, Russell King - ARM Linux admin wrote: > > On Fri, Dec 27, 2019 at 10:39:21AM +0800, Gen Zhang wrote: > > > In evm_led_setup(), the allocation result of platform_device_alloc() and > > > platform_device_add_data() should be checked. > > > > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > > --- > > > diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c > > > index 9d87d4e..9cd2785 100644 > > > --- a/arch/arm/mach-davinci/board-dm644x-evm.c > > > +++ b/arch/arm/mach-davinci/board-dm644x-evm.c > > > @@ -352,15 +352,20 @@ evm_led_setup(struct i2c_client *client, int gpio, unsigned ngpio, void *c) > > > * device unregistration ... > > > */ > > > evm_led_dev = platform_device_alloc("leds-gpio", 0); > > > - platform_device_add_data(evm_led_dev, > > > + if (!evm_led_dev) > > > + return -ENOMEM; > > > + status = platform_device_add_data(evm_led_dev, > > > &evm_led_data, sizeof evm_led_data); > > > + if (status) > > > + goto err; > > > > > > evm_led_dev->dev.parent = &client->dev; > > > status = platform_device_add(evm_led_dev); > > > - if (status < 0) { > > > - platform_device_put(evm_led_dev); > > > - evm_led_dev = NULL; > > > - } > > > + if (status) > > > + goto err; > > > +err: > > > + platform_device_put(evm_led_dev); > > > + evm_led_dev = NULL; > > > > Please look again at the above change very closely. You will want to > > send an updated patch. > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > > According to speedtest.net: 11.9Mbps down 500kbps up > > Thanks for your reply. You mean the if (state < 0 ) to if (state) or > anything else? Please point out directly. This is the old everything-successful path through the code: platform_device_alloc() platform_device_add_data() platform_device_add() evm_led_dev is set to the device This is the new everything-successful path through the code: platform_device_alloc() platform_device_add_data() platform_device_add() platform_device_put() evm_led_dev = NULL And, specifically, the code sequence (I quote from your patch): if (status) goto err; err: is very stupid; it might as well not exist at all. Since other code references evm_led_dev, one can assume that we do not want it to be NULL for the success path. So, taking all this together, your patch is very very wrong, and I also find it very worrying too. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() 2019-12-28 13:48 ` Russell King - ARM Linux admin @ 2019-12-28 15:40 ` Gen Zhang 0 siblings, 0 replies; 5+ messages in thread From: Gen Zhang @ 2019-12-28 15:40 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: bgolaszewski, nsekhar, linux-kernel, linux-arm-kernel On Sat, Dec 28, 2019 at 01:48:24PM +0000, Russell King - ARM Linux admin wrote: > This is the old everything-successful path through the code: > > platform_device_alloc() > platform_device_add_data() > platform_device_add() > evm_led_dev is set to the device > > This is the new everything-successful path through the code: > > platform_device_alloc() > platform_device_add_data() > platform_device_add() > platform_device_put() > evm_led_dev = NULL Thanks for your reply. Adding a return may handle this. successful path: platform_device_alloc() platform_device_add_data() platform_device_add() return status error path: platform_device_put() evm_led_dev = NULL return status correct? > > And, specifically, the code sequence (I quote from your patch): > > if (status) > goto err; > err: > > is very stupid; it might as well not exist at all. Well, you have to admit that the result of platform_device_alloc(), platform_device_add_data() and platform_device_add() should be checked, and error should be handled. e.g., there are 124 call sites of platform_device_add_data() in Linux. Only 24 are not checked, and most of them are in arm subsystem. Look forward to deeper discussion of this problem. Best, Gen > > Since other code references evm_led_dev, one can assume that we do > not want it to be NULL for the success path. So, taking all this > together, your patch is very very wrong, and I also find it very > worrying too. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-28 15:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-27 2:39 [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() Gen Zhang 2019-12-27 16:01 ` Russell King - ARM Linux admin 2019-12-28 13:19 ` Gen Zhang 2019-12-28 13:48 ` Russell King - ARM Linux admin 2019-12-28 15:40 ` Gen Zhang
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).