| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 6 May 2016 14:57:07 +0900
From: Simon Horman <simon.horman@...ronome.com>
To: pravin shelar <pshelar@....org>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
ovs dev <dev@...nvswitch.org>,
Lorand Jakab <lojakab@...co.com>,
Thomas Morin <thomas.morin@...nge.com>,
Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port
support
[CC Jiri Benc]
On Thu, May 05, 2016 at 10:37:08AM -0700, pravin shelar wrote:
> On Wed, May 4, 2016 at 12:36 AM, Simon Horman
> <simon.horman@...ronome.com> wrote:
> > From: Lorand Jakab <lojakab@...co.com>
> >
> > Implementation of the pop_eth and push_eth actions in the kernel, and
> > layer 3 flow support.
> >
> > This doesn't actually do anything yet as no layer 2 tunnel ports are
> > supported yet. The original patch by Lorand was against the Open vSwtich
> > tree which has L2 LISP tunnels but that is not supported in mainline Linux.
> > I (Simon) plan to follow up with support for non-TEB GRE ports based on
> > work by Thomas Morin.
> >
> > Cc: Thomas Morin <thomas.morin@...nge.com>
> > Signed-off-by: Lorand Jakab <lojakab@...co.com>
> > Signed-off-by: Simon Horman <simon.horman@...ronome.com>
> >
> > ---
>
> ...
>
> > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> > index 0ea128eeeab2..6e174ea5f2bb 100644
> > --- a/net/openvswitch/flow.c
> > +++ b/net/openvswitch/flow.c
> > @@ -468,28 +468,31 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> >
> > skb_reset_mac_header(skb);
> >
> > - /* Link layer. We are guaranteed to have at least the 14 byte Ethernet
> > - * header in the linear data area.
> > - */
> > - eth = eth_hdr(skb);
> > - ether_addr_copy(key->eth.src, eth->h_source);
> > - ether_addr_copy(key->eth.dst, eth->h_dest);
> > + /* Link layer. */
> > + if (key->phy.is_layer3) {
> > + key->eth.tci = 0;
> > + key->eth.type = skb->protocol;
> > + } else {
> > + eth = eth_hdr(skb);
> > + ether_addr_copy(key->eth.src, eth->h_source);
> > + ether_addr_copy(key->eth.dst, eth->h_dest);
> >
> > - __skb_pull(skb, 2 * ETH_ALEN);
> > - /* We are going to push all headers that we pull, so no need to
> > - * update skb->csum here.
> > - */
> > + __skb_pull(skb, 2 * ETH_ALEN);
> > + /* We are going to push all headers that we pull, so no need to
> > + * update skb->csum here.
> > + */
> >
> > - key->eth.tci = 0;
> > - if (skb_vlan_tag_present(skb))
> > - key->eth.tci = htons(skb->vlan_tci);
> > - else if (eth->h_proto == htons(ETH_P_8021Q))
> > - if (unlikely(parse_vlan(skb, key)))
> > - return -ENOMEM;
> > + key->eth.tci = 0;
> > + if (skb_vlan_tag_present(skb))
> > + key->eth.tci = htons(skb->vlan_tci);
> > + else if (eth->h_proto == htons(ETH_P_8021Q))
> > + if (unlikely(parse_vlan(skb, key)))
> > + return -ENOMEM;
> >
> > - key->eth.type = parse_ethertype(skb);
> > - if (unlikely(key->eth.type == htons(0)))
> > - return -ENOMEM;
> > + key->eth.type = parse_ethertype(skb);
> > + if (unlikely(key->eth.type == htons(0)))
> > + return -ENOMEM;
> > + }
> >
> > skb_reset_network_header(skb);
> > skb_reset_mac_len(skb);
> > @@ -696,11 +699,23 @@ int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key)
> > int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
> > struct sk_buff *skb, struct sw_flow_key *key)
> > {
> > + bool is_layer3 = false;
> > + bool is_teb = false;
> is_layer3 and is_teb are mutually exclusive, so can't we use single
> boolean here?
Sure, I can do something like the following if you prefer.
To my mind it makes things a bit less readable. But I don't feel
strongly about this.
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index d320c2657627..fc92cf542101 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -701,7 +701,6 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
struct sk_buff *skb, struct sw_flow_key *key)
{
bool is_layer3 = false;
- bool is_teb = false;
int err;
/* Extract metadata from packet. */
@@ -709,13 +708,9 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
key->tun_proto = ip_tunnel_info_af(tun_info);
memcpy(&key->tun_key, &tun_info->key, sizeof(key->tun_key));
- if (OVS_CB(skb)->input_vport->dev->type != ARPHRD_ETHER) {
- if (skb->protocol == htons(ETH_P_TEB))
- is_teb = true;
- else
- is_layer3 = true;
- }
-
+ if (OVS_CB(skb)->input_vport->dev->type != ARPHRD_ETHER &&
+ skb->protocol != htons(ETH_P_TEB))
+ is_layer3 = true;
if (tun_info->options_len) {
BUILD_BUG_ON((1 << (sizeof(tun_info->options_len) *
@@ -746,10 +741,12 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
if (err < 0)
return err;
- if (is_teb)
- skb->protocol = key->eth.type;
- else if (is_layer3)
- key->eth.type = skb->protocol;
+ if (tun_info && OVS_CB(skb)->input_vport->dev->type != ARPHRD_ETHER) {
+ if (is_layer3)
+ key->eth.type = skb->protocol;
+ else
+ skb->protocol = key->eth.type;
+ }
return err;
}
> > + int err;
> > +
> > /* Extract metadata from packet. */
> > if (tun_info) {
> > key->tun_proto = ip_tunnel_info_af(tun_info);
> > memcpy(&key->tun_key, &tun_info->key, sizeof(key->tun_key));
> >
> > + if (OVS_CB(skb)->input_vport->dev->type != ARPHRD_ETHER) {
> > + if (skb->protocol == htons(ETH_P_TEB))
> > + is_teb = true;
> > + else
> > + is_layer3 = true;
> > + }
> > +
> On transmit side you are using mac_len to detect l3 packet, why not do
> same while extracting the key?
Unfortunately mac_len can't be relied on here, emprically it has the same
value (28 in my tests) for both the TEB and layer3 case above.
Perhaps that could be changed by futher enhancements in the tunneling code
but I think things are symetric as they stand:
* On recieve skb->protocol can be read to distinguish TEB and layer3 packets
* On transmit skb->protocol should be set to distinguish TEB and layer3 packets
Powered by blists - more mailing lists