| Message ID | 20171219185753.23012-1-e@erig.me |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev,net] openvswitch: Fix pop_vlan action for double tagged frames | expand |
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
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
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.
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)) {
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(-)