Subject: Re: [PATCH net-next v10 3/5] openvswitch: add support to push and pop mpls for layer3 packets

From: Simon Horman <[email-masked]>
Date: Tue, 7 Jun 2016 11:51:33 +0900

On Thu, Jun 02, 2016 at 03:02:00PM -0700, pravin shelar wrote:
> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
> <[email-masked]> wrote:
> > Allow push and pop mpls actions to act on layer 3 packets by teaching
> > them not to access non-existent L2 headers of such packets.
> >
> > Signed-off-by: Simon Horman <[email-masked]>
> > ---
> > v10
> > * Limit scope of hdr in {push,pop}_mpls()
> >
> > v9
> > * New Patch
> > ---
> >  net/openvswitch/actions.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 9a3eb7a0ebf4..15f130e4c22b 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -172,7 +172,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> >
> >         skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
> >
> > -       update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
> > +       if (skb->mac_len)
> > +               update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
> We can move all ethernet related code in this if block. for example memmove().

My assumption is that the memmove() does nothing if skb->mac_len is zero
and from my point of view it seems clean to leave it where it is unless
the code around it also moves.

Is there other code you think could/should be moved into the
if (skb->mac_len) block?

> 
> >         if (!skb->inner_protocol)
> >                 skb_set_inner_protocol(skb, skb->protocol);
> >         skb->protocol = mpls->mpls_ethertype;
> > @@ -184,7 +185,6 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> >  static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> >                     const __be16 ethertype)
> >  {
> > -       struct ethhdr *hdr;
> >         int err;
> >
> >         err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
> > @@ -199,11 +199,16 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> >         __skb_pull(skb, MPLS_HLEN);
> >         skb_reset_mac_header(skb);
> >
> > -       /* skb_mpls_header() is used to locate the ethertype
> > -        * field correctly in the presence of VLAN tags.
> > -        */
> > -       hdr = (struct ethhdr *)(skb_mpls_header(skb) - ETH_HLEN);
> > -       update_ethertype(skb, hdr, ethertype);
> > +       if (skb->mac_len) {
> > +               struct ethhdr *hdr;
> > +
> > +               /* skb_mpls_header() is used to locate the ethertype
> > +                * field correctly in the presence of VLAN tags.
> > +                */
> > +               hdr = (struct ethhdr *)(skb_mpls_header(skb) - ETH_HLEN);
> > +               update_ethertype(skb, hdr, ethertype);
> > +       }
> same here.