netdev - Re: [ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support

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: <20160810102043.GE5451@penelope.isobedori.kobe.vergenet.net>
Date:	Wed, 10 Aug 2016 12:20:44 +0200
From:	Simon Horman <simon.horman@...ronome.com>
To:	pravin shelar <pshelar@....org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	ovs dev <dev@...nvswitch.org>, Jiri Benc <jbenc@...hat.com>
Subject: Re: [ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3
 flow/port support

On Tue, Aug 09, 2016 at 08:47:40AM -0700, pravin shelar wrote:
> On Mon, Aug 8, 2016 at 8:17 AM, Simon Horman <simon.horman@...ronome.com> wrote:

...

> > Hi Pravin,
> >
> > I have made an attempt to implement your suggestion to the extent that
> > I understand it. The following is an incremental change on top
> > of this patch-set. Does it move things closer to what you have in mind?
> >
> Following approach looks good to me. I have posted couple of comments.

Thanks, I am rather glad to hear that.

> > Light testing seems to indicate that it works for GSO skbs
> > received over both L3 and L2 GRE tunnels by OvS with both
> > IP-in-MPLS and IP (without MPLS) payloads.
> >
> 
> Thanks for testing it. Can you also add those tests to OVS kmod test suite?
> ..

Sure, I will look into doing that.
Am I correct in thinking Joe Stringer is the best person to contact if
I run into trouble there?

> > diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
> > index 2055e57ed1c3..113cba89653d 100644
> > --- a/net/mpls/mpls_gso.c
> > +++ b/net/mpls/mpls_gso.c
> > @@ -39,16 +39,18 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
> >         mpls_features = skb->dev->mpls_features & features;
> >         segs = skb_mac_gso_segment(skb, mpls_features);
> >
> > -
> > -       /* Restore outer protocol. */
> > -       skb->protocol = mpls_protocol;
> > -
> >         /* Re-pull the mac header that the call to skb_mac_gso_segment()
> >          * above pulled.  It will be re-pushed after returning
> >          * skb_mac_gso_segment(), an indirect caller of this function.
> >          */
> >         __skb_pull(skb, skb->data - skb_mac_header(skb));
> >
> > +       /* Restore outer protocol. */
> > +       skb->protocol = mpls_protocol;
> > +       if (!IS_ERR(segs))
> > +               for (skb = segs; skb; skb = skb->next)
> > +                       skb->protocol = mpls_protocol;
> > +
> 
> skb_mac_gso_segment() can also return NULL. Therefore segs should be
> checked for NULL case.

Sure, I will fix that.
I think that can be trivially resolved using IS_ERR_OR_NULL()

> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 0001f651c934..424164222f1e 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> 
> ...
> > @@ -308,8 +319,8 @@ static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> >  {
> >         skb_pull_rcsum(skb, ETH_HLEN);
> >         skb_reset_mac_header(skb);
> > -       skb->mac_len -= ETH_HLEN;
> >
> 
> I am not sure why this line is removed.

I will restore it.