| Message ID | 20161027150848.3623829-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. Thanks! Mauro Mauro Carvalho Chehab (3): [media] dvb_net: prepare to split a very complex function [media] dvb-net: split the logic at dvb_net_ule() into other functions [media] dvb_net: simplify the logic that fills the ethernet address drivers/media/dvb-core/dvb_net.c | 927 ++++++++++++++++++++++----------------- 1 file changed, 526 insertions(+), 401 deletions(-)
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. Arnd [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] -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c index 088914c4623f..c32156426463 100644 --- a/drivers/media/dvb-core/dvb_net.c +++ b/drivers/media/dvb-core/dvb_net.c @@ -688,6 +688,9 @@ static void dvb_net_ule( struct net_device *dev, const u8 *buf, size_t buf_len ) ETH_ALEN); skb_pull(priv->ule_skb, ETH_ALEN); } + } else { + /* otherwise use zero destination address */ + eth_zero_addr(dest_addr); } /* Handle ULE Extension Headers. */ @@ -715,13 +718,8 @@ static void dvb_net_ule( struct net_device *dev, const u8 *buf, size_t buf_len ) if (!priv->ule_bridged) { skb_push(priv->ule_skb, ETH_HLEN); ethh = (struct ethhdr *)priv->ule_skb->data; - if (!priv->ule_dbit) { - /* dest_addr buffer is only valid if priv->ule_dbit == 0 */ - memcpy(ethh->h_dest, dest_addr, ETH_ALEN); - eth_zero_addr(ethh->h_source); - } - else /* zeroize source and dest */ - memset( ethh, 0, ETH_ALEN*2 ); + memcpy(ethh->h_dest, dest_addr, ETH_ALEN); + eth_zero_addr(ethh->h_source); ethh->h_proto = htons(priv->ule_sndu_type); }
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 The problem here is that gcc doesn't track all of the conditions to prove it can't end up copying uninitialized data. This changes the logic around so we zero out the destination address earlier when we determine that it is not set here. This allows the compiler to figure it out. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- v2: fix typo pointed out by Jarod Wilson <jarod@redhat.com> drivers/media/dvb-core/dvb_net.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)