Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941438AbcJXT4Z (ORCPT ); Mon, 24 Oct 2016 15:56:25 -0400 Received: from ja.ssi.bg ([178.16.129.10]:59166 "EHLO ja.ssi.bg" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S938739AbcJXT4Y (ORCPT ); Mon, 24 Oct 2016 15:56:24 -0400 X-Greylist: delayed 472 seconds by postgrey-1.27 at vger.kernel.org; Mon, 24 Oct 2016 15:56:23 EDT Date: Mon, 24 Oct 2016 22:47:54 +0300 (EEST) From: Julian Anastasov To: Arnd Bergmann cc: Wensong Zhang , Simon Horman , Pablo Neira Ayuso , Patrick McHardy , Jozsef Kadlecsik , "David S. Miller" , Quentin Armitage , netdev@vger.kernel.org, lvs-devel@vger.kernel.org, netfilter-devel@vger.kernel.org, coreteam@netfilter.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] netfilter: ip_vs_sync: fix bogus maybe-uninitialized warning In-Reply-To: <20161024153454.2766113-1-arnd@arndb.de> Message-ID: References: <20161024153454.2766113-1-arnd@arndb.de> User-Agent: Alpine 2.11 (LFD 23 2013-08-11) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="-1463811672-1337701874-1477338476=:2276" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4082 Lines: 104 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1463811672-1337701874-1477338476=:2276 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT Hello, On Mon, 24 Oct 2016, Arnd Bergmann wrote: > Building the ip_vs_sync code with CONFIG_OPTIMIZE_INLINING on x86 > confuses the compiler to the point where it produces a rather > dubious warning message: > > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.init_seq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > struct ip_vs_sync_conn_options opt; > ^~~ > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.delta’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘opt.previous_delta’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void *)&opt+12).init_seq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void *)&opt+12).delta’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > net/netfilter/ipvs/ip_vs_sync.c:1073:33: error: ‘*((void *)&opt+12).previous_delta’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > The problem appears to be a combination of a number of factors, including > the __builtin_bswap32 compiler builtin being slightly odd, having a large > amount of code inlined into a single function, and the way that some > functions only get partially inlined here. > > I've spent way too much time trying to work out a way to improve the > code, but the best I've come up with is to add an explicit memset > right before the ip_vs_seq structure is first initialized here. When > the compiler works correctly, this has absolutely no effect, but in the > case that produces the warning, the warning disappears. > > In the process of analysing this warning, I also noticed that > we use memcpy to copy the larger ip_vs_sync_conn_options structure > over two members of the ip_vs_conn structure. This works because > the layout is identical, but seems error-prone, so I'm changing > this in the process to directly copy the two members. This change > seemed to have no effect on the object code or the warning, but > it deals with the same data, so I kept the two changes together. > > Signed-off-by: Arnd Bergmann OK, Acked-by: Julian Anastasov I guess, Simon will take the patch for ipvs-next. > --- > net/netfilter/ipvs/ip_vs_sync.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c > index 1b07578bedf3..9350530c16c1 100644 > --- a/net/netfilter/ipvs/ip_vs_sync.c > +++ b/net/netfilter/ipvs/ip_vs_sync.c > @@ -283,6 +283,7 @@ struct ip_vs_sync_buff { > */ > static void ntoh_seq(struct ip_vs_seq *no, struct ip_vs_seq *ho) > { > + memset(ho, 0, sizeof(*ho)); > ho->init_seq = get_unaligned_be32(&no->init_seq); > ho->delta = get_unaligned_be32(&no->delta); > ho->previous_delta = get_unaligned_be32(&no->previous_delta); So, now there is a double write here? What about such constructs?: *ho = (struct ip_vs_seq) { .init_seq = get_unaligned_be32(&no->init_seq), ... }; Any difference in the compiled code or warnings? > @@ -917,8 +918,10 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa > kfree(param->pe_data); > } > > - if (opt) > - memcpy(&cp->in_seq, opt, sizeof(*opt)); > + if (opt) { > + cp->in_seq = opt->in_seq; > + cp->out_seq = opt->out_seq; This fix is fine. > + } > atomic_set(&cp->in_pkts, sysctl_sync_threshold(ipvs)); > cp->state = state; > cp->old_state = cp->state; > -- > 2.9.0 Regards -- Julian Anastasov ---1463811672-1337701874-1477338476=:2276--