[ovs-dev] [PATCH] VLAN actions should use push/pop semantics

Jesse Gross jesse at nicira.com
Fri Sep 2 20:33:55 UTC 2011


On Tue, Aug 23, 2011 at 10:48 AM, Pravin Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 8aec438..0150be2 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -61,6 +53,9 @@ static int strip_vlan(struct sk_buff *skb)
>                skb->csum = csum_sub(skb->csum, csum_partial(skb->data
>                                        + ETH_HLEN, VLAN_HLEN, 0));
>
> +       veth = (struct vlan_ethhdr *) skb->data;
> +       *current_tci = veth->h_vlan_TCI;
> +
>        memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>
>        eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);

Might as well use __skb_pull() here since everything else in this
function is assuming that the skb is long enough.

> +static int push_vlan(struct sk_buff *skb, __be16 new_tci)
> +{
> +       if (unlikely(vlan_tx_tag_present(skb))) {
> +               u16 current_tag;
>
> +               current_tag = vlan_tx_tag_get(skb);
> +
> +               if (!__vlan_put_tag(skb, current_tag))
> +                       return -ENOMEM;

You need to update skb->csum in the CHECKSUM_COMPLETE case, similar to
what we do when popping a tag that is actually present in the skb.

> diff --git a/datapath/linux/compat/include/linux/if_ether.h b/datapath/linux/compat/include/linux/if_ether.h
> index b8390e2..75ab82c 100644
> --- a/datapath/linux/compat/include/linux/if_ether.h
> +++ b/datapath/linux/compat/include/linux/if_ether.h
> @@ -7,6 +7,7 @@
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
>
>  #define ETH_P_TEB      0x6558          /* Trans Ether Bridging         */
> +#define ETH_P_FCOE     0x8906          /* Fibre Channel over Ethernet  */

I think this wasn't actually added until 2.6.30.

> diff --git a/datapath/linux/compat/netdevice.c b/datapath/linux/compat/netdevice.c
> index 1a6f327..a06da0c 100644
> --- a/datapath/linux/compat/netdevice.c
> +++ b/datapath/linux/compat/netdevice.c
> @@ -1,5 +1,7 @@
>  #include <linux/if_link.h>
>  #include <linux/netdevice.h>
> +#include <linux/if_vlan.h>
> +

Extra blank line?

> +u32 rpl_netif_skb_features(struct sk_buff *skb)
> +{
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26)
> +       __be16 protocol = skb->protocol;
> +       u32 features = skb->dev->features;
> +
> +       if (protocol == htons(ETH_P_8021Q)) {
> +               struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
> +               protocol = veh->h_vlan_encapsulated_proto;
> +       } else if (!vlan_tx_tag_present(skb)) {
> +               return harmonize_features(skb, protocol, features);
> +       }
> +
> +       features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_TX);
> +
> +       if (protocol != htons(ETH_P_8021Q)) {
> +               return harmonize_features(skb, protocol, features);
> +       } else {
> +               features &= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
> +                       NETIF_F_GEN_CSUM | NETIF_F_HW_VLAN_TX;
> +               return harmonize_features(skb, protocol, features);
> +       }
> +#else
> +       return 0;

The reason why this was protected by the version guard before is it
used vlan_features, which didn't exist before 2.6.26.  This function
is a little more general, so it can be a little less restrictive in
the non-vlan case, even if it isn't used otherwise.

> +struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb, u32 features)
> +{
> +       int vlan_depth = ETH_HLEN;
> +       __be16 type = skb->protocol;
> +
> +       while (type == htons(ETH_P_8021Q)) {
> +               struct vlan_hdr *vh;
> +
> +               if (unlikely(!pskb_may_pull(skb, vlan_depth + VLAN_HLEN)))
> +                       return ERR_PTR(-EINVAL);
> +
> +               vh = (struct vlan_hdr *)(skb->data + vlan_depth);
> +               type = vh->h_vlan_encapsulated_proto;
> +               vlan_depth += VLAN_HLEN;
> +       }
> +
> +       /* this hack needed to get regular skb_gso_segment() */
> +#undef skb_gso_segment
> +       skb->protocol = type;
> +
> +       return skb_gso_segment(skb, features);

I think you should reset the original skb->protocol before returning.

> +}
> +
>  #endif /* kernel version < 2.6.36 */

Why is this in a block for kernels before 2.6.36?  I think it should be 2.6.38.

Ethan, you mentioned before that you saw a bug somewhere in here.  Can
you review the userspace portions?



More information about the dev mailing list