linux-kernel - Re: [PATCH] netfilter: ip_vs_sync: fix bogus maybe-uninitialized warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.11.1610242226310.2276@ja.home.ssi.bg>
Date:   Mon, 24 Oct 2016 22:47:54 +0300 (EEST)
From:   Julian Anastasov <ja@....bg>
To:     Arnd Bergmann <arnd@...db.de>
cc:     Wensong Zhang <wensong@...ux-vs.org>,
        Simon Horman <horms@...ge.net.au>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Patrick McHardy <kaber@...sh.net>,
        Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
        "David S. Miller" <davem@...emloft.net>,
        Quentin Armitage <quentin@...itage.org.uk>,
        netdev@...r.kernel.org, lvs-devel@...r.kernel.org,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netfilter: ip_vs_sync: fix bogus maybe-uninitialized
 warning


	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 <arnd@...db.de>

	OK,

Acked-by: Julian Anastasov <ja@....bg>

	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 <ja@....bg>