| 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
| ||
|
Message-ID: <CAOrHB_Cy-Gtt3OJSpAdZu3M07xNWv5dYNAbYbD6ei3H-r66vyA@mail.gmail.com>
Date: Thu, 2 Jun 2016 15:02:18 -0700
From: pravin shelar <pshelar@....org>
To: Simon Horman <simon.horman@...ronome.com>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next v10 4/5] openvswitch: add layer 3 flow/port support
On Wed, Jun 1, 2016 at 11:24 PM, 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 vSwitch
> 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>
>
> ---
> v10 [Simon Horman]
> * Move outermost VLAN into skb metadata in pop_eth and
> leave any VLAN as-is in push_eth. The effect is to allow the presence
> of a vlan to be independent of pushing and popping ethernet headers.
> * Omit unnecessary type field from push_eth action
> * Squash with the following patches to make a more complete patch:
> "openvswitch: add layer 3 support to ovs_packet_cmd_execute()"
> "openvswitch: extend layer 3 support to cover non-IP packets"
>
> v9 [Simon Horman]
> * Rebase
> * Minor coding style updates
> * Prohibit push/pop MPLS on l3 packets
> * There are no layer 3 ports supported at this time so only
> send and receive layer 2 packets: that is don't actually
> use this new infrastructure yet
> * Expect that vports that can handle layer 3 packets will: have
> a type other than ARPHRD_IPETHER; can also handle layer 2 packets;
> and that packets can be differentiated by layer 2 packets having
> skb->protocol set to htons(ETH_P_TEB)
>
> v1 - v8 [Lorand Jakub]
>
> wip: fix: openvswitch: add support to push and pop
>
> * Consistently use skb_hdr() in push_eth() by assigning
> its value to a local variable.
> * Limit scope of hdr in push_mpls()
> * Recalculate csum for protocl change in push_mpls.
> - Also needed for pop_mpls?
> - Break out into a fix-patch
>
> Signed-off-by: Simon Horman <simon.horman@...ronome.com>
...
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 15f130e4c22b..5567529904fa 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -300,6 +300,51 @@ static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *flow_key,
> return 0;
> }
>
> +static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + /* Pop outermost VLAN tag to skb metadata unless a VLAN tag
> + * is already present there.
> + */
> + if ((skb->protocol == htons(ETH_P_8021Q) ||
> + skb->protocol == htons(ETH_P_8021AD)) &&
> + !skb_vlan_tag_present(skb)) {
> + int err = skb_vlan_accel(skb);
> + if (unlikely(err))
> + return err;
> + }
> +
I do not think we can keep just the vlan tag and pop ethernet header.
There are multiple issues with this.
First networking stack can not handle suck packet. second issue even
after this patch OVS can not parse this type of packet. third this
patch does not allow pop-eth action on vlan tagged packet.
There is already separate vlan related actions in OVS so lets keep it simple.
> + skb_pull_rcsum(skb, ETH_HLEN);
> + skb_reset_mac_header(skb);
> + skb->mac_len -= ETH_HLEN;
> +
> + invalidate_flow_key(key);
> + return 0;
> +}
> +
...
...
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 0ea128eeeab2..2d9777abcfc9 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. */
> + key->eth.tci = 0;
> + if (key->phy.is_layer3) {
> + if (skb_vlan_tag_present(skb))
> + key->eth.tci = htons(skb->vlan_tci);
> + } else {
> + eth = eth_hdr(skb);
eth can be moved to this block.
> + 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;
> + 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;
> + }
...
> @@ -723,9 +730,19 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
> key->phy.skb_mark = skb->mark;
> ovs_ct_fill_key(skb, key);
> key->ovs_flow_hash = 0;
> + key->phy.is_layer3 = skb->mac_len == 0;
> key->recirc_id = 0;
>
> - return key_extract(skb, key);
> + err = key_extract(skb, key);
> + if (err < 0)
> + return err;
> +
> + if (key->phy.is_layer3)
> + key->eth.type = skb->protocol;
Now key->eth.type is set in three different function, can you
centralize in key_extract()?
> + else if (tun_info && skb->protocol == htons(ETH_P_TEB))
> + skb->protocol = key->eth.type;
> +
> + return err;
> }
>
> int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr,
> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index 03378e75a67c..5395ec0c3c13 100644
...
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 0bb650f4f219..1e1392c3c0ed 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -123,7 +123,7 @@ static void update_range(struct sw_flow_match *match,
> static bool match_validate(const struct sw_flow_match *match,
> u64 key_attrs, u64 mask_attrs, bool log)
> {
> - u64 key_expected = 1 << OVS_KEY_ATTR_ETHERNET;
> + u64 key_expected = 0;
> u64 mask_allowed = key_attrs; /* At most allow all key attributes */
>
> /* The following mask attributes allowed only if they
> @@ -145,6 +145,10 @@ static bool match_validate(const struct sw_flow_match *match,
> | (1 << OVS_KEY_ATTR_IN_PORT)
> | (1 << OVS_KEY_ATTR_ETHERTYPE));
>
> + /* If Ethertype is present, expect MAC addresses */
> + if (key_attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE))
> + key_expected |= 1ULL << OVS_KEY_ATTR_ETHERNET;
> +
> /* Check key attributes. */
> if (match->key->eth.type == htons(ETH_P_ARP)
> || match->key->eth.type == htons(ETH_P_RARP)) {
> @@ -282,7 +286,7 @@ size_t ovs_key_attr_size(void)
> /* Whenever adding new OVS_KEY_ FIELDS, we should consider
> * updating this function.
> */
> - BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 26);
> + BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 27);
>
> return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */
> + nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */
> @@ -355,6 +359,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> [OVS_KEY_ATTR_CT_ZONE] = { .len = sizeof(u16) },
> [OVS_KEY_ATTR_CT_MARK] = { .len = sizeof(u32) },
> [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_labels) },
> + [OVS_KEY_ATTR_PACKET_ETHERTYPE] = { .len = sizeof(__be16) },
> };
>
I do not see need for OVS_KEY_ATTR_PACKET_ETHERTYPE, we can use
existing OVS_KEY_ATTR_ETHERTYPE to serialize the flow key. If there is
no OVS_KEY_ATTR_ETHERNET attribute then its l3 packet.
Powered by blists - more mailing lists