[PATCH net 1/2] tun: unbreak truncated packet signalling

[PATCH net 1/2] tun: unbreak truncated packet signalling

[Jason Wang]

Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
(tuntap: hardware vlan tx support) breaks the truncated packet signal by never
return a length greater than iov length in tun_put_user(). This patch fixes this
by always return the length of packet plus possible vlan header. Caller can
detect the truncated packet by comparing the return value and the size of iov
length.

Reported-by: Vlad Yasevich <vyasevich@gmail.com>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch is needed for stable.
---
 drivers/net/tun.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e26cbea..dd1bd7a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 			    const struct iovec *iv, int len)
 {
 	struct tun_pi pi = { 0, skb->protocol };
-	ssize_t total = 0;
+	struct {
+		__be16 h_vlan_proto;
+		__be16 h_vlan_TCI;
+	} veth;
+	ssize_t total = 0, off = 0;
 	int vlan_offset = 0;
 
 	if (!(tun->flags & TUN_NO_PI)) {
@@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 		total += tun->vnet_hdr_sz;
 	}
 
+	off = total;
 	if (!vlan_tx_tag_present(skb)) {
 		len = min_t(int, skb->len, len);
 	} else {
 		int copy, ret;
-		struct {
-			__be16 h_vlan_proto;
-			__be16 h_vlan_TCI;
-		} veth;
 
 		veth.h_vlan_proto = skb->vlan_proto;
 		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
@@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 		len = min_t(int, skb->len + VLAN_HLEN, len);
 
 		copy = min_t(int, vlan_offset, len);
-		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
+		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
 		len -= copy;
-		total += copy;
+		off += copy;
 		if (ret || !len)
 			goto done;
 
 		copy = min_t(int, sizeof(veth), len);
-		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
+		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
 		len -= copy;
-		total += copy;
+		off += copy;
 		if (ret || !len)
 			goto done;
 	}
 
-	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
-	total += len;
+	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
+	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
 
 done:
 	tun->dev->stats.tx_packets++;
-- 
1.8.3.2

[PATCH net 2/2] macvtap: signal truncated packets

[Jason Wang]

macvtap_put_user() never return a value grater than iov length, this in fact
bypasses the truncated checking in macvtap_recvmsg(). Fix this by always
returning the size of packet plus the possible vlan header to let the truncated
checking work.

Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch is needed for stable.
---
 drivers/net/macvtap.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 957cc5c..7544a0c 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -767,10 +767,14 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
 				const struct sk_buff *skb,
 				const struct iovec *iv, int len)
 {
-	int ret;
+	int ret, off;
 	int vnet_hdr_len = 0;
 	int vlan_offset = 0;
 	int copied;
+	struct {
+		__be16 h_vlan_proto;
+		__be16 h_vlan_TCI;
+	} veth;
 
 	if (q->flags & IFF_VNET_HDR) {
 		struct virtio_net_hdr vnet_hdr;
@@ -785,16 +789,13 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
 		if (memcpy_toiovecend(iv, (void *)&vnet_hdr, 0, sizeof(vnet_hdr)))
 			return -EFAULT;
 	}
-	copied = vnet_hdr_len;
+	off = copied = vnet_hdr_len;
 
 	if (!vlan_tx_tag_present(skb))
 		len = min_t(int, skb->len, len);
 	else {
 		int copy;
-		struct {
-			__be16 h_vlan_proto;
-			__be16 h_vlan_TCI;
-		} veth;
+
 		veth.h_vlan_proto = skb->vlan_proto;
 		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
 
@@ -802,22 +803,22 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
 		len = min_t(int, skb->len + VLAN_HLEN, len);
 
 		copy = min_t(int, vlan_offset, len);
-		ret = skb_copy_datagram_const_iovec(skb, 0, iv, copied, copy);
+		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
 		len -= copy;
-		copied += copy;
+		off += copy;
 		if (ret || !len)
 			goto done;
 
 		copy = min_t(int, sizeof(veth), len);
-		ret = memcpy_toiovecend(iv, (void *)&veth, copied, copy);
+		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
 		len -= copy;
-		copied += copy;
+		off += copy;
 		if (ret || !len)
 			goto done;
 	}
 
-	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, copied, len);
-	copied += len;
+	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
+	copied += skb->len + (vlan_offset ? sizeof(veth) : 0);
 
 done:
 	return ret ? ret : copied;
@@ -875,7 +876,7 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
 	}
 
 	ret = macvtap_do_read(q, iocb, iv, len, file->f_flags & O_NONBLOCK);
-	ret = min_t(ssize_t, ret, len); /* XXX copied from tun.c. Why? */
+	ret = min_t(ssize_t, ret, len);
 	if (ret > 0)
 		iocb->ki_pos = ret;
 out:
-- 
1.8.3.2

Re: [PATCH net 2/2] macvtap: signal truncated packets

[Michael S. Tsirkin]

On Mon, Dec 09, 2013 at 06:25:17PM +0800, Jason Wang wrote:
> macvtap_put_user() never return a value grater than iov length, this in fact
> bypasses the truncated checking in macvtap_recvmsg(). Fix this by always
> returning the size of packet plus the possible vlan header to let the truncated
> checking work.
> 
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Same comments as for tun really, but here it's also
kind of ugly to call a variable copied if we don't copy.

Also, maybe we should name the variable "copied" for tun,
this would make the code more similar.

> ---
> The patch is needed for stable.
> ---
>  drivers/net/macvtap.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 957cc5c..7544a0c 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -767,10 +767,14 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>  				const struct sk_buff *skb,
>  				const struct iovec *iv, int len)
>  {
> -	int ret;
> +	int ret, off;
>  	int vnet_hdr_len = 0;
>  	int vlan_offset = 0;
>  	int copied;
> +	struct {
> +		__be16 h_vlan_proto;
> +		__be16 h_vlan_TCI;
> +	} veth;
>  
>  	if (q->flags & IFF_VNET_HDR) {
>  		struct virtio_net_hdr vnet_hdr;
> @@ -785,16 +789,13 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>  		if (memcpy_toiovecend(iv, (void *)&vnet_hdr, 0, sizeof(vnet_hdr)))
>  			return -EFAULT;
>  	}
> -	copied = vnet_hdr_len;
> +	off = copied = vnet_hdr_len;
>  
>  	if (!vlan_tx_tag_present(skb))
>  		len = min_t(int, skb->len, len);
>  	else {
>  		int copy;
> -		struct {
> -			__be16 h_vlan_proto;
> -			__be16 h_vlan_TCI;
> -		} veth;
> +
>  		veth.h_vlan_proto = skb->vlan_proto;
>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>  
> @@ -802,22 +803,22 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>  
>  		copy = min_t(int, vlan_offset, len);
> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, copied, copy);
> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>  		len -= copy;
> -		copied += copy;
> +		off += copy;
>  		if (ret || !len)
>  			goto done;
>  
>  		copy = min_t(int, sizeof(veth), len);
> -		ret = memcpy_toiovecend(iv, (void *)&veth, copied, copy);
> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>  		len -= copy;
> -		copied += copy;
> +		off += copy;
>  		if (ret || !len)
>  			goto done;
>  	}
>  
> -	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, copied, len);
> -	copied += len;
> +	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
> +	copied += skb->len + (vlan_offset ? sizeof(veth) : 0);
>  
>  done:
>  	return ret ? ret : copied;
> @@ -875,7 +876,7 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
>  	}
>  
>  	ret = macvtap_do_read(q, iocb, iv, len, file->f_flags & O_NONBLOCK);
> -	ret = min_t(ssize_t, ret, len); /* XXX copied from tun.c. Why? */
> +	ret = min_t(ssize_t, ret, len);
>  	if (ret > 0)
>  		iocb->ki_pos = ret;
>  out:



> -- 
> 1.8.3.2

Re: [PATCH net 2/2] macvtap: signal truncated packets

[Jason Wang]

On 12/09/2013 07:02 PM, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2013 at 06:25:17PM +0800, Jason Wang wrote:
>> macvtap_put_user() never return a value grater than iov length, this in fact
>> bypasses the truncated checking in macvtap_recvmsg(). Fix this by always
>> returning the size of packet plus the possible vlan header to let the truncated
>> checking work.
>>
>> Cc: Vlad Yasevich <vyasevich@gmail.com>
>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Same comments as for tun really, but here it's also
> kind of ugly to call a variable copied if we don't copy.
>
> Also, maybe we should name the variable "copied" for tun,
> this would make the code more similar.

Agree, but better with a separate patch for net-next.
>> ---
>> The patch is needed for stable.
>> ---
>>  drivers/net/macvtap.c | 27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index 957cc5c..7544a0c 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -767,10 +767,14 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>>  				const struct sk_buff *skb,
>>  				const struct iovec *iv, int len)
>>  {
>> -	int ret;
>> +	int ret, off;
>>  	int vnet_hdr_len = 0;
>>  	int vlan_offset = 0;
>>  	int copied;
>> +	struct {
>> +		__be16 h_vlan_proto;
>> +		__be16 h_vlan_TCI;
>> +	} veth;
>>  
>>  	if (q->flags & IFF_VNET_HDR) {
>>  		struct virtio_net_hdr vnet_hdr;
>> @@ -785,16 +789,13 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>>  		if (memcpy_toiovecend(iv, (void *)&vnet_hdr, 0, sizeof(vnet_hdr)))
>>  			return -EFAULT;
>>  	}
>> -	copied = vnet_hdr_len;
>> +	off = copied = vnet_hdr_len;
>>  
>>  	if (!vlan_tx_tag_present(skb))
>>  		len = min_t(int, skb->len, len);
>>  	else {
>>  		int copy;
>> -		struct {
>> -			__be16 h_vlan_proto;
>> -			__be16 h_vlan_TCI;
>> -		} veth;
>> +
>>  		veth.h_vlan_proto = skb->vlan_proto;
>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>>  
>> @@ -802,22 +803,22 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>>  
>>  		copy = min_t(int, vlan_offset, len);
>> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, copied, copy);
>> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>>  		len -= copy;
>> -		copied += copy;
>> +		off += copy;
>>  		if (ret || !len)
>>  			goto done;
>>  
>>  		copy = min_t(int, sizeof(veth), len);
>> -		ret = memcpy_toiovecend(iv, (void *)&veth, copied, copy);
>> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>>  		len -= copy;
>> -		copied += copy;
>> +		off += copy;
>>  		if (ret || !len)
>>  			goto done;
>>  	}
>>  
>> -	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, copied, len);
>> -	copied += len;
>> +	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
>> +	copied += skb->len + (vlan_offset ? sizeof(veth) : 0);
>>  
>>  done:
>>  	return ret ? ret : copied;
>> @@ -875,7 +876,7 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
>>  	}
>>  
>>  	ret = macvtap_do_read(q, iocb, iv, len, file->f_flags & O_NONBLOCK);
>> -	ret = min_t(ssize_t, ret, len); /* XXX copied from tun.c. Why? */
>> +	ret = min_t(ssize_t, ret, len);
>>  	if (ret > 0)
>>  		iocb->ki_pos = ret;
>>  out:
>
>
>> -- 
>> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 1/2] tun: unbreak truncated packet signalling

[Michael S. Tsirkin]

On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote:
> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
> (tuntap: hardware vlan tx support) breaks the truncated packet signal by never
> return a length greater than iov length in tun_put_user(). This patch fixes this
> by always return the length of packet plus possible vlan header. Caller can
> detect the truncated packet by comparing the return value and the size of iov
> length.
> 
> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

So writer gets back a value greater than what was written?

> ---
> The patch is needed for stable.
> ---
>  drivers/net/tun.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e26cbea..dd1bd7a 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			    const struct iovec *iv, int len)
>  {
>  	struct tun_pi pi = { 0, skb->protocol };
> -	ssize_t total = 0;
> +	struct {
> +		__be16 h_vlan_proto;
> +		__be16 h_vlan_TCI;
> +	} veth;
> +	ssize_t total = 0, off = 0;

Why off = 0 here?
We initialize it to total unconditionally, don't we?

>  	int vlan_offset = 0;
>  
>  	if (!(tun->flags & TUN_NO_PI)) {
> @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		total += tun->vnet_hdr_sz;
>  	}
>  
> +	off = total;
>  	if (!vlan_tx_tag_present(skb)) {
>  		len = min_t(int, skb->len, len);
>  	} else {
>  		int copy, ret;
> -		struct {
> -			__be16 h_vlan_proto;
> -			__be16 h_vlan_TCI;
> -		} veth;
>  
>  		veth.h_vlan_proto = skb->vlan_proto;
>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
> @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>  
>  		copy = min_t(int, vlan_offset, len);
> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>  		len -= copy;
> -		total += copy;
> +		off += copy;
>  		if (ret || !len)
>  			goto done;
>  
>  		copy = min_t(int, sizeof(veth), len);
> -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>  		len -= copy;
> -		total += copy;
> +		off += copy;
>  		if (ret || !len)
>  			goto done;

This seems wrong: if one of the branches above is taken, total is
never incremented.

>  	}
>  
> -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
> -	total += len;
> +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
> +	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
>  
>  done:
>  	tun->dev->stats.tx_packets++;

I also think it's inelegant that the veth struct is now in the
outside scope, and the extra ? is also ugly.

Here's a smaller patch to fix all these problems - what do you think?



Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 782e38b..3297e41 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 			    const struct iovec *iv, int len)
 {
 	struct tun_pi pi = { 0, skb->protocol };
-	ssize_t total = 0;
+	ssize_t total = 0, offset;
 	int vlan_offset = 0;
 
 	if (!(tun->flags & TUN_NO_PI)) {
@@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 		total += tun->vnet_hdr_sz;
 	}
 
+	offset = total;
+	total += skb->len;
 	if (!vlan_tx_tag_present(skb)) {
 		len = min_t(int, skb->len, len);
 	} else {
@@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 			__be16 h_vlan_TCI;
 		} veth;
 
+		total += sizeof(veth);
+
 		veth.h_vlan_proto = skb->vlan_proto;
 		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
 
@@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 	}
 
 	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
-	total += len;
 
 done:
 	tun->dev->stats.tx_packets++;

> -- 
> 1.8.3.2

Re: [PATCH net 1/2] tun: unbreak truncated packet signalling

[Michael S. Tsirkin]

On Mon, Dec 09, 2013 at 12:55:29PM +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote:
> > Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
> > (tuntap: hardware vlan tx support) breaks the truncated packet signal by never
> > return a length greater than iov length in tun_put_user(). This patch fixes this
> > by always return the length of packet plus possible vlan header. Caller can
> > detect the truncated packet by comparing the return value and the size of iov
> > length.
> > 
> > Reported-by: Vlad Yasevich <vyasevich@gmail.com>
> > Cc: Vlad Yasevich <vyasevich@gmail.com>
> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> So writer gets back a value greater than what was written?

Pls ignore this question - wrote it before I understood the
patch, and forgot to remove.
The rest of the comments and the proposed alternative patch
still stand.

> > ---
> > The patch is needed for stable.
> > ---
> >  drivers/net/tun.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index e26cbea..dd1bd7a 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> >  			    const struct iovec *iv, int len)
> >  {
> >  	struct tun_pi pi = { 0, skb->protocol };
> > -	ssize_t total = 0;
> > +	struct {
> > +		__be16 h_vlan_proto;
> > +		__be16 h_vlan_TCI;
> > +	} veth;
> > +	ssize_t total = 0, off = 0;
> 
> Why off = 0 here?
> We initialize it to total unconditionally, don't we?
> 
> >  	int vlan_offset = 0;
> >  
> >  	if (!(tun->flags & TUN_NO_PI)) {
> > @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> >  		total += tun->vnet_hdr_sz;
> >  	}
> >  
> > +	off = total;
> >  	if (!vlan_tx_tag_present(skb)) {
> >  		len = min_t(int, skb->len, len);
> >  	} else {
> >  		int copy, ret;
> > -		struct {
> > -			__be16 h_vlan_proto;
> > -			__be16 h_vlan_TCI;
> > -		} veth;
> >  
> >  		veth.h_vlan_proto = skb->vlan_proto;
> >  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
> > @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> >  		len = min_t(int, skb->len + VLAN_HLEN, len);
> >  
> >  		copy = min_t(int, vlan_offset, len);
> > -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
> > +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
> >  		len -= copy;
> > -		total += copy;
> > +		off += copy;
> >  		if (ret || !len)
> >  			goto done;
> >  
> >  		copy = min_t(int, sizeof(veth), len);
> > -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
> > +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
> >  		len -= copy;
> > -		total += copy;
> > +		off += copy;
> >  		if (ret || !len)
> >  			goto done;
> 
> This seems wrong: if one of the branches above is taken, total is
> never incremented.
> 
> >  	}
> >  
> > -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
> > -	total += len;
> > +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
> > +	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
> >  
> >  done:
> >  	tun->dev->stats.tx_packets++;
> 
> I also think it's inelegant that the veth struct is now in the
> outside scope, and the extra ? is also ugly.
> 
> Here's a smaller patch to fix all these problems - what do you think?
> 
> 
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 782e38b..3297e41 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			    const struct iovec *iv, int len)
>  {
>  	struct tun_pi pi = { 0, skb->protocol };
> -	ssize_t total = 0;
> +	ssize_t total = 0, offset;
>  	int vlan_offset = 0;
>  
>  	if (!(tun->flags & TUN_NO_PI)) {
> @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		total += tun->vnet_hdr_sz;
>  	}
>  
> +	offset = total;
> +	total += skb->len;
>  	if (!vlan_tx_tag_present(skb)) {
>  		len = min_t(int, skb->len, len);
>  	} else {
> @@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			__be16 h_vlan_TCI;
>  		} veth;
>  
> +		total += sizeof(veth);
> +
>  		veth.h_vlan_proto = skb->vlan_proto;
>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>  
> @@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  	}
>  
>  	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
> -	total += len;
>  
>  done:
>  	tun->dev->stats.tx_packets++;
> 
> > -- 
> > 1.8.3.2

Re: [PATCH net 1/2] tun: unbreak truncated packet signalling

[Jason Wang]

On 12/09/2013 06:56 PM, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2013 at 12:55:29PM +0200, Michael S. Tsirkin wrote:
>> On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote:
>>> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
>>> (tuntap: hardware vlan tx support) breaks the truncated packet signal by never
>>> return a length greater than iov length in tun_put_user(). This patch fixes this
>>> by always return the length of packet plus possible vlan header. Caller can
>>> detect the truncated packet by comparing the return value and the size of iov
>>> length.
>>>
>>> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Cc: Vlad Yasevich <vyasevich@gmail.com>
>>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> So writer gets back a value greater than what was written?
> Pls ignore this question - wrote it before I understood the
> patch, and forgot to remove.
> The rest of the comments and the proposed alternative patch
> still stand.
>
>>> ---
>>> The patch is needed for stable.
>>> ---
>>>  drivers/net/tun.c | 23 ++++++++++++-----------
>>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index e26cbea..dd1bd7a 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>>  			    const struct iovec *iv, int len)
>>>  {
>>>  	struct tun_pi pi = { 0, skb->protocol };
>>> -	ssize_t total = 0;
>>> +	struct {
>>> +		__be16 h_vlan_proto;
>>> +		__be16 h_vlan_TCI;
>>> +	} veth;
>>> +	ssize_t total = 0, off = 0;
>> Why off = 0 here?
>> We initialize it to total unconditionally, don't we?

True, it's useless.
>>>  	int vlan_offset = 0;
>>>  
>>>  	if (!(tun->flags & TUN_NO_PI)) {
>>> @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>>  		total += tun->vnet_hdr_sz;
>>>  	}
>>>  
>>> +	off = total;
>>>  	if (!vlan_tx_tag_present(skb)) {
>>>  		len = min_t(int, skb->len, len);
>>>  	} else {
>>>  		int copy, ret;
>>> -		struct {
>>> -			__be16 h_vlan_proto;
>>> -			__be16 h_vlan_TCI;
>>> -		} veth;
>>>  
>>>  		veth.h_vlan_proto = skb->vlan_proto;
>>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>>> @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>>>  
>>>  		copy = min_t(int, vlan_offset, len);
>>> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
>>> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>>>  		len -= copy;
>>> -		total += copy;
>>> +		off += copy;
>>>  		if (ret || !len)
>>>  			goto done;
>>>  
>>>  		copy = min_t(int, sizeof(veth), len);
>>> -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
>>> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>>>  		len -= copy;
>>> -		total += copy;
>>> +		off += copy;
>>>  		if (ret || !len)
>>>  			goto done;
>> This seems wrong: if one of the branches above is taken, total is
>> never incremented.

Right.
>>>  	}
>>>  
>>> -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
>>> -	total += len;
>>> +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
>>> +	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
>>>  
>>>  done:
>>>  	tun->dev->stats.tx_packets++;
>> I also think it's inelegant that the veth struct is now in the
>> outside scope, and the extra ? is also ugly.
>>
>> Here's a smaller patch to fix all these problems - what do you think?
>>
>>
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> ---
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 782e38b..3297e41 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  			    const struct iovec *iv, int len)
>>  {
>>  	struct tun_pi pi = { 0, skb->protocol };
>> -	ssize_t total = 0;
>> +	ssize_t total = 0, offset;
>>  	int vlan_offset = 0;
>>  
>>  	if (!(tun->flags & TUN_NO_PI)) {
>> @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  		total += tun->vnet_hdr_sz;
>>  	}
>>  
>> +	offset = total;
>> +	total += skb->len;
>>  	if (!vlan_tx_tag_present(skb)) {
>>  		len = min_t(int, skb->len, len);
>>  	} else {
>> @@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  			__be16 h_vlan_TCI;
>>  		} veth;
>>  
>> +		total += sizeof(veth);
>> +
>>  		veth.h_vlan_proto = skb->vlan_proto;
>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>>  
>> @@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  	}
>>  
>>  	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
>> -	total += len;
>>  

We should use offset here and it should be advanced during vlan tag putting.
>>  done:
>>  	tun->dev->stats.tx_packets++;
>>
>>> -- 
>>> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net 1/2] tun: unbreak truncated packet signalling

[Vlad Yasevich]

On 12/09/2013 05:55 AM, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote:
>> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
>> (tuntap: hardware vlan tx support) breaks the truncated packet signal
by never
>> return a length greater than iov length in tun_put_user(). This patch
fixes this
>> by always return the length of packet plus possible vlan header.
Caller can
>> detect the truncated packet by comparing the return value and the
size of iov
>> length.
>>
>> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
>> Cc: Vlad Yasevich <vyasevich@gmail.com>
>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> So writer gets back a value greater than what was written?
>
>> ---
>> The patch is needed for stable.
>> ---
>>  drivers/net/tun.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index e26cbea..dd1bd7a 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct
*tun,
>>  			    const struct iovec *iv, int len)
>>  {
>>  	struct tun_pi pi = { 0, skb->protocol };
>> -	ssize_t total = 0;
>> +	struct {
>> +		__be16 h_vlan_proto;
>> +		__be16 h_vlan_TCI;
>> +	} veth;
>> +	ssize_t total = 0, off = 0;
>
> Why off = 0 here?
> We initialize it to total unconditionally, don't we?
>
>>  	int vlan_offset = 0;
>>
>>  	if (!(tun->flags & TUN_NO_PI)) {
>> @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct
*tun,
>>  		total += tun->vnet_hdr_sz;
>>  	}
>>
>> +	off = total;
>>  	if (!vlan_tx_tag_present(skb)) {
>>  		len = min_t(int, skb->len, len);
>>  	} else {
>>  		int copy, ret;
>> -		struct {
>> -			__be16 h_vlan_proto;
>> -			__be16 h_vlan_TCI;
>> -		} veth;
>>
>>  		veth.h_vlan_proto = skb->vlan_proto;
>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>> @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct
*tun,
>>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>>
>>  		copy = min_t(int, vlan_offset, len);
>> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
>> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>>  		len -= copy;
>> -		total += copy;
>> +		off += copy;
>>  		if (ret || !len)
>>  			goto done;
>>
>>  		copy = min_t(int, sizeof(veth), len);
>> -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
>> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>>  		len -= copy;
>> -		total += copy;
>> +		off += copy;
>>  		if (ret || !len)
>>  			goto done;
>
> This seems wrong: if one of the branches above is taken, total is
> never incremented.
>
>>  	}
>>
>> -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
>> -	total += len;
>> +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
>> +	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
>>
>>  done:
>>  	tun->dev->stats.tx_packets++;
>
> I also think it's inelegant that the veth struct is now in the
> outside scope, and the extra ? is also ugly.
>
> Here's a smaller patch to fix all these problems - what do you think?
>
>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 782e38b..3297e41 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			    const struct iovec *iv, int len)
>  {
>  	struct tun_pi pi = { 0, skb->protocol };
> -	ssize_t total = 0;
> +	ssize_t total = 0, offset;
>  	int vlan_offset = 0;
>
>  	if (!(tun->flags & TUN_NO_PI)) {
> @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		total += tun->vnet_hdr_sz;
>  	}
>
> +	offset = total;
> +	total += skb->len;
>  	if (!vlan_tx_tag_present(skb)) {
>  		len = min_t(int, skb->len, len);
>  	} else {
> @@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			__be16 h_vlan_TCI;
>  		} veth;
>
> +		total += sizeof(veth);
> +
>  		veth.h_vlan_proto = skb->vlan_proto;
>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>
> @@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  	}
>
>  	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
> -	total += len;
>
>  done:
>  	tun->dev->stats.tx_packets++;
>


You have to use 'offset' instead of 'total' when doing skb_copy and
adjust offset as you write the vlan header.

I think something like this will fix it:

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 782e38b..d71c393 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 			    const struct iovec *iv, int len)
 {
 	struct tun_pi pi = { 0, skb->protocol };
-	ssize_t total = 0;
+	ssize_t total = 0, offset;
 	int vlan_offset = 0;

 	if (!(tun->flags & TUN_NO_PI)) {
@@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 		total += tun->vnet_hdr_sz;
 	}

+	offset = total;
+	total += skb->len;
 	if (!vlan_tx_tag_present(skb)) {
 		len = min_t(int, skb->len, len);
 	} else {
@@ -1262,24 +1264,24 @@ static ssize_t tun_put_user(struct tun_struct *tun,

 		vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto);
 		len = min_t(int, skb->len + VLAN_HLEN, len);
+		total += VLAN_HLEN;

 		copy = min_t(int, vlan_offset, len);
-		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
+		ret = skb_copy_datagram_const_iovec(skb, 0, iv, offset, copy);
 		len -= copy;
-		total += copy;
+		offset += copy;
 		if (ret || !len)
 			goto done;

 		copy = min_t(int, sizeof(veth), len);
-		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
+		ret = memcpy_toiovecend(iv, (void *)&veth, offset, copy);
 		len -= copy;
-		total += copy;
+		offset += copy;
 		if (ret || !len)
 			goto done;
 	}

-	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
-	total += len;
+	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, offset, len);

 done:
 	tun->dev->stats.tx_packets++;

-vlad

Re: [PATCH net 1/2] tun: unbreak truncated packet signalling

[Jason Wang]

On 12/09/2013 11:31 PM, Vlad Yasevich wrote:
> On 12/09/2013 05:55 AM, Michael S. Tsirkin wrote:
>> On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote:
>>> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
>>> (tuntap: hardware vlan tx support) breaks the truncated packet signal
> by never
>>> return a length greater than iov length in tun_put_user(). This patch
> fixes this
>>> by always return the length of packet plus possible vlan header.
> Caller can
>>> detect the truncated packet by comparing the return value and the
> size of iov
>>> length.
>>>
>>> Reported-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Cc: Vlad Yasevich <vyasevich@gmail.com>
>>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> So writer gets back a value greater than what was written?
>>
>>> ---
>>> The patch is needed for stable.
>>> ---
>>>  drivers/net/tun.c | 23 ++++++++++++-----------
>>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index e26cbea..dd1bd7a 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct
> *tun,
>>>  			    const struct iovec *iv, int len)
>>>  {
>>>  	struct tun_pi pi = { 0, skb->protocol };
>>> -	ssize_t total = 0;
>>> +	struct {
>>> +		__be16 h_vlan_proto;
>>> +		__be16 h_vlan_TCI;
>>> +	} veth;
>>> +	ssize_t total = 0, off = 0;
>> Why off = 0 here?
>> We initialize it to total unconditionally, don't we?
>>
>>>  	int vlan_offset = 0;
>>>
>>>  	if (!(tun->flags & TUN_NO_PI)) {
>>> @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct
> *tun,
>>>  		total += tun->vnet_hdr_sz;
>>>  	}
>>>
>>> +	off = total;
>>>  	if (!vlan_tx_tag_present(skb)) {
>>>  		len = min_t(int, skb->len, len);
>>>  	} else {
>>>  		int copy, ret;
>>> -		struct {
>>> -			__be16 h_vlan_proto;
>>> -			__be16 h_vlan_TCI;
>>> -		} veth;
>>>
>>>  		veth.h_vlan_proto = skb->vlan_proto;
>>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>>> @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct
> *tun,
>>>  		len = min_t(int, skb->len + VLAN_HLEN, len);
>>>
>>>  		copy = min_t(int, vlan_offset, len);
>>> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
>>> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy);
>>>  		len -= copy;
>>> -		total += copy;
>>> +		off += copy;
>>>  		if (ret || !len)
>>>  			goto done;
>>>
>>>  		copy = min_t(int, sizeof(veth), len);
>>> -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
>>> +		ret = memcpy_toiovecend(iv, (void *)&veth, off, copy);
>>>  		len -= copy;
>>> -		total += copy;
>>> +		off += copy;
>>>  		if (ret || !len)
>>>  			goto done;
>> This seems wrong: if one of the branches above is taken, total is
>> never incremented.
>>
>>>  	}
>>>
>>> -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
>>> -	total += len;
>>> +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len);
>>> +	total += skb->len + (vlan_offset ? sizeof(veth) : 0);
>>>
>>>  done:
>>>  	tun->dev->stats.tx_packets++;
>> I also think it's inelegant that the veth struct is now in the
>> outside scope, and the extra ? is also ugly.
>>
>> Here's a smaller patch to fix all these problems - what do you think?
>>
>>
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> ---
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 782e38b..3297e41 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  			    const struct iovec *iv, int len)
>>  {
>>  	struct tun_pi pi = { 0, skb->protocol };
>> -	ssize_t total = 0;
>> +	ssize_t total = 0, offset;
>>  	int vlan_offset = 0;
>>
>>  	if (!(tun->flags & TUN_NO_PI)) {
>> @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  		total += tun->vnet_hdr_sz;
>>  	}
>>
>> +	offset = total;
>> +	total += skb->len;
>>  	if (!vlan_tx_tag_present(skb)) {
>>  		len = min_t(int, skb->len, len);
>>  	} else {
>> @@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  			__be16 h_vlan_TCI;
>>  		} veth;
>>
>> +		total += sizeof(veth);
>> +
>>  		veth.h_vlan_proto = skb->vlan_proto;
>>  		veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb));
>>
>> @@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  	}
>>
>>  	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
>> -	total += len;
>>
>>  done:
>>  	tun->dev->stats.tx_packets++;
>>
>
> You have to use 'offset' instead of 'total' when doing skb_copy and
> adjust offset as you write the vlan header.
>
> I think something like this will fix it:
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 782e38b..d71c393 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  			    const struct iovec *iv, int len)
>  {
>  	struct tun_pi pi = { 0, skb->protocol };
> -	ssize_t total = 0;
> +	ssize_t total = 0, offset;
>  	int vlan_offset = 0;
>
>  	if (!(tun->flags & TUN_NO_PI)) {
> @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		total += tun->vnet_hdr_sz;
>  	}
>
> +	offset = total;
> +	total += skb->len;
>  	if (!vlan_tx_tag_present(skb)) {
>  		len = min_t(int, skb->len, len);
>  	} else {
> @@ -1262,24 +1264,24 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>
>  		vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto);
>  		len = min_t(int, skb->len + VLAN_HLEN, len);
> +		total += VLAN_HLEN;
>
>  		copy = min_t(int, vlan_offset, len);
> -		ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy);
> +		ret = skb_copy_datagram_const_iovec(skb, 0, iv, offset, copy);
>  		len -= copy;
> -		total += copy;
> +		offset += copy;
>  		if (ret || !len)
>  			goto done;
>
>  		copy = min_t(int, sizeof(veth), len);
> -		ret = memcpy_toiovecend(iv, (void *)&veth, total, copy);
> +		ret = memcpy_toiovecend(iv, (void *)&veth, offset, copy);
>  		len -= copy;
> -		total += copy;
> +		offset += copy;
>  		if (ret || !len)
>  			goto done;
>  	}
>
> -	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
> -	total += len;
> +	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, offset, len);
>
>  done:
>  	tun->dev->stats.tx_packets++;
>
> -vlad

This looks good, will send a formal V2 with your sign-off.

Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/