[PATCH v2 1/2] macvtap: update file current position

[PATCH v2 1/2] macvtap: update file current position

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 drivers/net/macvtap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 9093004..e6e2dd6 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -876,6 +876,8 @@ 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? */
+	if (ret > 0)
+		iocb->ki_pos += ret;
 out:
 	return ret;
 }
-- 
1.7.6.5

[PATCH v2 2/2] tun: update file current position

[Zhi Yong Wu]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 drivers/net/tun.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 782e38b..c8ddbd0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1356,6 +1356,8 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
 	ret = tun_do_read(tun, tfile, iocb, iv, len,
 			  file->f_flags & O_NONBLOCK);
 	ret = min_t(ssize_t, ret, len);
+	if (ret > 0)
+		iocb->ki_pos += ret;
 out:
 	tun_put(tun);
 	return ret;
-- 
1.7.6.5

Re: [PATCH v2 2/2] tun: update file current position

[David Miller]

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Fri,  6 Dec 2013 17:08:50 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Also applied and queued up for -stable, thanks.

I noticed in these two cases that that min_t() adjustment of 'ret'
seems strange.  I can't understand why it's needed.

If, for example, tun_do_read() really did read more than 'len'
bytes:

1) That would write past the end of the buffer.

2) Writing a different value to the ->ki_pos would mean
   that ->ki_pos is now inaccurate.

Unless someone can explain why the min_t() is needed, we should remove
it.

Re: [PATCH v2 2/2] tun: update file current position

[Zhi Yong Wu]

On Sat, Dec 7, 2013 at 1:45 AM, David Miller <davem@davemloft.net> wrote:
> From: Zhi Yong Wu <zwu.kernel@gmail.com>
> Date: Fri,  6 Dec 2013 17:08:50 +0800
>
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
> Also applied and queued up for -stable, thanks.
>
> I noticed in these two cases that that min_t() adjustment of 'ret'
> seems strange.  I can't understand why it's needed.
>
> If, for example, tun_do_read() really did read more than 'len'
> bytes:
>
> 1) That would write past the end of the buffer.
>
> 2) Writing a different value to the ->ki_pos would mean
>    that ->ki_pos is now inaccurate.
>
> Unless someone can explain why the min_t() is needed, we should remove
> it.
Yes, from my side, it seems to be impossible that ret is bigger than
let or total_len.
So we also remove the branch "if (ret > total_len) {...}" in xxx_rcvmsg().
If you hope to submit the patch for this, please let me know, thanks.


-- 
Regards,

Zhi Yong Wu

Re: [PATCH v2 2/2] tun: update file current position

[David Miller]

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Sat, 7 Dec 2013 04:32:20 +0800

> If you hope to submit the patch for this, please let me know, thanks.

Someone should :-)

Re: [PATCH v2 2/2] tun: update file current position

[Vlad Yasevich]

On 12/06/2013 12:45 PM, David Miller wrote:
> From: Zhi Yong Wu <zwu.kernel@gmail.com>
> Date: Fri,  6 Dec 2013 17:08:50 +0800
> 
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Also applied and queued up for -stable, thanks.
> 
> I noticed in these two cases that that min_t() adjustment of 'ret'
> seems strange.  I can't understand why it's needed.
> 
> If, for example, tun_do_read() really did read more than 'len'
> bytes:
> 
> 1) That would write past the end of the buffer.
> 
> 2) Writing a different value to the ->ki_pos would mean
>    that ->ki_pos is now inaccurate.
> 
> Unless someone can explain why the min_t() is needed, we should remove
> it.

So, back when that code was added, it was actually possible for the
tun_do_read to return a value larger then user specified length, but
the copy would only be done the length bytes.  This was used to signal
MSG_TRUNC.

Specifically we had the following code in tun_put_user()
      len = min_t(int, skb->len, len);

      skb_copy_datagram_const_iovec(skb, 0, iv, total, len);
      total += skb->len;

This has since changed to:
      len = min_t(int, skb->len, len);
      ...
      skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len);
      total += len;

So, no it seems impossible to return a value larger then user specified
length, so MSG_TRUNC will never be set.  It probably makes sense to
signal message truncation, but that seems broken right now.

-vlad

> --
> 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 v2 1/2] macvtap: update file current position

[David Miller]

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Fri,  6 Dec 2013 17:08:49 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Applied and queued up for -stable.