[ovs-dev] [RFC v3] datapath/flow_netlink: Create right mask with disabled megaflows
Pravin Shelar
pshelar at nicira.com
Fri Jul 11 17:25:33 UTC 2014
On Fri, Jul 11, 2014 at 10:01 AM, Daniele Di Proietto
<ddiproietto at 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 <alexw at nicira.com>
> Suggested-by: Pravin B Shelar <pshelar at nicira.com>
> Signed-off-by: Daniele Di Proietto <ddiproietto at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list