netdev - Re: [PATCH 5/6] openvswitch: Rename GENEVE_TUN_OPTS() to TUN_METADATA

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=-X_S1qmMQt3VjOUT9+qUEnxeJxUSf-B90Yktns72zWaA@mail.gmail.com>
Date:	Wed, 7 Jan 2015 14:46:23 -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>,
	netdev <netdev@...r.kernel.org>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH 5/6] openvswitch: Rename GENEVE_TUN_OPTS() to TUN_METADATA_OPTS()

On Tue, Jan 6, 2015 at 6:05 PM, Thomas Graf <tgraf@...g.ch> wrote:
> A subsequent patch will introduce VXLAN options. Rename the existing
> GENEVE_TUN_OPTS() to reflect its extended purpose of carrying generic
> tunnel metadata options.
>
> Signed-off-by: Thomas Graf <tgraf@...g.ch>

This is generally a good idea (even outside of the larger context of
this patch series) although a couple comments below:

> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index d1eecf7..c60ae3f 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -387,20 +387,20 @@ static int parse_flow_nlattrs(const struct nlattr *attr,
>         return __parse_flow_nlattrs(attr, a, attrsp, log, false);
>  }
>
> -static int genev_tun_opt_from_nlattr(const struct nlattr *a,
> -                                    struct sw_flow_match *match, bool is_mask,
> -                                    bool log)
> +static int tun_md_opt_from_nlattr(const struct nlattr *a,
> +                                 struct sw_flow_match *match, bool is_mask,
> +                                 bool log)

I think this is a somewhat overzealous conversion. This function is
verifying OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and as such has some
elements of Geneve protocol knowledge baked in (such as the check that
the options are a multiple of 4 bytes). It's also nice for the log
messages to indicate which netlink key is causing the problem and not
be too generic.

> @@ -1148,10 +1145,10 @@ int ovs_nla_put_flow(const struct sw_flow_key *swkey,
>                 goto nla_put_failure;
>
>         if ((swkey->tun_key.ipv4_dst || is_mask)) {
> -               const struct geneve_opt *opts = NULL;
> +               const void *opts = NULL;
>
>                 if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
> -                       opts = GENEVE_OPTS(output, swkey->tun_opts_len);
> +                       opts = TUN_METADATA_OPTS(output, swkey->tun_opts_len);
>
>                 if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,
>                                        swkey->tun_opts_len))

I think it's probably better to factor this block of code out into a
new function as you have done in the next patch (and include it with
this one). It makes it clearer that it is Geneve-specific.
--
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