| Message ID | 20180416220038.21743-1-bjorn@mork.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,stable] tun: fix vlan packet truncation | expand |
On 2018年04月17日 06:00, Bjørn Mork wrote: > Bogus trimming in tun_net_xmit() causes truncated vlan packets. > > skb->len is correct whether or not skb_vlan_tag_present() is true. There > is no more reason to adjust the skb length on xmit in this driver than > any other driver. tun_put_user() adds 4 bytes to the total for tagged > packets because it transmits the tag inline to userspace. This is > similar to a nic transmitting the tag inline on the wire. > > Reproducing the bug by sending any tagged packet through back-to-back > connected tap interfaces: > > socat TUN,tun-type=tap,iff-up,tun-name=in TUN,tun-type=tap,iff-up,tun-name=out & > ip link add link in name in.20 type vlan id 20 > ip addr add 10.9.9.9/24 dev in.20 > ip link set in.20 up > tshark -nxxi in -f arp -c1 2>/dev/null & > tshark -nxxi out -f arp -c1 2>/dev/null & > ping -c 1 10.9.9.5 >/dev/null 2>&1 > > The output from the 'in' and 'out' interfaces are different when the > bug is present: > > Capturing on 'in' > 0000 ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14 ......v.v7...... > 0010 08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a ..........v.v7.. > 0020 0a 09 09 09 00 00 00 00 00 00 0a 09 09 05 .............. > > Capturing on 'out' > 0000 ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14 ......v.v7...... > 0010 08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a ..........v.v7.. > 0020 0a 09 09 09 00 00 00 00 00 00 .......... > > Fixes: aff3d70a07ff ("tun: allow to attach ebpf socket filter") > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > drivers/net/tun.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 28583aa0c17d..01cf8e3d8edc 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1103,13 +1103,6 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > > len = run_ebpf_filter(tun, skb, len); > > - /* Trim extra bytes since we may insert vlan proto & TCI > - * in tun_put_user(). > - */ > - len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0; > - if (len <= 0 || pskb_trim(skb, len)) > - goto drop; > - > if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) > goto drop; > Good catch, but I still think we should do the truncation in run_ebpf_filter to match the behavior socket ebpf filter. Thanks
Jason Wang <jasowang@redhat.com> writes: > Good catch, Thanks. I guess I am "lucky" as I apparently use configs no one else are using, and therefore often experience breakage ;-) I have been using tagged interfaces over tap for an occasional virtual "lab" of kvm machines for years. It's a simple way to have a flexible netowrk between host and virtual machine. So v4.16 broke my "lab". I'm sorry it took me a while before I had time to investigate why. > but I still think we should do the truncation in > run_ebpf_filter to match the behavior socket ebpf filter. Haven't looked at that code. But I'd like to point out that any length adjustments should be done next to the code adding or removing data. Doing data and length modifications in different funtions is either a bug, or a bug bound to happen. Bjørn
From: Bjørn Mork <bjorn@mork.no> Date: Tue, 17 Apr 2018 00:00:38 +0200 > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 28583aa0c17d..01cf8e3d8edc 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1103,13 +1103,6 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > > len = run_ebpf_filter(tun, skb, len); > > - /* Trim extra bytes since we may insert vlan proto & TCI > - * in tun_put_user(). > - */ > - len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0; > - if (len <= 0 || pskb_trim(skb, len)) > - goto drop; > - > if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) The VLAN business might be bogus, and needs to be removed. However, the pskb_trim() has to stay in some form. I think this is what Jason is trying to say. The semantics of running a BPF program is that the program returns the desired packet length. We must truncate the packet to the length returned by the BPF program, therefore.
David Miller <davem@davemloft.net> writes: > The VLAN business might be bogus, and needs to be removed. > > However, the pskb_trim() has to stay in some form. I think this is what > Jason is trying to say. > > The semantics of running a BPF program is that the program returns the > desired packet length. We must truncate the packet to the length > returned by the BPF program, therefore. Right. That's obvious when I look closer at it. Sorry for being so slow... New version coming up. Bjørn
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 28583aa0c17d..01cf8e3d8edc 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1103,13 +1103,6 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) len = run_ebpf_filter(tun, skb, len); - /* Trim extra bytes since we may insert vlan proto & TCI - * in tun_put_user(). - */ - len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0; - if (len <= 0 || pskb_trim(skb, len)) - goto drop; - if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) goto drop;
Bogus trimming in tun_net_xmit() causes truncated vlan packets. skb->len is correct whether or not skb_vlan_tag_present() is true. There is no more reason to adjust the skb length on xmit in this driver than any other driver. tun_put_user() adds 4 bytes to the total for tagged packets because it transmits the tag inline to userspace. This is similar to a nic transmitting the tag inline on the wire. Reproducing the bug by sending any tagged packet through back-to-back connected tap interfaces: socat TUN,tun-type=tap,iff-up,tun-name=in TUN,tun-type=tap,iff-up,tun-name=out & ip link add link in name in.20 type vlan id 20 ip addr add 10.9.9.9/24 dev in.20 ip link set in.20 up tshark -nxxi in -f arp -c1 2>/dev/null & tshark -nxxi out -f arp -c1 2>/dev/null & ping -c 1 10.9.9.5 >/dev/null 2>&1 The output from the 'in' and 'out' interfaces are different when the bug is present: Capturing on 'in' 0000 ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14 ......v.v7...... 0010 08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a ..........v.v7.. 0020 0a 09 09 09 00 00 00 00 00 00 0a 09 09 05 .............. Capturing on 'out' 0000 ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14 ......v.v7...... 0010 08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a ..........v.v7.. 0020 0a 09 09 09 00 00 00 00 00 00 .......... Fixes: aff3d70a07ff ("tun: allow to attach ebpf socket filter") Cc: Jason Wang <jasowang@redhat.com> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/tun.c | 7 ------- 1 file changed, 7 deletions(-)