[ovs-dev] OVS DPDK: dpdk_merge pull request

[Stokes, Ian]

> On Fri, Nov 17, 2017 at 07:27:51PM +0000, Stokes, Ian wrote:
> > The following changes since commit
> 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:
> >
> >   netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29
> > -0800)
> >
> > are available in the git repository at:
> >
> >   https://github.com/istokes/ovs dpdk_merge
> >
> > for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:
> >
> >   netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17
> > 16:26:33 +0000)
> 
> Thanks a lot.  I merged this branch into master.

Thanks Ben.

> 
> This yields a new "sparse" warning:
>     ../lib/netdev-tc-offloads.c:584:20: warning: Variable length array is
> used.
> because of this declaration in parse_put_flow_set_masked_action():
>     char *set_buff[set_len], *set_data, *set_mask;
> 

So this was introduced in commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f: netdev-tc-offloads: Add support for action set. This was pushed just after I had submitted the pull request (but wasn't actually part of the pull request patches).

It's strange though, I ran sparse before submitting the pull request and it came back clean. Checking after the merge and I don't see the warning you see. (Normally I would not submit a pull request if a sparse warning occurred). What version of sparse are you using out of interest? I'm using sparse-0.5.0-10.
 
> It may or may not be worth fixing that.  In favor of fixing it is keeping
> OVS sparse warning free, but against it is that the replacement would
> probably be malloc(), which is slower.
> 
> However, looking a bit closer (now that I've already done the merge), I
> think there is a bug here.  set_buff[] is declared as char
> *set_buff[set_len], which has size (set_len * sizeof(char *)), but only
> set_len bytes of it are used.  I think that the right declaration would be
> more like uint8_t set_buff[set_len];, although that would lack the proper
> alignment for struct nl_attr.
> 
> Maybe something like this would be the appropriate fix for both issues.
> Will you please review it?

Sure, will be happy to review, but would also be good to get someone with tc-offload experience to sign off also.

Roi has also submitted a patch to fix the issue, maybe Roi could review below also?

https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341111.html

> 
> Thanks,
> 
> Ben.
> 
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c index
> 781d849e4a87..d6b61f6360d0 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -581,7 +581,9 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>                                   bool hasmask)  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> -    char *set_buff[set_len], *set_data, *set_mask;
> +    uint64_t set_stub[1024 / 8];
> +    struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
> +    char *set_data, *set_mask;
>      char *key = (char *) &flower->rewrite.key;
>      char *mask = (char *) &flower->rewrite.mask;
>      const struct nlattr *attr;
> @@ -589,8 +591,7 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      size_t size;
> 
>      /* copy so we can set attr mask to 0 for used ovs key struct members
> */
> -    memcpy(set_buff, set, set_len);
> -    attr = (struct nlattr *) set_buff;
> +    attr = ofpbuf_put(&set_buf, set, set_len);
> 
>      type = nl_attr_type(attr);
>      size = nl_attr_get_size(attr) / 2;
> @@ -600,6 +601,7 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      if (type >= ARRAY_SIZE(set_flower_map)
>          || !set_flower_map[type][0].size) {
>          VLOG_DBG_RL(&rl, "unsupported set action type: %d", type);
> +        ofpbuf_uninit(&set_buf);
>          return EOPNOTSUPP;
>      }
> 
> @@ -632,9 +634,11 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
>      if (hasmask && !is_all_zeros(set_mask, size)) {
>          VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type
> %d",
>                      type);
> +        ofpbuf_uninit(&set_buf);
>          return EOPNOTSUPP;
>      }
> 
> +    ofpbuf_uninit(&set_buf);
>      return 0;
>  }
>