| Message ID | 1464848686-7656-3-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: > * Set skb protocol based on contents of packet. I have observed this is > necessary to get actual protocol of a packet when it is injected into an > internal device e.g. by libnet in which case skb protocol will be set to > ETH_ALL. > > * Set the mac_len which has been observed to not be set up correctly when > an ARP packet is generated and sent via an openvswitch bridge. > My test case is a scenario where there are two open vswtich bridges. > One outputs to a tunnel port which egresses on the other. > > The motivation for this is that support for outputting to layer 3 (non-tap) > GRE tunnels as implemented by a subsequent patch depends on protocol and > mac_len being set correctly on receive. > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > --- > v10 > * Set mac_len > > v9 > * New patch > --- > net/openvswitch/vport-internal_dev.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > index 2ee48e447b72..f89b1efa88f1 100644 > --- a/net/openvswitch/vport-internal_dev.c > +++ b/net/openvswitch/vport-internal_dev.c > @@ -48,6 +48,10 @@ static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev) > { > int len, err; > > + skb->protocol = eth_type_trans(skb, netdev); > + skb_push(skb, ETH_HLEN); > + skb_reset_mac_len(skb); > + resetting mac-len breaks the assumption about mac_len for referencing MPLS header ref: skb_mpls_header().
On Thu, Jun 02, 2016 at 03:01:47PM -0700, pravin shelar wrote: > On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman > <simon.horman@netronome.com> wrote: > > * Set skb protocol based on contents of packet. I have observed this is > > necessary to get actual protocol of a packet when it is injected into an > > internal device e.g. by libnet in which case skb protocol will be set to > > ETH_ALL. > > > > * Set the mac_len which has been observed to not be set up correctly when > > an ARP packet is generated and sent via an openvswitch bridge. > > My test case is a scenario where there are two open vswtich bridges. > > One outputs to a tunnel port which egresses on the other. > > > > The motivation for this is that support for outputting to layer 3 (non-tap) > > GRE tunnels as implemented by a subsequent patch depends on protocol and > > mac_len being set correctly on receive. > > > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > > --- > > v10 > > * Set mac_len > > > > v9 > > * New patch > > --- > > net/openvswitch/vport-internal_dev.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > > index 2ee48e447b72..f89b1efa88f1 100644 > > --- a/net/openvswitch/vport-internal_dev.c > > +++ b/net/openvswitch/vport-internal_dev.c > > @@ -48,6 +48,10 @@ static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev) > > { > > int len, err; > > > > + skb->protocol = eth_type_trans(skb, netdev); > > + skb_push(skb, ETH_HLEN); > > + skb_reset_mac_len(skb); > > + > resetting mac-len breaks the assumption about mac_len for referencing > MPLS header ref: skb_mpls_header(). Thanks I had overlooked this. I think it is actually safe as the mac_len is recalculated quite soon in key_extract() and IIRC the most important thing is for mac_len to be 0 or non-zero for the benefit of ovs_flow_key_extract(). None the less it does seem untidy and moreover inconsistent with the handling in netdev_port_receive() by a latter patch which does the following: eth_type = eth_type_trans(skb, skb->dev); skb->mac_len = skb->data - skb_mac_header(skb); __skb_push(skb, skb->mac_len); if (eth_type == htons(ETH_P_8021Q)) skb->mac_len += VLAN_HLEN; Perhaps that logic ought to be in a helper used by both internal_dev_xmit() and netdev_port_receive(). Or somehow centralised in ovs_vport_receive().
On Mon, Jun 6, 2016 at 8:08 PM, Simon Horman <simon.horman@netronome.com> wrote: > On Thu, Jun 02, 2016 at 03:01:47PM -0700, pravin shelar wrote: >> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman >> <simon.horman@netronome.com> wrote: >> > * Set skb protocol based on contents of packet. I have observed this is >> > necessary to get actual protocol of a packet when it is injected into an >> > internal device e.g. by libnet in which case skb protocol will be set to >> > ETH_ALL. >> > >> > * Set the mac_len which has been observed to not be set up correctly when >> > an ARP packet is generated and sent via an openvswitch bridge. >> > My test case is a scenario where there are two open vswtich bridges. >> > One outputs to a tunnel port which egresses on the other. >> > >> > The motivation for this is that support for outputting to layer 3 (non-tap) >> > GRE tunnels as implemented by a subsequent patch depends on protocol and >> > mac_len being set correctly on receive. >> > >> > Signed-off-by: Simon Horman <simon.horman@netronome.com> >> > >> > --- >> > v10 >> > * Set mac_len >> > >> > v9 >> > * New patch >> > --- >> > net/openvswitch/vport-internal_dev.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c >> > index 2ee48e447b72..f89b1efa88f1 100644 >> > --- a/net/openvswitch/vport-internal_dev.c >> > +++ b/net/openvswitch/vport-internal_dev.c >> > @@ -48,6 +48,10 @@ static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev) >> > { >> > int len, err; >> > >> > + skb->protocol = eth_type_trans(skb, netdev); >> > + skb_push(skb, ETH_HLEN); >> > + skb_reset_mac_len(skb); >> > + >> resetting mac-len breaks the assumption about mac_len for referencing >> MPLS header ref: skb_mpls_header(). > > Thanks I had overlooked this. I think it is actually safe as > the mac_len is recalculated quite soon in key_extract() and IIRC > the most important thing is for mac_len to be 0 or non-zero > for the benefit of ovs_flow_key_extract(). None the less it does > seem untidy and moreover inconsistent with the handling in > netdev_port_receive() by a latter patch which does the following: > > eth_type = eth_type_trans(skb, skb->dev); > skb->mac_len = skb->data - skb_mac_header(skb); > __skb_push(skb, skb->mac_len); > > if (eth_type == htons(ETH_P_8021Q)) > skb->mac_len += VLAN_HLEN; > > Perhaps that logic ought to be in a helper used by both internal_dev_xmit() > and netdev_port_receive(). Or somehow centralised in ovs_vport_receive(). This does looks bit complex. Can we use other skb metadata like skb_mac_header_was_set()?
diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c index 2ee48e447b72..f89b1efa88f1 100644 --- a/net/openvswitch/vport-internal_dev.c +++ b/net/openvswitch/vport-internal_dev.c @@ -48,6 +48,10 @@ static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev) { int len, err; + skb->protocol = eth_type_trans(skb, netdev); + skb_push(skb, ETH_HLEN); + skb_reset_mac_len(skb); + len = skb->len; rcu_read_lock(); err = ovs_vport_receive(internal_dev_priv(netdev)->vport, skb, NULL);
* Set skb protocol based on contents of packet. I have observed this is necessary to get actual protocol of a packet when it is injected into an internal device e.g. by libnet in which case skb protocol will be set to ETH_ALL. * Set the mac_len which has been observed to not be set up correctly when an ARP packet is generated and sent via an openvswitch bridge. My test case is a scenario where there are two open vswtich bridges. One outputs to a tunnel port which egresses on the other. The motivation for this is that support for outputting to layer 3 (non-tap) GRE tunnels as implemented by a subsequent patch depends on protocol and mac_len being set correctly on receive. Signed-off-by: Simon Horman <simon.horman@netronome.com> --- v10 * Set mac_len v9 * New patch --- net/openvswitch/vport-internal_dev.c | 4 ++++ 1 file changed, 4 insertions(+)