Thanks for the review,
I’ve fixed the two memory leaks and expanded the comment.
I’m about to post a v2. Let mw know if you think the patch could be improved
further.
Daniele
On Jul 10, 2014, at 3:27 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Wed, Jul 9, 2014 at 3:30 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>
>> ---
>> We chose this solution because it seemed generic and immune to changes in
>> struct sw_flow_key. It only needs knowledge of which OVS_KEY_ATTR_* are
>> nested.
>>
>> Given that supporting properly operations with disabled megaflows is only
>> useful for testing purposes, I would prefer discussing more rather that
>> including an improper fix.
>>
>> What do you guys think?
>
> Thanks for fixing it.
>
>> ---
>> datapath/flow_netlink.c | 75
>> +++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 54 insertions(+), 21 deletions(-)
>>
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index f0cf2ed..4f0618d 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);
>> +
>> + /* 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)) {
>> + nlattr_set(nla, val, false);
>> + } else {
>> + memset(nla_data(nla), val, nla_len(nla));
>> + }
>> + }
>>
>> - mask->range = *range;
>> - memset(m, val, range_n_bytes(range));
>> + 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,35 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>> if (err)
>> return err;
>>
>> + if (match->mask && !mask) {
>> + /* Create mask by cloning key attributes and setting them to
>> + * 0xff.
>> + */
> Can you add comment about why do we need to create netlink mask attributes?
>
>> + 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;
>>
> memory leak.
>
>> - 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 +941,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 +954,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;
>>
> memory leak.
>
>> - 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=qu%2FRIGKsq7j%2F1ZkSE2jcz9n%2FJVKqkkwE22OM2tl8%2Fdg%3D%0A&s=61ff5b7e3420eb29511ac4ef5db2452bdff16bd9dcec055ea4f2ddcf9f878c36
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
