Re: [PATCH 0/3] Avoid warnings about using unitialized dest_dir

[Mauro Carvalho Chehab <mchehab@infradead.org>]

Em Mon, 21 Nov 2016 17:35:42 +0100
Arnd Bergmann <arnd@arndb.de> escreveu:

> On Saturday, November 19, 2016 12:56:57 PM CET Mauro Carvalho Chehab wrote:
> > As Arnd reported:
> > 
> > 	With gcc-5 or higher on x86, we can get a bogus warning in the
> > 	dvb-net code:
> > 
> > 	drivers/media/dvb-core/dvb_net.c: In function ‘dvb_net_ule’:
> > 	arch/x86/include/asm/string_32.h:77:14: error: ‘dest_addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > 	drivers/media/dvb-core/dvb_net.c:633:8: note: ‘dest_addr’ was declared here
> > 
> > Inspecting the code is really hard, as the function Arnd patched is really
> > complex.
> > 
> > IMHO, the best is to first simplify the logic, by breaking parts of it into
> > sub-routines, and then apply a proper fix.
> > 
> > This patch series does that.
> > 
> > Arnd,
> > 
> > After splitting the function, I think that the GCC 5 warning is not bogus,
> > as this code:
> > 		skb_copy_from_linear_data(h->priv->ule_skb, dest_addr,
> > 					  ETH_ALEN);
> >
> > is called before initializing dest_dir, but, even if I'm wrong, it is not a bad
> > idea to zero the dest_addr before handing the logic.  
> 
> My conclusion after looking at it for a while was that it is correct, the
> relevant code is roughtly:
> 
> 	if (!priv->ule_dbit) {
> 		drop = ...;
> 		if (drop)
> 			goto done;
> 		else
> 			skb_copy_from_linear_data(priv->ule_skb, dest_addr, ETH_ALEN);
> 	}
> 
> 	...
> 
> 	if (!priv->ule_dbit) {
> 		memcpy(ethh->h_dest, dest_addr, ETH_ALEN)
> 	}
> 
> done:
> 	...
> 
> 
> So it is always copied from the skb data before it gets used.
> 
> > PS.: I took a lot of care to avoid breaking something on this series, as I don't
> > have any means here to test DVB net.  So, I'd appreciate if you could take
> > a look and see if everything looks fine.  
> 
> I have replaced my patch with your series in my randconfig builds and see no
> new warnings so far.
> 
> The first patch looks correct to me, but I can't really verify the
> second one by inspection. It looks like a nice cleanup and I'd assume
> you did it correctly too. The third patch is probably not needed now,
> I think with the 'goto' removed, gcc will be able to figure it out
> already. It probably adds a few extra cycles to copy the zero data,
> which shouldn't be too bad either.

Thanks for looking into it. I'll apply only the first two
patches for now. If gcc starts complaining again, we can apply
it later.


> [btw, your mchehab@s-opensource.com keeps bouncing for me, I had to
>  remove that from Cc to get my reply to make it out]

Gah! I guess I'll need to switch to use some other e-mail instead.
Maybe it is time to use mchehab@kernel.org for everything.

Thanks,
Mauro