On Wed, Aug 16, 2017 at 10:03:22PM +0800, Jiri Benc wrote:
> On Wed, 16 Aug 2017 17:31:30 +0800, Yang, Yi wrote:
> > On Wed, Aug 16, 2017 at 11:19:21AM +0200, Jiri Benc wrote:
> > > > --- a/include/uapi/linux/openvswitch.h
> > > > +++ b/include/uapi/linux/openvswitch.h
> > > [...]
> > > > +#define NSH_MD1_CONTEXT_SIZE 4
> > >
> > > Please move this to nsh.h and use it there, too, instead of the open
> > > coded 4.
> >
> > ovs code is very ugly, it will convert array[4] in
> > datapath/linux/compat/include/linux/openvswitch.h to other struct, I
> > have to change context[4] to such format :-), we can use 4 here for
> > Linux kernel.
>
> Oh, right, this is uAPI and nsh.h is kernel internal. My suggestion was
> nonsense, let's keep it as it was in your patch.
>
> > > > + case OVS_KEY_ATTR_NSH: {
> > > > + struct ovs_key_nsh nsh;
> > > > + struct ovs_key_nsh nsh_mask;
> > > > + size_t size = nla_len(a) / 2;
> > > > + struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct
> > > > ovs_key_ipv6)
> > > > + , sizeof(struct
> > > > nlattr))];
> > > > + struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct
> > > > ovs_key_ipv6)
> > > > + , sizeof(struct
> > > > nlattr))];
> > > > +
> > > > + attr->nla_type = nla_type(a);
> > > > + mask->nla_type = attr->nla_type;
> > > > + attr->nla_len = NLA_HDRLEN + size;
> > > > + mask->nla_len = attr->nla_len;
> > > > + memcpy(attr + 1, (char *)(a + 1), size);
> > > > + memcpy(mask + 1, (char *)(a + 1) + size, size);
> > >
> > > This is too hacky. Please find a better way to handle this.
> > >
> > > One option is to create a struct with struct nlattr as the first member
> > > followed by a char buffer. Still not nice but at least it's clear
> > > what's the intent.
> >
> > The issue is nested attributes only can use this way, nested attribute
> > for SET_MASKED is very special, we have to handle it specially.
>
> I'm not sure you understood what I meant. Let me explain in code:
>
> struct {
> struct nlattr attr;
> struct ovs_key_ipv6 data;
> } attr, mask;
>
> attr->attr.nla_type = nla_type(a);
> attr->attr.nl_len = NLA_HDRLEN + size;
> memcpy(&attr->data, a + 1, size);
>
> It's much less hacky and doing the same thing.
>
> I'm not sure we don't need verification of size not overflowing the
> available buffer. Is it checked beforehand elsewhere?
>
> I also spotted one more bug: the 'mask' variable is not used anywhere.
> The second call of nsh_key_from_nlattr should use mask, not attr.
>
I have verified it in my real sfc test environment, but it is indeed
a bug, because mask and attr are same, so the result is still attr.
But here attr and mask shoud be of "struct nlattr attr", I'll polish it.
> > > > + key->nsh.path_hdr = nsh->path_hdr;
> > > > + switch (key->nsh.mdtype) {
> > > > + case NSH_M_TYPE1:
> > > > + if ((length << 2) != NSH_M_TYPE1_LEN)
> > >
> > > Why length << 2?
> >
> > len in NSH header is number of 4 octets, so need to multiply 4.
>
> Should nsh_get_len take care of that, then?
>
There are two helpers for this, nsh_hdr_len gets actual length,
nsh_get_len gets "len" field in nsh header.
> Thanks,
>
> Jiri
