netdev - Re: [PATCH 6/6] openvswitch: Support VXLAN Group Policy extension

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEP_g=8TnrwdGiTOB_mKeQSsEakV8y2OU7_ARwi+j9WDYh=Wag@mail.gmail.com>
Date:	Mon, 12 Jan 2015 13:54:55 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	Thomas Graf <tgraf@...g.ch>
Cc:	David Miller <davem@...emloft.net>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Pravin Shelar <pshelar@...ira.com>,
	Tom Herbert <therbert@...gle.com>,
	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 6/6] openvswitch: Support VXLAN Group Policy extension

On Mon, Jan 12, 2015 at 4:26 AM, Thomas Graf <tgraf@...g.ch> wrote:
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 457ccf3..cea492b 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> +static int vxlan_tun_opt_from_nlattr(const struct nlattr *a,
> +                                    struct sw_flow_match *match, bool is_mask,
> +                                    bool log)
> +{
> +       struct nlattr *tb[OVS_VXLAN_EXT_MAX+1];
> +       unsigned long opt_key_offset;
> +       struct ovs_vxlan_opts opts;
> +       int err;
> +
> +       BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
> +
> +       err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
> +       if (err < 0)
> +               return err;
> +
> +       memset(&opts, 0, sizeof(opts));
> +
> +       if (tb[OVS_VXLAN_EXT_MAX])
> +               opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_MAX]);

Shouldn't this be OVS_VXLAN_EXT_GBP instead of OVS_VXLAN_EXT_MAX?
(They have the same value.)

> +       if (!is_mask)
> +               SW_FLOW_KEY_PUT(match, tun_opts_len, sizeof(opts), false);
> +       else
> +               SW_FLOW_KEY_PUT(match, tun_opts_len, 0xff, true);

Have you thought carefully about how the masking model work as other
extensions are potentially added? This was a little tricky with Geneve
because I wanted to be able to match on both "no options present" as
well as wildcard all options. The other interesting thing is how you
serialize them back correctly to userspace, which was the genesis of
the TUNNEL_OPTIONS_PRESENT flag.

My guess is that this may basically work fine now that there is only
one extension present but it is important to think about how it might
work with multiple independent extensions in the future. (I haven't
thought about it, I'm just asking.)

> diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
> index 266c595..dbd6c75 100644
> --- a/net/openvswitch/vport-vxlan.c
> +++ b/net/openvswitch/vport-vxlan.c
> +static int vxlan_ext_gbp(struct sk_buff *skb)
> +{
> +       const struct ovs_tunnel_info *tun_info;
> +       const struct ovs_vxlan_opts *opts;
> +
> +       tun_info = OVS_CB(skb)->egress_tun_info;
> +       opts = tun_info->options;
> +
> +       if (tun_info->options_len >= sizeof(*opts))
> +               return opts->gbp;
> +       else
> +               return 0;
> +}

If you set Geneve options and output to a VXLAN port (or vice versa),
you will get garbage, right? Is there any way that we can sanity check
that?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html