vlan: Fix duplicate delivery of vlan 0 packets to ETH_P_ALL packet sockets
Subsystems (2)
PATCH 1/1 vlan: Fix duplicate delivery of vlan 0 packets to ETH_P_ALL packet sockets
For vlan data coming in from nics without vlan hardware accelleration we get two copies of vlan packets with vlan id 0 on pf_packet sockets, causing userspace to break. This is caused by delivering the same packet to the same networking device more than once.
The cleanest and simplist fix I can find is to make the vlan accellerated path
the common path for vlan tagged packets. Moving the classification of vlan
tagged packets sooner and removes the need for vlan 0 packets to be double delivered. This has a small impact on the fast path.
The penalty on the fast path for the non-vlan tagged case should be a single forward branch that is always predicted as not taken. With some clean up to the routines for vlan accelleration and non-vlan accelleration we should be able to even remove that branch if it is a problem.
Once the classification is moved sooner the traditional vlan reception path can be removed entirely. Reducing duplicate maintenance for the vlan code.
Receiving non vlan accellerated packets into vlan_accel_do_receive only requires adding the special case for hiding the vlan header from pf_packet sockets on vlan headers.
While moving the code to hide the packet sockets I have fixed two bugs. I moved the rx_error case inside the proper stats collection bracket, and I have reduced the received packet size by the number of bytes we are hiding.
--- include/linux/if_vlan.h | 5 ++ net/8021q/vlan.c | 8 -- net/8021q/vlan.h | 2 - net/8021q/vlan_core.c | 49 +++++++++++++- net/8021q/vlan_dev.c | 173 ----------------------------------------------- net/core/dev.c | 9 +++ 6 files changed, 62 insertions(+), 184 deletions(-)
include/linux/if_vlan.h +5 -0
@@ -133,6 +133,7 @@ extern u16 vlan_dev_vlan_id(const struct net_device *dev);extern int __vlan_hwaccel_rx (struct sk_buff *skb,struct vlan_group *grp,u16 vlan_tci,int polling);extern bool vlan_hwaccel_do_receive (struct sk_buff **skb); +extern void emulate_vlan_hwaccel (struct sk_buff *skb);extern gro_result_t vlan_gro_receive (struct napi_struct *napi,struct vlan_group *grp,unsigned int vlan_tci,struct sk_buff *skb);
@@ -173,6 +174,10 @@ static inline bool vlan_hwaccel_do_receive(struct sk_buff **skb)return false; } +static void emulate_vlan_hwaccel (struct sk_buff *skb) +{ +} +static inline gro_result_t vlan_gro_receive (struct napi_struct *napi,struct vlan_group *grp,unsigned int vlan_tci,struct sk_buff *skb)
net/8021q/vlan.c +0 -8
@@ -49,11 +49,6 @@ const char vlan_version[] = DRV_VERSION;static const char vlan_copyright[] ="Ben Greear <[email protected]>" ;static const char vlan_buggyright[] ="David S. Miller <[email protected]>" ; -static struct packet_type vlan_packet_type__read_mostly = { - .type =cpu_to_be16 (ETH_P_8021Q ), - .func = vlan_skb_recv,/* VLAN receive method */ -}; -/* End of global variables definitions. */ static void vlan_group_free (struct vlan_group *grp)
@@ -688,7 +683,6 @@ static int __init vlan_proto_init(void)if (err <0 ) goto err4; -dev_add_pack (&vlan_packet_type);vlan_ioctl_set (vlan_ioctl_handler);return 0 ;
@@ -709,8 +703,6 @@ static void __exit vlan_cleanup_module(void)unregister_netdevice_notifier (&vlan_notifier_block); -dev_remove_pack (&vlan_packet_type); -unregister_pernet_subsys (&vlan_net_ops);rcu_barrier ();/* Wait for completion of call_rcu()'s */
net/8021q/vlan.h +0 -2
@@ -75,8 +75,6 @@ static inline struct vlan_dev_info *vlan_dev_info(const struct net_device *dev) }/* found in vlan_dev.c */ -int vlan_skb_recv (struct sk_buff *skb,struct net_device *dev, -struct packet_type *ptype,struct net_device *orig_dev);void vlan_dev_set_ingress_priority (const struct net_device *dev,u32 skb_prio,u16 vlan_prio); int vlan_dev_set_egress_priority(const struct net_device *dev,
net/8021q/vlan_core.c +48 -1
63 lines not on this page · raw message
net/8021q/vlan_dev.c +0 -173
179 lines not on this page · raw message
net/core/dev.c +9 -0
@@ -3152,6 +3152,15 @@ static int __netif_receive_skb(struct sk_buff *skb) skb->skb_iif = skb->dev->ifindex; orig_dev = skb->dev; +/* If a vlan header is present pretend to be hardware accelerated +* to remove special cases and to prevent duplicate delivery to +* ETH_P_ALL sockets in the case vlan id == 0, where the vlan +* header is only used to set the packets priority. +*/ +if (unlikely (skb->protocol ==htons (ETH_P_8021Q ) && + !vlan_tx_tag_present (skb))) +emulate_vlan_hwaccel (skb); +skb_reset_network_header (skb);skb_reset_transport_header (skb); skb->mac_len = skb->network_header - skb->mac_header;
Signature · 1 line
1.7.4
Quoted 5 lines · Eric W. Biederman
For vlan data coming in from nics without vlan hardware accelleration we get two copies of vlan packets with vlan id 0 on
pf_packetsockets, causing userspace to break. This is caused by delivering the same packet to the same networking device more than once.
I agree that this is a problem and the code consolidation is very nice but I'm concerned that there is extra complexity for the rest of the system to counterbalance what is saved here.
Quoted 4 lines
net/8021q/vlan_core.c +0
-0
net/8021q/vlan_core.c line 16
Quoted 10 lines
net/8021q/vlan_core.c +10
-0+void emulate_vlan_hwaccel (struct sk_buff *skb) +{ + struct vlan_hdr *vhdr = (struct vlan_hdr *)skb->data; + __be16 proto; + + if (!pskb_may_pull(skb, VLAN_HLEN)) + return; + + __vlan_hwaccel_put_tag(skb, vhdr->h_vlan_TCI); + skb_pull_rcsum(skb, VLAN_HLEN);
Doesn't this break things which push the header back on? Bridging pushes ETH_HLEN before forwarding but here it will be a garbage value due to the extra vlan header. AF_PACKET pushes the mac header back on, which in this case includes the original vlan header. However, since we've also put the tag in skb->vlan_tci, won't it appear to be double tagged?
More generally, even though we pull the tag off the skb it's pretty common on the receive path to look backwards into previous headers. Given that this can happen, I think it's somewhat confusing/fragile to have packet data which effectively should not be there. It also adds a third case to any generic vlan handling code: tag in packet (can still happen, such as on transmit), received on vlan accelerated NIC - tag in skb but not in packet, receive on non-vlan accelerated NIC - tag in both skb and packet.
If we actually removed the tag in the emulated case that would avoid these concerns but would, of course, add extra overhead in some situations.
Quoted 25 lines · Jesse Gross
Quoted 5 lines · Eric W. Biederman
For vlan data coming in from nics without vlan hardware accelleration we get two copies of vlan packets with vlan id 0 on
pf_packetsockets, causing userspace to break. This is caused by delivering the same packet to the same networking device more than once.I agree that this is a problem and the code consolidation is very nice but I'm concerned that there is extra complexity for the rest of the system to counterbalance what is saved here.
Quoted 13 lines
net/8021q/vlan_core.c +9
-0+void emulate_vlan_hwaccel (struct sk_buff *skb) +{ + struct vlan_hdr *vhdr = (struct vlan_hdr *)skb->data; + __be16 proto; + + if (!pskb_may_pull(skb, VLAN_HLEN)) + return; + + __vlan_hwaccel_put_tag(skb, vhdr->h_vlan_TCI);
Quoted 8 lines
Quoted 1 line
+
skb_pull_rcsum(skb, VLAN_HLEN);Doesn't this break things which push the header back on? Bridging pushes
ETH_HLENbefore forwarding but here it will be a garbage value due to the extra vlan header.AF_PACKETpushes the mac header back on, which in this case includes the original vlan header. However, since we've also put the tag inskb->vlan_tci, won't it appear to be double tagged?
Probably that part does indeed look like a bug, and my testing certainly shows that there are problems with my patch.
Quoted 9 lines
More generally, even though we pull the tag off the skb it's pretty common on the receive path to look backwards into previous headers. Given that this can happen, I think it's somewhat confusing/fragile to have packet data which effectively should not be there. It also adds a third case to any generic vlan handling code: tag in packet (can still happen, such as on transmit), received on vlan accelerated NIC - tag in skb but not in packet, receive on non-vlan accelerated NIC - tag in both skb and packet.
Quoted 3 lines
If we actually removed the tag in the emulated case that would avoid these concerns but would, of course, add extra overhead in some situations.
The only extra overhead I can really see is the need to put the vlan
tag back on in a few instances. Moving the ethernet addresses around
in the packet (the cost of adding/removing the vlan header) since they are in a hot cacheline doesn't concern me very much.
But we definitely need to do something to fix the regression for pf_packet sockets.
Eric
Quoted 48 lines · Eric W. Biederman
Quoted 25 lines · Jesse Gross
Quoted 5 lines · Eric W. Biederman
For vlan data coming in from nics without vlan hardware accelleration we get two copies of vlan packets with vlan id 0 on
pf_packetsockets, causing userspace to break. This is caused by delivering the same packet to the same networking device more than once.I agree that this is a problem and the code consolidation is very nice but I'm concerned that there is extra complexity for the rest of the system to counterbalance what is saved here.
Quoted 13 lines
net/8021q/vlan_core.c +9
-0+void emulate_vlan_hwaccel (struct sk_buff *skb) +{ + struct vlan_hdr *vhdr = (struct vlan_hdr *)skb->data; + __be16 proto; + + if (!pskb_may_pull(skb, VLAN_HLEN)) + return; + + __vlan_hwaccel_put_tag(skb, vhdr->h_vlan_TCI);Quoted 8 lines
Quoted 1 line
+
skb_pull_rcsum(skb, VLAN_HLEN);Doesn't this break things which push the header back on? Bridging pushes
ETH_HLENbefore forwarding but here it will be a garbage value due to the extra vlan header. AF_PACKET pushes the mac header back on, which in this case includes the original vlan header. However, since we've also put the tag inskb->vlan_tci, won't it appear to be double tagged?Probably that part does indeed look like a bug, and my testing certainly shows that there are problems with my patch.
Quoted 9 lines
More generally, even though we pull the tag off the skb it's pretty common on the receive path to look backwards into previous headers. Given that this can happen, I think it's somewhat confusing/fragile to have packet data which effectively should not be there. It also adds a third case to any generic vlan handling code: tag in packet (can still happen, such as on transmit), received on vlan accelerated NIC - tag in skb but not in packet, receive on non-vlan accelerated NIC - tag in both skb and packet.
Quoted 8 lines
Quoted 3 lines
If we actually removed the tag in the emulated case that would avoid these concerns but would, of course, add extra overhead in some situations.
The only extra overhead I can really see is the need to put the vlan tag back on in a few instances. Moving the ethernet addresses around in the packet (the cost of adding/removing the vlan header) since they are in a hot cacheline doesn't concern me very much.
Yes, you're probably right. It likely rare that we hit this path at all, unlikely that we'll need to put it back, and, as you say, it's in a hot cacheline. In that case, it seems like a reasonable approach to me.
Quoted 3 lines
But we definitely need to do something to fix the regression for
pf_packetsockets.
I agree.