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]).
> 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
