tun issue after e0b46d0ee9c: tun: Use iovec iterators

DISCUSSION Applied Marcelo Ricardo Leitner 5 participants 12 messages Nov 28, 2014 – Dec 3, 2014

Subsystems (3)

Networking Network drivers TUN/TAP driver

lore

Share & tools

lore

Marcelo Ricardo Leitner

author

2014-11-28 19:25 UTC #0 lore raw

Hi,

I saw there are tun updates on Dave's queue but none seemed to handle this.

I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:

commit e0b46d0ee9c240c7430a47e9b0365674d4a04522

Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800

tun: Use iovec iterators

This patch removes the use of skb_copy_datagram_const_iovec in favour of the iovec iterator-based skb_copy_datagram_iter.

tun interface, host drops 1 incoming packets from guest, shown at ip -s l l, and keeps like that forever. netstat -s didn't mention any checksum issue

12: vnet0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast master virbr0 state UNKNOWN mode DEFAULT group default qlen 500
link/ether fe:54:00:10:3f:06 brd ff:ff:ff:ff:ff:ff
RX: bytes packets errors dropped overrun mcast
0 0 0 1 0 0
TX: bytes packets errors dropped carrier collsns
2704 51 0 0 0 0

And tap interfaces go counting dropped++ every time:

20: tap0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast master virbr0 state UNKNOWN mode DEFAULT group default qlen 500
link/ether fe:97:a4:a6:14:65 brd ff:ff:ff:ff:ff:ff
RX: bytes packets errors dropped overrun mcast
0 0 0 16 0 0
TX: bytes packets errors dropped carrier collsns
1456 28 0 0 0 0

traffic capture on guest shows incoming and outgoing packets just fine, while on host, it shows no incoming packets at all.

Tested with virtio_net and e1000.

With:

tun.c tun_get_user():

        if (zerocopy)
                err = zerocopy_sg_from_iter(skb, from);
        else {
                err = skb_copy_datagram_from_iter(skb, 0, from, len); <- fails
                if (!err && msg_control) {
                        struct ubuf_info *uarg = msg_control;
                        uarg->callback(uarg, false);
                }
        }

        if (err) {
                tun->dev->stats.rx_dropped++;
                kfree_skb(skb);
                pr_err("%d %d %d %p\n", __LINE__, zerocopy, err, msg_control);
                return -EFAULT;
        }

And net/core/datagram.c, skb_copy_datagram_from_iter():

        if (copy > 0) {
                int ret;
                if (copy > len)
                        copy = len;
                ret = copy_from_iter(skb->data + offset, copy, from);
                if (ret != copy) {
                        pr_err("%d ret=%d copy=%d offset=%d len=%d\n", 
__LINE__, ret, copy, offset, len);
                        goto fault;
                }

I get, for tun interfaces:

[   75.435552] 506 ret=80 copy=90 offset=0 len=90
[   75.435563] tun: 1124 0 -14           (null)
[   75.499528] 506 ret=80 copy=90 offset=0 len=90
[   75.499540] tun: 1124 0 -14           (null)

These were 1 drop on 1 interface each

And for tap interfaces:

[  301.982639] 506 ret=80 copy=90 offset=0 len=90
[  301.982649] tun: 1124 0 -14           (null)
[  301.988625] 506 ret=80 copy=90 offset=0 len=90
[  301.988635] tun: 1124 0 -14           (null)
[  301.994762] 506 ret=80 copy=90 offset=0 len=90
[  301.994773] tun: 1124 0 -14           (null)
[  302.229962] 506 ret=332 copy=342 offset=0 len=342
[  302.229972] tun: 1124 0 -14           (null)
[  302.230621] 506 ret=332 copy=342 offset=0 len=342
[  302.230627] tun: 1124 0 -14           (null)
[  302.239065] 506 ret=332 copy=342 offset=0 len=342
[  302.239071] tun: 1124 0 -14           (null)

It's returning 10 bytes less than the expected... ideas?

I can provide more info if needed, it's easy to reproduce in here.

Thanks,
Marcelo

Al Viro

2014-11-28 20:37 UTC #1 lore raw

Quoted 7 lines · Marcelo Ricardo Leitner

Hi,
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:

Umm... In host, presumably?

Quoted 35 lines

And net/core/datagram.c, skb_copy_datagram_from_iter():

        if (copy > 0) {
                int ret;
                if (copy > len)
                        copy = len;
                ret = copy_from_iter(skb->data + offset, copy, from);
                if (ret != copy) {
                        pr_err("%d ret=%d copy=%d offset=%d
len=%d\n", __LINE__, ret, copy, offset, len);
                        goto fault;
                }

I get, for tun interfaces:

[   75.435552] 506 ret=80 copy=90 offset=0 len=90
[   75.435563] tun: 1124 0 -14           (null)
[   75.499528] 506 ret=80 copy=90 offset=0 len=90
[   75.499540] tun: 1124 0 -14           (null)

These were 1 drop on 1 interface each

And for tap interfaces:

[  301.982639] 506 ret=80 copy=90 offset=0 len=90
[  301.982649] tun: 1124 0 -14           (null)
[  301.988625] 506 ret=80 copy=90 offset=0 len=90
[  301.988635] tun: 1124 0 -14           (null)
[  301.994762] 506 ret=80 copy=90 offset=0 len=90
[  301.994773] tun: 1124 0 -14           (null)
[  302.229962] 506 ret=332 copy=342 offset=0 len=342
[  302.229972] tun: 1124 0 -14           (null)
[  302.230621] 506 ret=332 copy=342 offset=0 len=342
[  302.230627] tun: 1124 0 -14           (null)
[  302.239065] 506 ret=332 copy=342 offset=0 len=342
[  302.239071] tun: 1124 0 -14           (null)

Quoted 1 line

It's returning 10 bytes less than the expected... ideas?

Could you print vnet_hdr_sz and sizeof(gso) right after that copy_from_iter(&gso, ...)?

Marcelo Ricardo Leitner

author

2014-11-28 22:10 UTC #2 lore raw

Quoted 11 lines · Al Viro

Quoted 7 lines · Marcelo Ricardo Leitner
Hi,
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
Umm... In host, presumably?

Exactly

Quoted 35 lines

And net/core/datagram.c, skb_copy_datagram_from_iter():

         if (copy > 0) {
                 int ret;
                 if (copy > len)
                         copy = len;
                 ret = copy_from_iter(skb->data + offset, copy, from);
                 if (ret != copy) {
                         pr_err("%d ret=%d copy=%d offset=%d
len=%d\n", __LINE__, ret, copy, offset, len);
                         goto fault;
                 }

I get, for tun interfaces:

[   75.435552] 506 ret=80 copy=90 offset=0 len=90
[   75.435563] tun: 1124 0 -14           (null)
[   75.499528] 506 ret=80 copy=90 offset=0 len=90
[   75.499540] tun: 1124 0 -14           (null)

These were 1 drop on 1 interface each

And for tap interfaces:

[  301.982639] 506 ret=80 copy=90 offset=0 len=90
[  301.982649] tun: 1124 0 -14           (null)
[  301.988625] 506 ret=80 copy=90 offset=0 len=90
[  301.988635] tun: 1124 0 -14           (null)
[  301.994762] 506 ret=80 copy=90 offset=0 len=90
[  301.994773] tun: 1124 0 -14           (null)
[  302.229962] 506 ret=332 copy=342 offset=0 len=342
[  302.229972] tun: 1124 0 -14           (null)
[  302.230621] 506 ret=332 copy=342 offset=0 len=342
[  302.230627] tun: 1124 0 -14           (null)
[  302.239065] 506 ret=332 copy=342 offset=0 len=342
[  302.239071] tun: 1124 0 -14           (null)

Quoted 4 lines

Quoted 1 line
It's returning 10 bytes less than the expected... ideas?
Could you print vnet_hdr_sz and sizeof(gso) right after that copy_from_iter(&gso, ...)?

Did a:

        else {
                err = skb_copy_datagram_from_iter(skb, 0, from, len);
+               pr_err("vnet_hdr_sz=%d sizeof(gso)=%lu\n", tun->vnet_hdr_sz, sizeof(gso));
                if (!err && msg_control) {

Got, for tun:

[   50.514165] tun: vnet_hdr_sz=12 sizeof(gso)=10

for tap:

[   82.911840] tun: vnet_hdr_sz=10 sizeof(gso)=10

other values were just as before.

Marcelo

Al Viro

2014-11-28 22:35 UTC #3 lore raw

Quoted 10 lines · Marcelo Ricardo Leitner

Quoted 2 lines
Could you print vnet_hdr_sz and sizeof(gso) right after that copy_from_iter(&gso, ...)?
Did a:
        else {
                err = skb_copy_datagram_from_iter(skb, 0, from, len);
+               pr_err("vnet_hdr_sz=%d sizeof(gso)=%lu\n",
tun->vnet_hdr_sz, sizeof(gso));
                if (!err && msg_control) {

Quoted 7 lines

Got, for tun:
[   50.514165] tun: vnet_hdr_sz=12 sizeof(gso)=10
for tap:
[   82.911840] tun: vnet_hdr_sz=10 sizeof(gso)=10
other values were just as before.

Hmm... Do you have
commit 8c847d254146d32c86574a1b16923ff91bb784dd

Author: Jason Wang <[email protected]>
Date:   Thu Nov 13 16:54:14 2014 +0800

tun: fix issues of iovec iterators using in tun_put_user()
in your tree?

Marcelo Ricardo Leitner

author

2014-11-29 04:50 UTC #4 lore raw

Quoted 19 lines · Al Viro

Quoted 10 lines · Marcelo Ricardo Leitner
Quoted 2 lines
Could you print vnet_hdr_sz and sizeof(gso) right after that copy_from_iter(&gso, ...)?
Did a:
         else {
                 err = skb_copy_datagram_from_iter(skb, 0, from, len);
+               pr_err("vnet_hdr_sz=%d sizeof(gso)=%lu\n",
tun->vnet_hdr_sz, sizeof(gso));
                 if (!err && msg_control) {
Quoted 7 lines
Got, for tun:
[   50.514165] tun: vnet_hdr_sz=12 sizeof(gso)=10
for tap:
[   82.911840] tun: vnet_hdr_sz=10 sizeof(gso)=10
other values were just as before.

Quoted 8 lines

Hmm... Do you have
commit 8c847d254146d32c86574a1b16923ff91bb784dd
Author: Jason Wang <[email protected]>
Date:   Thu Nov 13 16:54:14 2014 +0800
tun: fix issues of iovec iterators using in tun_put_user()
in your tree?

Yes I have. I'm using based on net-next's 799d2fff1858004526ad75d66a5dd8a5cce6ad40.

Marcelo

Herbert Xu

2014-11-29 00:00 UTC #5 lore raw

In reply to Marcelo Ricardo Leitner

Quoted 10 lines · Marcelo Ricardo Leitner

I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800

Does this patch help?

-- >8 --
Subject: tun: Fix GSO meta-data handling in tun_get_user

When we write the GSO meta-data in tun_get_user we end up advancing the IO vector twice, thus exhausting the user buffer before we can finish writing the packet.

Fixes f5ff53b4d97c ("{macvtap,tun}_get_user(): switch to iov_iter")

Reported-by Marcelo Ricardo Leitner <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

drivers/net/tun.c +1 -1

index 4b743c6..9357871 100644
@@ -1052,7 +1052,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 		if (gso.hdr_len > len)
 			return -EINVAL;
-		iov_iter_advance(from, tun->vnet_hdr_sz);
+		iov_iter_advance(iter, tun->vnet_hdr_sz - sizeof(gso));
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV) {

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Marcelo Ricardo Leitner

author

2014-11-29 05:08 UTC #6 lore raw

Quoted 8 lines · Herbert Xu

Quoted 7 lines · Marcelo Ricardo Leitner
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:

Quoted 5 lines

Quoted 3 lines
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800
Does this patch help?

Yay, it does! Works for me, thanks Herbert.
I didn't test performance, but dhcp could get through.

Are you sure about the Fixes tag? Because bisect really pointed to e0b46d0ee9c.

Cheers,
Marcelo

Quoted 27 lines

-- >8 --
Subject: tun: Fix GSO meta-data handling in tun_get_user
When we write the GSO meta-data in tun_get_user we end up advancing the IO vector twice, thus exhausting the user buffer before we can finish writing the packet.
Fixes f5ff53b4d97c ("{macvtap,tun}_get_user(): switch to iov_iter")
Reported-by Marcelo Ricardo Leitner <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
drivers/net/tun.c +1 -1
index 4b743c6..9357871 100644
@@ -1052,7 +1052,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
  		if (gso.hdr_len > len)
  			return -EINVAL;
-		iov_iter_advance(from, tun->vnet_hdr_sz);
+		iov_iter_advance(iter, tun->vnet_hdr_sz - sizeof(gso));
  	}
 
  	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV) {
Cheers,

Herbert Xu

2014-11-30 08:21 UTC #7 lore raw

Quoted 13 lines · Marcelo Ricardo Leitner

Quoted 12 lines · Herbert Xu
Quoted 10 lines · Marcelo Ricardo Leitner
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800

Quoted 6 lines

Quoted 1 line
Does this patch help?
Yay, it does! Works for me, thanks Herbert.
I didn't test performance, but dhcp could get through.
Are you sure about the Fixes tag? Because bisect really pointed to e0b46d0ee9c.

Well according to your report you were having problems with tun_get_user. The bug I introduced was in tun_put_user and has already been fixed by Jason Wang.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Marcelo Ricardo Leitner

author

2014-12-01 13:06 UTC #8 lore raw

Quoted 16 lines · Herbert Xu

Quoted 15 lines · Marcelo Ricardo Leitner
Quoted 13 lines · Herbert Xu
Quoted 10 lines · Marcelo Ricardo Leitner
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800
Does this patch help?

Quoted 8 lines

Quoted 4 lines
Yay, it does! Works for me, thanks Herbert.
I didn't test performance, but dhcp could get through.
Are you sure about the Fixes tag? Because bisect really pointed to e0b46d0ee9c.
Well according to your report you were having problems with tun_get_user. The bug I introduced was in tun_put_user and has already been fixed by Jason Wang.

Ahh yes, ok. Thanks

Cheers,
Marcelo

Herbert Xu

2014-11-30 10:03 UTC #9 lore raw

In reply to Herbert Xu

Quoted 8 lines · Herbert Xu

Quoted 7 lines · Marcelo Ricardo Leitner
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:

Quoted 5 lines

Quoted 3 lines
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800
Does this patch help?

Oops, there was an embarrassing typo in the patch which causes it to not even build. Here is the corrected version.

-- >8 --
Subject: tun: Fix GSO meta-data handling in tun_get_user

When we write the GSO meta-data in tun_get_user we end up advancing the IO vector twice, thus exhausting the user buffer before we can finish writing the packet.

Fixes f5ff53b4d97c ("{macvtap,tun}_get_user(): switch to iov_iter")

Reported-by Marcelo Ricardo Leitner <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

drivers/net/tun.c +1 -1

index 4b743c6..6d44da1 100644
@@ -1052,7 +1052,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 		if (gso.hdr_len > len)
 			return -EINVAL;
-		iov_iter_advance(from, tun->vnet_hdr_sz);
+		iov_iter_advance(from, tun->vnet_hdr_sz - sizeof(gso));
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV) {

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Jason Wang

Acked-by

2014-12-01 05:33 UTC #10 lore raw

Quoted 44 lines · Herbert Xu

Quoted 10 lines · Herbert Xu
Quoted 2 lines · Marcelo Ricardo Leitner
I saw there are tun updates on Dave's queue but none seemed to
handle this.
Quoted 5 lines
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
Quoted 5 lines
Quoted 3 lines
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800
Does this patch help?
Oops, there was an embarrassing typo in the patch which causes it to not even build. Here is the corrected version.
-- >8 --
Subject: tun: Fix GSO meta-data handling in tun_get_user
When we write the GSO meta-data in tun_get_user we end up advancing the IO vector twice, thus exhausting the user buffer before we can finish writing the packet.
Fixes f5ff53b4d97c ("{macvtap,tun}_get_user(): switch to iov_iter")
Reported-by Marcelo Ricardo Leitner <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
drivers/net/tun.c +1 -1
index 4b743c6..6d44da1 100644
@@ -1052,7 +1052,7 @@ static ssize_t tun_get_user(struct tun_struct 
 *tun, struct tun_file *tfile,
 
 		if (gso.hdr_len > len)
 			return -EINVAL;
-		iov_iter_advance(from, tun->vnet_hdr_sz);
+		iov_iter_advance(from, tun->vnet_hdr_sz - sizeof(gso));
 	}
 

Quoted 2 lines

if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV) {

Acked-by Jason Wang <[email protected]>

David Miller

maintainer Applied

2014-12-03 04:50 UTC #11 lore raw

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Sun, 30 Nov 2014 18:03:31 +0800

Quoted 21 lines · Herbert Xu

Quoted 8 lines · Marcelo Ricardo Leitner
Quoted 7 lines
I saw there are tun updates on Dave's queue but none seemed to handle this.
I can't use current net-next (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor because tun got clogged somehow. Bisected down to:
Quoted 5 lines
Quoted 3 lines
commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
Author: Herbert Xu <[email protected]>
Date:   Fri Nov 7 21:22:23 2014 +0800
Does this patch help?
Oops, there was an embarrassing typo in the patch which causes it to not even build. Here is the corrected version.
-- >8 --
Subject: tun: Fix GSO meta-data handling in tun_get_user

Quoted 7 lines

When we write the GSO meta-data in tun_get_user we end up advancing the IO vector twice, thus exhausting the user buffer before we can finish writing the packet.
Fixes f5ff53b4d97c ("{macvtap,tun}_get_user(): switch to iov_iter")
Reported-by Marcelo Ricardo Leitner <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied, thanks.