* [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
@ 2017-12-19 18:57 Eric Garver
2017-12-19 19:39 ` Jiri Benc
2017-12-19 19:39 ` Jiri Benc
0 siblings, 2 replies; 6+ messages in thread
From: Eric Garver @ 2017-12-19 18:57 UTC (permalink / raw)
To: netdev; +Cc: Jiri Benc, ovs-dev
skb_vlan_pop() expects skb->protocol to be a valid TPID for double
tagged frames. As such don't override skb->protocol for normal ethernet
frames when extracting the key.
Fixes: 5108bbaddc37 ("openvswitch: add processing of L3 packets")
Signed-off-by: Eric Garver <e@erig.me>
---
net/openvswitch/flow.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index dbe2379329c5..06d32ba3dd69 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
* of a correct length, otherwise the same as skb->network_header.
* For other key->eth.type values it is left untouched.
*
- * - skb->protocol: the type of the data starting at skb->network_header.
- * Equals to key->eth.type.
+ * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
+ * For non-Ethernet, the type of the data starting at skb->network_header
+ * (also equal to key->eth.type).
*/
static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
{
@@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
return -EINVAL;
skb_reset_network_header(skb);
+ key->eth.type = skb->protocol;
} else {
eth = eth_hdr(skb);
ether_addr_copy(key->eth.src, eth->h_source);
@@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
if (unlikely(parse_vlan(skb, key)))
return -ENOMEM;
- skb->protocol = parse_ethertype(skb);
- if (unlikely(skb->protocol == htons(0)))
+ key->eth.type = parse_ethertype(skb);
+ if (unlikely(key->eth.type == htons(0)))
return -ENOMEM;
skb_reset_network_header(skb);
__skb_push(skb, skb->data - skb_mac_header(skb));
}
skb_reset_mac_len(skb);
- key->eth.type = skb->protocol;
/* Network layer. */
if (key->eth.type == htons(ETH_P_IP)) {
--
2.12.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
2017-12-19 18:57 [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames Eric Garver
@ 2017-12-19 19:39 ` Jiri Benc
2017-12-19 20:42 ` Eric Garver
2017-12-19 20:42 ` Eric Garver
2017-12-19 19:39 ` Jiri Benc
1 sibling, 2 replies; 6+ messages in thread
From: Jiri Benc @ 2017-12-19 19:39 UTC (permalink / raw)
To: Eric Garver; +Cc: netdev, ovs-dev
On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> * of a correct length, otherwise the same as skb->network_header.
> * For other key->eth.type values it is left untouched.
> *
> - * - skb->protocol: the type of the data starting at skb->network_header.
> - * Equals to key->eth.type.
> + * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> + * For non-Ethernet, the type of the data starting at skb->network_header
> + * (also equal to key->eth.type).
> */
> static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> {
> @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> return -EINVAL;
>
> skb_reset_network_header(skb);
> + key->eth.type = skb->protocol;
> } else {
> eth = eth_hdr(skb);
> ether_addr_copy(key->eth.src, eth->h_source);
> @@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> if (unlikely(parse_vlan(skb, key)))
> return -ENOMEM;
>
> - skb->protocol = parse_ethertype(skb);
> - if (unlikely(skb->protocol == htons(0)))
> + key->eth.type = parse_ethertype(skb);
> + if (unlikely(key->eth.type == htons(0)))
> return -ENOMEM;
>
> skb_reset_network_header(skb);
> __skb_push(skb, skb->data - skb_mac_header(skb));
> }
> skb_reset_mac_len(skb);
> - key->eth.type = skb->protocol;
>
> /* Network layer. */
> if (key->eth.type == htons(ETH_P_IP)) {
Unfortunately, this does not work. key_extract must set skb->protocol
even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
key_extract, skb->protocol has to be correctly set to the dissected
value.
Which means that we have to check for the existence of inner vlan tag
(by checking key->eth.cvlan.tci or, perhaps better, by returning it
from parse_vlan) and set skb->protocol accordingly.
Jiri
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
2017-12-19 19:39 ` Jiri Benc
@ 2017-12-19 20:42 ` Eric Garver
2017-12-19 20:59 ` Eric Garver
2017-12-19 20:42 ` Eric Garver
1 sibling, 1 reply; 6+ messages in thread
From: Eric Garver @ 2017-12-19 20:42 UTC (permalink / raw)
To: Jiri Benc; +Cc: netdev, ovs-dev
On Tue, Dec 19, 2017 at 08:39:29PM +0100, Jiri Benc wrote:
> On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> > --- a/net/openvswitch/flow.c
> > +++ b/net/openvswitch/flow.c
> > @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > * of a correct length, otherwise the same as skb->network_header.
> > * For other key->eth.type values it is left untouched.
> > *
> > - * - skb->protocol: the type of the data starting at skb->network_header.
> > - * Equals to key->eth.type.
> > + * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> > + * For non-Ethernet, the type of the data starting at skb->network_header
> > + * (also equal to key->eth.type).
> > */
> > static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > {
> > @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > return -EINVAL;
> >
> > skb_reset_network_header(skb);
> > + key->eth.type = skb->protocol;
> > } else {
> > eth = eth_hdr(skb);
> > ether_addr_copy(key->eth.src, eth->h_source);
> > @@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > if (unlikely(parse_vlan(skb, key)))
> > return -ENOMEM;
> >
> > - skb->protocol = parse_ethertype(skb);
> > - if (unlikely(skb->protocol == htons(0)))
> > + key->eth.type = parse_ethertype(skb);
> > + if (unlikely(key->eth.type == htons(0)))
> > return -ENOMEM;
> >
> > skb_reset_network_header(skb);
> > __skb_push(skb, skb->data - skb_mac_header(skb));
> > }
> > skb_reset_mac_len(skb);
> > - key->eth.type = skb->protocol;
> >
> > /* Network layer. */
> > if (key->eth.type == htons(ETH_P_IP)) {
>
> Unfortunately, this does not work. key_extract must set skb->protocol
> even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
> packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
> skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
> key_extract, skb->protocol has to be correctly set to the dissected
> value.
AFAICS, it's always overridden to ETH_P_TEB on output by
ovs_vport_send() and that's the sole reason it works today.
For dissecting, the L2 case is currently setting skb->protocol to the
real ethertype (e.g. 0x800) not ETH_P_TEB. For RX from tunnel case it'll
indeed be ETH_P_TEB.
>
> Which means that we have to check for the existence of inner vlan tag
> (by checking key->eth.cvlan.tci or, perhaps better, by returning it
> from parse_vlan) and set skb->protocol accordingly.
>
> Jiri
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
2017-12-19 20:42 ` Eric Garver
@ 2017-12-19 20:59 ` Eric Garver
0 siblings, 0 replies; 6+ messages in thread
From: Eric Garver @ 2017-12-19 20:59 UTC (permalink / raw)
To: Jiri Benc, netdev, ovs-dev
On Tue, Dec 19, 2017 at 03:42:47PM -0500, Eric Garver wrote:
> On Tue, Dec 19, 2017 at 08:39:29PM +0100, Jiri Benc wrote:
> > On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> > > --- a/net/openvswitch/flow.c
> > > +++ b/net/openvswitch/flow.c
> > > @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > > * of a correct length, otherwise the same as skb->network_header.
> > > * For other key->eth.type values it is left untouched.
> > > *
> > > - * - skb->protocol: the type of the data starting at skb->network_header.
> > > - * Equals to key->eth.type.
> > > + * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> > > + * For non-Ethernet, the type of the data starting at skb->network_header
> > > + * (also equal to key->eth.type).
> > > */
> > > static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > > {
> > > @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > > return -EINVAL;
> > >
> > > skb_reset_network_header(skb);
> > > + key->eth.type = skb->protocol;
> > > } else {
> > > eth = eth_hdr(skb);
> > > ether_addr_copy(key->eth.src, eth->h_source);
> > > @@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > > if (unlikely(parse_vlan(skb, key)))
> > > return -ENOMEM;
> > >
> > > - skb->protocol = parse_ethertype(skb);
> > > - if (unlikely(skb->protocol == htons(0)))
> > > + key->eth.type = parse_ethertype(skb);
> > > + if (unlikely(key->eth.type == htons(0)))
> > > return -ENOMEM;
> > >
> > > skb_reset_network_header(skb);
> > > __skb_push(skb, skb->data - skb_mac_header(skb));
> > > }
> > > skb_reset_mac_len(skb);
> > > - key->eth.type = skb->protocol;
> > >
> > > /* Network layer. */
> > > if (key->eth.type == htons(ETH_P_IP)) {
> >
> > Unfortunately, this does not work. key_extract must set skb->protocol
> > even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
> > packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
> > skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
> > key_extract, skb->protocol has to be correctly set to the dissected
> > value.
>
> AFAICS, it's always overridden to ETH_P_TEB on output by
> ovs_vport_send() and that's the sole reason it works today.
>
> For dissecting, the L2 case is currently setting skb->protocol to the
> real ethertype (e.g. 0x800) not ETH_P_TEB. For RX from tunnel case it'll
> indeed be ETH_P_TEB.
Of course after I hit send I realize what you were saying. I follow now
why skb->protocol needs to be the real ethertype.
> >
> > Which means that we have to check for the existence of inner vlan tag
> > (by checking key->eth.cvlan.tci or, perhaps better, by returning it
> > from parse_vlan) and set skb->protocol accordingly.
I'll see what we can do here. Thanks Jiri.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
2017-12-19 19:39 ` Jiri Benc
2017-12-19 20:42 ` Eric Garver
@ 2017-12-19 20:42 ` Eric Garver
1 sibling, 0 replies; 6+ messages in thread
From: Eric Garver @ 2017-12-19 20:42 UTC (permalink / raw)
To: Jiri Benc; +Cc: netdev, ovs-dev
On Tue, Dec 19, 2017 at 08:39:29PM +0100, Jiri Benc wrote:
> On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> > --- a/net/openvswitch/flow.c
> > +++ b/net/openvswitch/flow.c
> > @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > * of a correct length, otherwise the same as skb->network_header.
> > * For other key->eth.type values it is left untouched.
> > *
> > - * - skb->protocol: the type of the data starting at skb->network_header.
> > - * Equals to key->eth.type.
> > + * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> > + * For non-Ethernet, the type of the data starting at skb->network_header
> > + * (also equal to key->eth.type).
> > */
> > static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > {
> > @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > return -EINVAL;
> >
> > skb_reset_network_header(skb);
> > + key->eth.type = skb->protocol;
> > } else {
> > eth = eth_hdr(skb);
> > ether_addr_copy(key->eth.src, eth->h_source);
> > @@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> > if (unlikely(parse_vlan(skb, key)))
> > return -ENOMEM;
> >
> > - skb->protocol = parse_ethertype(skb);
> > - if (unlikely(skb->protocol == htons(0)))
> > + key->eth.type = parse_ethertype(skb);
> > + if (unlikely(key->eth.type == htons(0)))
> > return -ENOMEM;
> >
> > skb_reset_network_header(skb);
> > __skb_push(skb, skb->data - skb_mac_header(skb));
> > }
> > skb_reset_mac_len(skb);
> > - key->eth.type = skb->protocol;
> >
> > /* Network layer. */
> > if (key->eth.type == htons(ETH_P_IP)) {
>
> Unfortunately, this does not work. key_extract must set skb->protocol
> even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
> packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
> skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
> key_extract, skb->protocol has to be correctly set to the dissected
> value.
AFAICS, it's always overridden to ETH_P_TEB on output by
ovs_vport_send() and that's the sole reason it works today.
For dissecting, the L2 case is currently setting skb->protocol to the
real ethertype (e.g. 0x800) not ETH_P_TEB. For RX from tunnel case it'll
indeed be ETH_P_TEB.
>
> Which means that we have to check for the existence of inner vlan tag
> (by checking key->eth.cvlan.tci or, perhaps better, by returning it
> from parse_vlan) and set skb->protocol accordingly.
>
> Jiri
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames
2017-12-19 18:57 [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames Eric Garver
2017-12-19 19:39 ` Jiri Benc
@ 2017-12-19 19:39 ` Jiri Benc
1 sibling, 0 replies; 6+ messages in thread
From: Jiri Benc @ 2017-12-19 19:39 UTC (permalink / raw)
To: Eric Garver; +Cc: netdev, ovs-dev
On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> * of a correct length, otherwise the same as skb->network_header.
> * For other key->eth.type values it is left untouched.
> *
> - * - skb->protocol: the type of the data starting at skb->network_header.
> - * Equals to key->eth.type.
> + * - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> + * For non-Ethernet, the type of the data starting at skb->network_header
> + * (also equal to key->eth.type).
> */
> static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> {
> @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> return -EINVAL;
>
> skb_reset_network_header(skb);
> + key->eth.type = skb->protocol;
> } else {
> eth = eth_hdr(skb);
> ether_addr_copy(key->eth.src, eth->h_source);
> @@ -592,15 +594,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> if (unlikely(parse_vlan(skb, key)))
> return -ENOMEM;
>
> - skb->protocol = parse_ethertype(skb);
> - if (unlikely(skb->protocol == htons(0)))
> + key->eth.type = parse_ethertype(skb);
> + if (unlikely(key->eth.type == htons(0)))
> return -ENOMEM;
>
> skb_reset_network_header(skb);
> __skb_push(skb, skb->data - skb_mac_header(skb));
> }
> skb_reset_mac_len(skb);
> - key->eth.type = skb->protocol;
>
> /* Network layer. */
> if (key->eth.type == htons(ETH_P_IP)) {
Unfortunately, this does not work. key_extract must set skb->protocol
even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
key_extract, skb->protocol has to be correctly set to the dissected
value.
Which means that we have to check for the existence of inner vlan tag
(by checking key->eth.cvlan.tci or, perhaps better, by returning it
from parse_vlan) and set skb->protocol accordingly.
Jiri
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-19 20:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-19 18:57 [PATCH net] openvswitch: Fix pop_vlan action for double tagged frames Eric Garver
2017-12-19 19:39 ` Jiri Benc
2017-12-19 20:42 ` Eric Garver
2017-12-19 20:59 ` Eric Garver
2017-12-19 20:42 ` Eric Garver
2017-12-19 19:39 ` Jiri Benc
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).