On Thu, Nov 21, 2024 at 03:10:02PM +0100, Paolo Perego wrote: > On Thu, Nov 21, 2024 at 10:37:30AM GMT, Dan Carpenter wrote: > > On Wed, Nov 20, 2024 at 08:54:16PM +0100, Kees Bakker wrote: > > > Op 20-11-2024 om 18:04 schreef Dan Carpenter: > > > > On Wed, Nov 20, 2024 at 03:46:53PM +0100, Paolo Perego wrote: > > > > > This commit fixes a dereference before null check issue discovered by > > > > > Coverity (CID 1601566). > > > > > > > > > > The check ad line 1450 suggests that a_priv can be NULL, however it has > > > > > been derefenced before, in the interface_to_usbdev() call. > > > > > > > > > > Signed-off-by: Paolo Perego <pperego@xxxxxxx> > > > > > --- > > > > You need a Fixes tag. But I'm pretty sure the correct fix is to remove the NULL > > > > check. > > > In the whole agilent_82357a.c module there is no consistency whether > > > board->private_data needs to be checked for a NULL or not. > > > > > > If Dan is correct then it is better to drop the NULL check, not only here > > > but in a few more places as well. There are at least 10 functions were > > > there is no NULL check for private_data. > > > > > > Run this command and you'll see what I mean > > > git grep -3 -e '->private_data' -- drivers/staging/gpib/agilent_82357a > > > > > > > I had looked at similar issue in a different driver: > > https://lore.kernel.org/all/2d99b7a6-f427-4d54-bde7-fb3df5e19e53@stanley.mountain/ > > > > Here the NULL check we are discussing is the same thing. The private data is > > allocated in attach() and freed in detach(). The detach has no need to check > > for NULL because we can't detach something which isn't attached. > > > > The other NULL checks are in agilent_82357a_driver_disconnect(), > > agilent_82357a_driver_suspend() and agilent_82357a_driver_resume(). And there > > the NULL checks are required because it could happen when the driver isn't > > attached. > > > > I also did a quick glance through to see if any of the functions which didn't > > check for NULL should get a NULL check but they all seemed okay because either > > the board was attached or the caller had a NULL check. > > > > Hi all and thanks for the fruitful discussion. > > > So I think we can just remove this one NULL check and everything else makes > > sense. > Please, apologise if I'm too newbie here to understand next step on my > own. Am I asked to do something, to submit a V2 with the correct Tag or > the patch is good as is? > > ( No pressure to be accepted, it's just my willing to understand and go > into the process :-) ) > Please send a v2 which deletes the NULL check. regards, dan carpenter
