[ovs-dev] [PATCH v2 14/17] datapath: Fix skb->protocol for vlan frames

Pravin Shelar pshelar at ovn.org
Thu Jan 19 20:17:29 UTC 2017


On Wed, Jan 18, 2017 at 4:53 PM, Yang, Yi <yi.y.yang at intel.com> wrote:
> On Wed, Jan 18, 2017 at 01:29:14PM -0800, Joe Stringer wrote:
>> On 18 January 2017 at 11:54, Eric Garver <e at erig.me> wrote:
>> > On Tue, Jan 17, 2017 at 12:37:19AM +0000, Yang, Yi Y wrote:
>> >> What userspace do "802.1ad patches" depend on? Per Pravin's statement, we just backport 802.1ad patches to ovs, then the below patch can be applied to ovs.
>> >
>> > Userspace does not yet have support for 802.1ad. I'm still working on
>> > it. You can check the list archives for a recent RFC version.
>> >
>> > I don't know if it's acceptable to backport the datapath (kernel module)
>> > support before the userspace support is accepted. If not, you'll have to
>> > wait on the userspace.
>> > Perhaps Pravin can answer.
>>
>> IMO the general method of:
>> 1) Add support upstream
>> 2) Add userspace support
>> 3) Add backport
>>
>> ...works nicely because we get feedback for all interested parties for
>> the APIs in (1), (2) can add tests and be easily tested against a
>> version that works (upstream kernel) and a version that doesn't
>> (version in tree) to ensure both cases are handled in a reasonable
>> way, then (3) allows people on older kernels to gain access to the
>> newer features.
>>
>> That said, if other people are blocking on (3) then I think that piece
>> should be expedited. Let's say (2) and (3) were swapped, it just means
>> we need to be a bit more careful when reviewing/testing to check that
>> the newer userspace still handles old kernels (that lack support)
>> fine.
>>
>> The nice thing about getting the backport earlier is, the closer to
>> upstream we are, the sooner we may find issues that affect the latest
>> code.
>
> If so, I think the below patch is the best solution to current
> situation, it will be automatically overwritten once userspace part and
> 802.1ad backport are ready to merge.
>
It is much easier to backport and review patches when it is done in
same sequence as applied on upstream kernel. It is not just about this
patch. by changing backport order the implementation and review of
both L3 patch series and 802.1AD patch series becomes harder than
necessary.

> So I propose we can merge this patch serial first, then merge Eric
> Garver's rtnetlink patch serial. By then, ovs can completely support L3
> tunnel ports in both compat mode and kernel datapath mode, it will also
> work well on Linux 4.7 and later versions.
>

In general out of tree kernel module does not deviate much from
in-tree ovs module. So there is no need for following patch and we
should backport 802.1AD datapath patches first even before the
userspace support.

> From 127b1c7af10220dacc5b0c6a7c8b4e19d9d4ecac Mon Sep 17 00:00:00 2001
> From: Yi Yang <yi.y.yang at intel.com>
> Date: Wed, 28 Dec 2016 10:27:11 +0800
> Subject: [PATCH v2 14/17] datapath: Fix skb->protocol for vlan frames
>
> Do not always set skb->protocol to be the ethertype of the L3 header.
> For a packet with non-accelerated VLAN tags skb->protocol needs to be the ethertype of the outermost non-accelerated VLAN ethertype.
>
> Any VLAN offloading is undone on the OVS netlink interface, and any VLAN tags added by userspace are non-accelerated, as are double tagged VLAN packets.
>
> Fixes: 018c1dda5f ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes")
> Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
> Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
> Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> ---
>  datapath/datapath.c |  1 -
>  datapath/flow.c     | 42 +++++++++++++++++-------------------------
>  2 files changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 1deb62d..dd19a0e 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -617,7 +617,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>         rcu_assign_pointer(flow->sf_acts, acts);
>         packet->priority = flow->key.phy.priority;
>         packet->mark = flow->key.phy.skb_mark;
> -       packet->protocol = flow->key.eth.type;
>
>         rcu_read_lock();
>         dp = get_dp_rcu(net, ovs_header->dp_ifindex);
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 240bd00..cf35272 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -334,6 +334,7 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>
>         qp = (struct qtag_prefix *) skb->data;
>         key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
> +       key->eth.type = qp->eth_type;
>         __skb_pull(skb, sizeof(struct qtag_prefix));
>
>         return 0;
> @@ -493,6 +494,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);
> @@ -504,11 +506,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>                  */
>
>                 key->eth.tci = 0;
> -               if (skb_vlan_tag_present(skb))
> +               if (skb_vlan_tag_present(skb)) {
>                         key->eth.tci = htons(skb->vlan_tci);
> +                       key->eth.type = skb->vlan_proto;
> +               }
>                 else if (eth->h_proto == htons(ETH_P_8021Q))
>                         if (unlikely(parse_vlan(skb, key)))
>                                 return -ENOMEM;
> +               skb->protocol = key->eth.type;
>
>                 key->eth.type = parse_ethertype(skb);
>                 if (unlikely(key->eth.type == htons(0)))
> @@ -519,7 +524,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>         }
>
>         skb_reset_mac_len(skb);
> -       key->eth.type = skb->protocol;
> +       if (!eth_type_is_vlan(skb->protocol))
> +               skb->protocol = key->eth.type;
>
>         /* Network layer. */
>         if (key->eth.type == htons(ETH_P_IP)) {
> @@ -786,29 +792,15 @@ int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr,
>         if (err)
>                 return err;
>
> -       if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
> -               /* key_extract assumes that skb->protocol is set-up for
> -                * layer 3 packets which is the case for other callers,
> -                * in particular packets recieved from the network stack.
> -                * Here the correct value can be set from the metadata
> -                * extracted above.
> -                */
> -               skb->protocol = key->eth.type;
> -       } else {
> -               struct ethhdr *eth;
> -
> -               skb_reset_mac_header(skb);
> -               eth = eth_hdr(skb);
> -
> -               /* Normally, setting the skb 'protocol' field would be
> -                * handled by a call to eth_type_trans(), but it assumes
> -                * there's a sending device, which we may not have.
> -                */
> -               if (eth_proto_is_802_3(eth->h_proto))
> -                       skb->protocol = eth->h_proto;
> -               else
> -                       skb->protocol = htons(ETH_P_802_2);
> -       }
> +       /* key_extract assumes that skb->protocol is set-up for
> +        * layer 3 packets which is the case for other callers,
> +        * in particular packets recieved from the network stack.
> +        * Here the correct value can be set from the metadata
> +        * extracted above.  For layer 2 packets we initialize
> +        * skb->protocol to zero and set it in key_extract() while
> +        * parsing the L2 headers.
> +        */
> +       skb->protocol = key->eth.type;
>
>         return key_extract(skb, key);
>  }
> --
> 2.1.0
>


More information about the dev mailing list