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]

Date:	Wed, 7 Jan 2015 22:55:07 +0000
From:	Thomas Graf <tgraf@...g.ch>
To:	Jesse Gross <jesse@...ira.com>
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 01/07/15 at 02:46pm, Jesse Gross wrote:
> 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.

OK. I was planning on always padding vxlan_opts to 4 bytes as well but
I agree with you. It seems clearer to have distinct functions for each
and they can share code where it makes sense.

> > @@ -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.

OK, will do.
--
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