On Fri, Jul 11, 2014 at 9:32 AM, Daniele Di Proietto
<ddiproie...@vmware.com> wrote:
>
> On Jul 10, 2014, at 5:27 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>
>> On Thu, Jul 10, 2014 at 4:29 PM, Daniele Di Proietto
>> <ddiproie...@vmware.com> wrote:
>>> If megaflows are disabled, the userspace does not send the netlink attribute
>>> OVS_FLOW_ATTR_MASK, and the kernel must create an exact match mask.
>>>
>>> sw_flow_mask_set() sets every bytes (in 'range') of the mask to 0xff, even
>>> the
>>> bytes that represent padding for struct sw_flow, or the bytes that represent
>>> fields that may not be set during ovs_flow_extract().
>>> This is a problem, because when we extract a flow from a packet,
>>> we do not memset() anymore the struct sw_flow to 0 (since commit
>>> 9cef26ac6a71).
>>>
>>> This commit gets rid of sw_flow_mask_set() and introduces mask_set_nlattr(),
>>> which operates on the netlink attributes rather than on the mask key. Using
>>> this approach we are sure that only the bytes that the user provided in the
>>> flow are matched.
>>>
>>> Also, if the parse_flow_mask_nlattrs() for the mask ENCAP attribute fails,
>>> we
>>> now return with an error.
>>>
>>> Reported-by: Alex Wang <al...@nicira.com>
>>> Suggested-by: Pravin B Shelar <pshe...@nicira.com>
>>> Signed-off-by: Daniele Di Proietto <ddiproie...@vmware.com>
>>> ---
>>> v2:
>>> Implemented Pravin's suggestions:
>>> - fixed two memory leaks
>>> - expanded the comment
>>> ---
>>
>> I just noticed couple of issues.
>>
>>> datapath/flow_netlink.c | 84
>>> +++++++++++++++++++++++++++++++++++--------------
>>> 1 file changed, 61 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>> index f0cf2ed..d7706a6 100644
>>> --- a/datapath/flow_netlink.c
>>> +++ b/datapath/flow_netlink.c
>>> @@ -106,11 +106,6 @@ static void update_range__(struct sw_flow_match *match,
>>> SW_FLOW_KEY_MEMCPY_OFFSET(match, offsetof(struct sw_flow_key,
>>> field), \
>>> value_p, len, is_mask)
>>>
>>> -static u16 range_n_bytes(const struct sw_flow_key_range *range)
>>> -{
>>> - return range->end - range->start;
>>> -}
>>> -
>>> static bool match_validate(const struct sw_flow_match *match,
>>> u64 key_attrs, u64 mask_attrs)
>>> {
>>> @@ -732,7 +727,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>> *match, u64 attrs,
>>> mpls_key->mpls_lse, is_mask);
>>>
>>> attrs &= ~(1ULL << OVS_KEY_ATTR_MPLS);
>>> - }
>>> + }
>>>
>>> if (attrs & (1ULL << OVS_KEY_ATTR_TCP)) {
>>> const struct ovs_key_tcp *tcp_key;
>>> @@ -814,13 +809,32 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>> *match, u64 attrs,
>>> return 0;
>>> }
>>>
>>> -static void sw_flow_mask_set(struct sw_flow_mask *mask,
>>> - struct sw_flow_key_range *range, u8 val)
>>> +/* We expect the nlattr stream to be already validated */
>>> +static int nlattr_set(struct nlattr *attr, u8 val, bool parent)
>>> {
>>> - u8 *m = (u8 *)&mask->key + range->start;
>>> + struct nlattr *nla;
>>> + int rem;
>>> +
>>> + nla_for_each_nested(nla, attr, rem) {
>>> + u16 type = nla_type(nla);
>>>
>> type should be enum ovs_key_attr
>>
>
> I’m not sure. ‘type' sometimes can contain an enum ovs_tunnel_key_attr (but
> in that case we’re not using the value).
> Elsewhere in the code we use int to store the nla_type() return value.
> If I’m using the ovs_key_lens array I might just remove the variable.
>
>>> - mask->range = *range;
>>> - memset(m, val, range_n_bytes(range));
>>> + /* If we are parsing the parent attribute, we may encounter
>>> + * nested attributes. To deal with them we call ourselves
>>> + */
>>> + if (parent && (type == OVS_KEY_ATTR_ENCAP ||
>>> + type == OVS_KEY_ATTR_TUNNEL)) {
>>
>> These two checks looks arbitrary, can you use ovs_key_lens array
>> already defined, to find nested attributes?
>>
>
> Sure, I'll do that. The reason I didn’t do it in the first place is that a
> value of -1 in ovs_*_key_lens means variable length attribute, not just
> nested attribute (e.g. ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS]).
>
I see that -1 is not consistently used for nested attributes across
all netlink attribute types. But we can just use ovs_key_attr and add
comment for the assumption.
>> We can not compare type from nested attribute to enum of ovs_key_attr.
>> I think that's why you have introduced parent argument. in that case
>> we need to rename parent to some better name to make it cleaner.
>>
>
> Yes, that’s why I introduced parent. I’ll give it a better name
>
>> Thanks,
>> Pravin.
>
> I’ll post a v3 with the suggested changes.
>
> Thanks,
> Daniele
>
>>
>>> + nlattr_set(nla, val, false);
>>> + } else {
>>> + memset(nla_data(nla), val, nla_len(nla));
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int mask_set_nlattr(struct nlattr *attr, u8 val)
>>> +{
>>> + return nlattr_set(attr, val, true);
>>> }
>>>
>>> /**
>>> @@ -841,6 +855,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>> {
>>> const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
>>> const struct nlattr *encap;
>>> + struct nlattr *newmask = NULL;
>>> u64 key_attrs = 0;
>>> u64 mask_attrs = 0;
>>> bool encap_valid = false;
>>> @@ -887,18 +902,40 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>> if (err)
>>> return err;
>>>
>>> + if (match->mask && !mask) {
>>> + /* Create an exact match mask. We need to set to 0xff all
>>> the
>>> + * 'match->mask' fields that have been touched in
>>> 'match->key'.
>>> + * We cannot simply memset 'match->mask', because padding
>>> bytes
>>> + * and fields not specified in 'match->key' should be left
>>> to 0.
>>> + * Instead, we use a stream of netlink attributes, copied
>>> from
>>> + * 'key' and set to 0xff: ovs_key_from_nlattrs() will take
>>> care
>>> + * of filling 'match->mask' appropriately.
>>> + */
>>> + newmask = kmemdup(key, nla_total_size(nla_len(key)),
>>> + GFP_KERNEL);
>>> + if (!newmask)
>>> + return -ENOMEM;
>>> +
>>> + err = mask_set_nlattr(newmask, 0xff);
>>> + if (err)
>>> + goto free_newmask;
>>> +
>>> + mask = newmask;
>>> + }
>>> +
>>> if (mask) {
>>> err = parse_flow_mask_nlattrs(mask, a, &mask_attrs);
>>> if (err)
>>> - return err;
>>> + goto free_newmask;
>>>
>>> - if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
>>> + if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
>>> __be16 eth_type = 0;
>>> __be16 tci = 0;
>>>
>>> if (!encap_valid) {
>>> OVS_NLERR("Encap mask attribute is set for
>>> non-VLAN frame.\n");
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>> + goto free_newmask;
>>> }
>>>
>>> mask_attrs &= ~(1ULL << OVS_KEY_ATTR_ENCAP);
>>> @@ -909,10 +946,12 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>> mask_attrs &= ~(1ULL <<
>>> OVS_KEY_ATTR_ETHERTYPE);
>>> encap = a[OVS_KEY_ATTR_ENCAP];
>>> err = parse_flow_mask_nlattrs(encap, a,
>>> &mask_attrs);
>>> + goto free_newmask;
>>> } else {
>>> OVS_NLERR("VLAN frames must have an exact
>>> match on the TPID (mask=%x).\n",
>>> ntohs(eth_type));
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>> + goto free_newmask;
>>> }
>>>
>>> if (a[OVS_KEY_ATTR_VLAN])
>>> @@ -920,23 +959,22 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>>
>>> if (!(tci & htons(VLAN_TAG_PRESENT))) {
>>> OVS_NLERR("VLAN tag present bit must have an
>>> exact match (tci_mask=%x).\n", ntohs(tci));
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>> + goto free_newmask;
>>> }
>>> }
>>>
>>> err = ovs_key_from_nlattrs(match, mask_attrs, a, true);
>>> if (err)
>>> - return err;
>>> - } else {
>>> - /* Populate exact match flow's key mask. */
>>> - if (match->mask)
>>> - sw_flow_mask_set(match->mask, &match->range, 0xff);
>>> + goto free_newmask;
>>> }
>>>
>>> if (!match_validate(match, key_attrs, mask_attrs))
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>>
>>> - return 0;
>>> +free_newmask:
>>> + kfree(newmask);
>>> + return err;
>>> }
>>>
>>> /**
>>> --
>>> 2.0.0
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
