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@xxxxxxxx>
OK,
Acked-by: Julian Anastasov <ja@xxxxxx>
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@xxxxxx>
