[ovs-dev] [PATCH] datapath: Fix pop_vlan()

[Jesse Gross]

On Fri, Nov 11, 2011 at 4:23 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/acinclude.m4 b/acinclude.m4
> index 648132a..458b9e5 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -238,6 +238,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_warn_if_lro],
>                   [OVS_DEFINE([HAVE_SKB_WARN_LRO])])
>   OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [consume_skb])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_reset_mac_len])

Usually we don't add tests unless it's known to be backported.  In
this case I'm guessing (although you should check) that it can be
nested under HAVE_SKBUFF_HEADER_HELPERS.

> @@ -254,6 +255,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_get_be16])
>   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_find_nested])
>
> +  OVS_GREP_IFELSE([$KSRC/include/linux/if_vlan.h], [vlan_set_encap_proto])

Since we know that this isn't backported, I would just leave out the
test for now and then we can put a version check in when we move this
to be non-static upstream.

> diff --git a/datapath/actions.c b/datapath/actions.c
> index ac7187b..f9746ff 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static void vlan_reorder_header(struct sk_buff *skb)
> +{

It's not really good to have a function with the same name as another
one in a different file.  I would normally expect that callers of this
function are actually using the one in vlan_core.c but they're not and
this one is subtly different.

>  static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>  {
[...]
> +       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> +       *current_tci = vhdr->h_vlan_TCI;

It would actually be better to just use vlan_eth_hdr().

> +       vlan_set_encap_proto(skb, vhdr);
>
> -       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> +       skb_pull_rcsum(skb, VLAN_HLEN);
> +       vlan_reorder_header(skb);

These functions don't do the right thing because they assume that the
current position of skb->data is different from what it actually is at
this point in OVS.  They assume that the Ethernet header has already
been pulled off but we pushed it back on, so the offsets are wrong.

> +       skb_set_network_header(skb, VLAN_HLEN);
> +       skb_set_transport_header(skb, VLAN_HLEN);

These offsets aren't right - the network header assumes that we have
no Ethernet header on at the moment and the transport header will be
the same as the network header when in reality it should be based on
our previous parsing.

> diff --git a/datapath/vlan.h b/datapath/vlan.h
> index 7e1084e..1374f43 100644
> --- a/datapath/vlan.h
> +++ b/datapath/vlan.h
> +#ifndef HAVE_VLAN_SET_ENCAP_PROTO
> +void vlan_set_encap_proto(struct sk_buff *skb, struct vlan_hdr *vhdr);
> +#endif

This doesn't really belong in this file.  Since it is a straight
backport without any dependencies on other code, it belongs in the
compat directory.