Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup()

[Gen Zhang]

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