Thanks,
Daniele
On Jul 11, 2014, at 10:25 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Fri, Jul 11, 2014 at 10:01 AM, 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>
>> ---
>> v3:
>> Implemented Joe's suggestion:
>> - functions return type is now void
>> Implemented Pravin's suggestions:
>> - Use ovs_key_lens to detect nested attributes
>> - Rename 'parent' to 'is_attr_mask_key'
>> v2:
>> Implemented Pravin's suggestions:
>> - fixed two memory leaks
>> - expanded the comment
>
> LGTM. I pushed to master.
>
> Thanks,
> Pravin.
>
>> ---
>> datapath/flow_netlink.c | 76
>> ++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index f0cf2ed..24794c4 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,26 @@ 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)
>> +/* The nlattr stream should already have been validated */
>> +static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
>> {
>> - u8 *m = (u8 *)&mask->key + range->start;
>> + struct nlattr *nla;
>> + int rem;
>>
>> - mask->range = *range;
>> - memset(m, val, range_n_bytes(range));
>> + nla_for_each_nested(nla, attr, rem) {
>> + /* We assume that ovs_key_lens[type] == -1 means that type
>> is a
>> + * nested attribute
>> + */
>> + if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
>> + nlattr_set(nla, val, false);
>> + else
>> + memset(nla_data(nla), val, nla_len(nla));
>> + }
>> +}
>> +
>> +static void mask_set_nlattr(struct nlattr *attr, u8 val)
>> +{
>> + nlattr_set(attr, val, true);
>> }
>>
>> /**
>> @@ -841,6 +849,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 +896,38 @@ 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;
>> +
>> + mask_set_nlattr(newmask, 0xff);
>> +
>> + 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 +938,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 +951,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
>> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=rXAOYcqDLN3%2BZXLuzKZcZgxFqIt%2FZwRE1iBhE9egwH8%3D%0A&s=c6580761b251d59eb4abc7eaef72b51d5aacc31d0027c3cb325c7a3e17838b4a
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
