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

[Julian Anastasov <ja@xxxxxx>]

        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>