| Message ID | 1464848686-7656-4-git-send-email-simon.horman@netronome.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman <simon.horman@netronome.com> 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 <simon.horman@netronome.com> > --- > 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(). > 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.
On Thu, Jun 02, 2016 at 03:02:00PM -0700, pravin shelar wrote: > On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman > <simon.horman@netronome.com> 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 <simon.horman@netronome.com> > > --- > > 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.
On Mon, Jun 6, 2016 at 7:51 PM, Simon Horman <simon.horman@netronome.com> wrote: > On Thu, Jun 02, 2016 at 03:02:00PM -0700, pravin shelar wrote: >> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman >> <simon.horman@netronome.com> 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 <simon.horman@netronome.com> >> > --- >> > 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? > yes, the code is correct. But I think grouping l2 related updated together makes code easier to follow.
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); 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); + } + if (eth_p_mpls(skb->protocol)) skb->protocol = ethertype;
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 <simon.horman@netronome.com> --- v10 * Limit scope of hdr in {push,pop}_mpls() v9 * New Patch --- net/openvswitch/actions.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)