RE: [PATCH net] net: Account for all vlan headers in skb_mac_gso_segment
From: Neil Jerram <hidden>
Date: 2014-03-31 21:22:23
Hi Vlad,
skb_network_protocol() already accounts for multiple vlan headers that may be present in the skb. However, skb_mac_gso_segment() doesn't know anything about it and assumes that skb->mac_len is set correctly to skip all mac headers. That may not always be the case. If we are simply forwarding the packet (via bridge or macvtap), all vlan headers may not be accounted for.
When is it the case that skb->mac_len does not include all VLAN tags? If you can clearly describe when this is true, and when not, it would be nice to add a comment above the mac_len field in skbuff.h, to explain.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index d855794..18b8c1b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h@@ -3015,7 +3015,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb,netdev_features_t features) { return __skb_gso_segment(skb, features, true); } -__be16 skb_network_protocol(struct sk_buff *skb); +__be16 skb_network_protocol(struct sk_buff *skb, int *depth);
For me, eth_hdr_len or l2_hdr_len would be much clearer names than "depth".
quoted hunk ↗ jump to hunk
static inline bool can_checksum_protocol(netdev_features_t features, __be16 protocol)diff --git a/net/core/dev.c b/net/core/dev.c index a98f7fa..49c41e6 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -2287,7 +2287,7 @@ out: } EXPORT_SYMBOL(skb_checksum_help); -__be16 skb_network_protocol(struct sk_buff *skb) +__be16 skb_network_protocol(struct sk_buff *skb, int *depth) { __be16 type = skb->protocol; int vlan_depth = ETH_HLEN;@@ -2314,6 +2314,9 @@ __be16 skb_network_protocol(struct sk_buff *skb) vlan_depth += VLAN_HLEN; } + if (depth) + *depth = vlan_depth; + return type; }
Similarly here ...
quoted hunk ↗ jump to hunk
@@ -2327,12 +2330,13 @@ struct sk_buff *skb_mac_gso_segment(struct sk_buff*skb, { struct sk_buff *segs = ERR_PTR(-EPROTONOSUPPORT); struct packet_offload *ptype; - __be16 type = skb_network_protocol(skb); + int vlan_depth = 0; + __be16 type = skb_network_protocol(skb, &vlan_depth); if (unlikely(!type)) return ERR_PTR(-EINVAL); - __skb_pull(skb, skb->mac_len); + __skb_pull(skb, vlan_depth > skb->mac_len ? vlan_depth : skb->mac_len);
... and here (obviously). Regards, Neil