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
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
