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

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

[Jason Wang]

Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
(tuntap: hardware vlan tx support) breaks the truncated packet signal by nev
return a length greater than iov length in tun_put_user(). This patch fixes
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 io
length.

Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from v1:
- increase total unconditionally
- do not move veth structure out of the vlan handing block
---
 drivers/net/tun.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e26cbea..cd142134 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1184,7 +1184,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 {
 	struct tun_pi pi = { 0, skb->protocol };
 	ssize_t total = 0;
-	int vlan_offset = 0;
+	int vlan_offset = 0, offset;
 
 	if (!(tun->flags & TUN_NO_PI)) {
 		if ((len -= sizeof(pi)) < 0)
@@ -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++;
-- 
1.8.3.2

[PATCH net V2 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 trunca
checking work.

Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from v1:
- increase total unconditionally
- do not move the structure veth out of the vlan handling block
---
 drivers/net/macvtap.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 957cc5c..ded4b2c 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -770,7 +770,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
 	int ret;
 	int vnet_hdr_len = 0;
 	int vlan_offset = 0;
-	int copied;
+	int copied, offset;
 
 	if (q->flags & IFF_VNET_HDR) {
 		struct virtio_net_hdr vnet_hdr;
@@ -785,7 +785,8 @@ 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;
+	offset = copied = vnet_hdr_len;
+	copied += skb->len;
 
 	if (!vlan_tx_tag_present(skb))
 		len = min_t(int, skb->len, len);
@@ -800,24 +801,24 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
 
 		vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto);
 		len = min_t(int, skb->len + VLAN_HLEN, len);
+		copied += VLAN_HLEN;
 
 		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, offset, copy);
 		len -= copy;
-		copied += copy;
+		offset += 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, offset, copy);
 		len -= copy;
-		copied += copy;
+		offset += 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, offset, len);
 
 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 V2 2/2] macvtap: signal truncated packets

[Michael S. Tsirkin]

On Tue, Dec 10, 2013 at 01:49:46PM +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 trunca
> checking work.
> 
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from v1:
> - increase total unconditionally
> - do not move the structure veth out of the vlan handling block
> ---
>  drivers/net/macvtap.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 957cc5c..ded4b2c 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -770,7 +770,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>  	int ret;
>  	int vnet_hdr_len = 0;
>  	int vlan_offset = 0;
> -	int copied;
> +	int copied, offset;
>  
>  	if (q->flags & IFF_VNET_HDR) {
>  		struct virtio_net_hdr vnet_hdr;
> @@ -785,7 +785,8 @@ 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;
> +	offset = copied = vnet_hdr_len;
> +	copied += skb->len;
>  
>  	if (!vlan_tx_tag_present(skb))
>  		len = min_t(int, skb->len, len);
> @@ -800,24 +801,24 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>  
>  		vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto);
>  		len = min_t(int, skb->len + VLAN_HLEN, len);
> +		copied += VLAN_HLEN;
>  
>  		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, offset, copy);
>  		len -= copy;
> -		copied += copy;
> +		offset += 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, offset, copy);
>  		len -= copy;
> -		copied += copy;
> +		offset += 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, offset, len);
>  
>  done:
>  	return ret ? ret : copied;

I commented on this already. copied is how much we copied,
so its value is correct.
You want to name the new one total_len or something along
these lines and return it.

> @@ -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 V2 2/2] macvtap: signal truncated packets

[Jason Wang]

On 12/10/2013 11:29 PM, Michael S. Tsirkin wrote:
> On Tue, Dec 10, 2013 at 01:49:46PM +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 trunca
>> > checking work.
>> > 
>> > Cc: Vlad Yasevich <vyasevich@gmail.com>
>> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>> > ---
>> > Changes from v1:
>> > - increase total unconditionally
>> > - do not move the structure veth out of the vlan handling block
>> > ---
>> >  drivers/net/macvtap.c | 19 ++++++++++---------
>> >  1 file changed, 10 insertions(+), 9 deletions(-)
>> > 
>> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> > index 957cc5c..ded4b2c 100644
>> > --- a/drivers/net/macvtap.c
>> > +++ b/drivers/net/macvtap.c
>> > @@ -770,7 +770,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>> >  	int ret;
>> >  	int vnet_hdr_len = 0;
>> >  	int vlan_offset = 0;
>> > -	int copied;
>> > +	int copied, offset;
>> >  
>> >  	if (q->flags & IFF_VNET_HDR) {
>> >  		struct virtio_net_hdr vnet_hdr;
>> > @@ -785,7 +785,8 @@ 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;
>> > +	offset = copied = vnet_hdr_len;
>> > +	copied += skb->len;
>> >  
>> >  	if (!vlan_tx_tag_present(skb))
>> >  		len = min_t(int, skb->len, len);
>> > @@ -800,24 +801,24 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>> >  
>> >  		vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto);
>> >  		len = min_t(int, skb->len + VLAN_HLEN, len);
>> > +		copied += VLAN_HLEN;
>> >  
>> >  		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, offset, copy);
>> >  		len -= copy;
>> > -		copied += copy;
>> > +		offset += 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, offset, copy);
>> >  		len -= copy;
>> > -		copied += copy;
>> > +		offset += 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, offset, len);
>> >  
>> >  done:
>> >  	return ret ? ret : copied;
> I commented on this already. copied is how much we copied,
> so its value is correct.
> You want to name the new one total_len or something along
> these lines and return it.
>

Ok, to be same with tun, I will use "total" in V3.

Thanks

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

[Michael S. Tsirkin]

On Tue, Dec 10, 2013 at 01:49:45PM +0800, Jason Wang wrote:
> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
> (tuntap: hardware vlan tx support) breaks the truncated packet signal by nev
> return a length greater than iov length in tun_put_user(). This patch fixes
> 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 io
> length.
> 
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from v1:
> - increase total unconditionally
> - do not move veth structure out of the vlan handing block
> ---
>  drivers/net/tun.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e26cbea..cd142134 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1184,7 +1184,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  {
>  	struct tun_pi pi = { 0, skb->protocol };
>  	ssize_t total = 0;
> -	int vlan_offset = 0;
> +	int vlan_offset = 0, offset;
>  
>  	if (!(tun->flags & TUN_NO_PI)) {
>  		if ((len -= sizeof(pi)) < 0)
> @@ -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++;

offset is not descriptive. I would call new variable "copied",
and change all code to use that, do
total = copied + skb->len, total is then the total length.

> -- 
> 1.8.3.2

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

[Jason Wang]

On 12/10/2013 11:32 PM, Michael S. Tsirkin wrote:
> On Tue, Dec 10, 2013 at 01:49:45PM +0800, Jason Wang wrote:
>> > Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532
>> > (tuntap: hardware vlan tx support) breaks the truncated packet signal by nev
>> > return a length greater than iov length in tun_put_user(). This patch fixes
>> > 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 io
>> > length.
>> > 
>> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
>> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>> > ---
>> > Changes from v1:
>> > - increase total unconditionally
>> > - do not move veth structure out of the vlan handing block
>> > ---
>> >  drivers/net/tun.c | 16 +++++++++-------
>> >  1 file changed, 9 insertions(+), 7 deletions(-)
>> > 
>> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> > index e26cbea..cd142134 100644
>> > --- a/drivers/net/tun.c
>> > +++ b/drivers/net/tun.c
>> > @@ -1184,7 +1184,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>> >  {
>> >  	struct tun_pi pi = { 0, skb->protocol };
>> >  	ssize_t total = 0;
>> > -	int vlan_offset = 0;
>> > +	int vlan_offset = 0, offset;
>> >  
>> >  	if (!(tun->flags & TUN_NO_PI)) {
>> >  		if ((len -= sizeof(pi)) < 0)
>> > @@ -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++;
> offset is not descriptive. I would call new variable "copied",
> and change all code to use that, do
> total = copied + skb->len, total is then the total length.
>

Ok, will do it in v3.

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

[David Miller]

You and Michael are still discussing these changes it seems.

I accidently commited the first version of these patches, but then
immediately reverted that after I saw the followups.

Let me know when you have something both of you are happy with.

Thanks.

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

[Jason Wang]

On 12/11/2013 11:11 AM, David Miller wrote:
> You and Michael are still discussing these changes it seems.
>
> I accidently commited the first version of these patches, but then
> immediately reverted that after I saw the followups.
>
> Let me know when you have something both of you are happy with.
>
> Thanks.

Sure, I will post v3 which should address all comments from Michael.

Thanks