[PATCH net] tun: handle copy failure in tun_put_user()

[PATCH net] tun: handle copy failure in tun_put_user()

[Jason Wang]

This patch return the error code of copy helpers in tun_put_user() instead of
ignoring them.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/tun.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ecec802..4ec8f28 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1185,7 +1185,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, copied;
+	int vlan_offset = 0, copied, ret;
 
 	if (!(tun->flags & TUN_NO_PI)) {
 		if ((len -= sizeof(pi)) < 0)
@@ -1254,7 +1254,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 	if (!vlan_tx_tag_present(skb)) {
 		len = min_t(int, skb->len, len);
 	} else {
-		int copy, ret;
+		int copy;
 		struct {
 			__be16 h_vlan_proto;
 			__be16 h_vlan_TCI;
@@ -1282,13 +1282,13 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 			goto done;
 	}
 
-	skb_copy_datagram_const_iovec(skb, vlan_offset, iv, copied, len);
+	ret = skb_copy_datagram_const_iovec(skb, vlan_offset, iv, copied, len);
 
 done:
 	tun->dev->stats.tx_packets++;
 	tun->dev->stats.tx_bytes += len;
 
-	return total;
+	return ret ? ret : total;
 }
 
 static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
-- 
1.8.3.2

Re: [PATCH net] tun: handle copy failure in tun_put_user()

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Mon, 20 Jan 2014 11:16:48 +0800

> This patch return the error code of copy helpers in tun_put_user() instead of
> ignoring them.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

If you perform some of the copy successfully, you have to report that
length rather than just an error.

Otherwise userland has no way to determine how much of the data was
successfully sourced.

I'm not applying this, sorry.

Re: [PATCH net] tun: handle copy failure in tun_put_user()

[Jason Wang]

On 01/20/2014 11:48 AM, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Mon, 20 Jan 2014 11:16:48 +0800
>
>> This patch return the error code of copy helpers in tun_put_user() instead of
>> ignoring them.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> If you perform some of the copy successfully, you have to report that
> length rather than just an error.
>
> Otherwise userland has no way to determine how much of the data was
> successfully sourced.
>
> I'm not applying this, sorry.

Right, looks like we need more changes in tun to return the accurate
length copied in this case.

Re: [PATCH net] tun: handle copy failure in tun_put_user()

[Michael S. Tsirkin]

On Sun, Jan 19, 2014 at 07:48:56PM -0800, David Miller wrote:
> From: Jason Wang <jasowang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date: Mon, 20 Jan 2014 11:16:48 +0800
> 
> > This patch return the error code of copy helpers in tun_put_user() instead of
> > ignoring them.
> > 
> > Cc: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Jason Wang <jasowang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I'm not sure we need to worry about this too much.
But if yes, a bunch of places besides tun should be
changed. Consider for example udp_recvmsg: it
never seems to return any error except -EAGAIN.

Is this a bug? Man page for recvmsg says:
     EFAULT The receive buffer pointer(s)  point  outside  the process's  address
              space.

this isn't very clear: does this mean "all pointers are invalid"
or "some pointers are invalid"?
Also, what if pointers themselves are valid but length
makes us go outside the address space?

I'm guessing the simplest way is to clarify in the man page that
passing invalid pointers / lengths is not guaranteed
to result in EFAULT and that Linux makes no guarantees
about the returned length in this case.

Cc linux-man in case they can suggest some insights on this.

> If you perform some of the copy successfully, you have to report that
> length rather than just an error.
> 
> Otherwise userland has no way to determine how much of the data was
> successfully sourced.
>
> I'm not applying this, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] tun: handle copy failure in tun_put_user()

[Jason Wang]

On 01/20/2014 04:43 PM, Michael S. Tsirkin wrote:
> On Sun, Jan 19, 2014 at 07:48:56PM -0800, David Miller wrote:
>> From: Jason Wang <jasowang@redhat.com>
>> Date: Mon, 20 Jan 2014 11:16:48 +0800
>>
>>> This patch return the error code of copy helpers in tun_put_user() instead of
>>> ignoring them.
>>>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I'm not sure we need to worry about this too much.
> But if yes, a bunch of places besides tun should be
> changed.

Yes, I send the patch because the error processing here is different
from what macvtap does. Macvtap just return error in this case and so do
packet socket.
>  Consider for example udp_recvmsg: it
> never seems to return any error except -EAGAIN.
>
> Is this a bug? Man page for recvmsg says:
>      EFAULT The receive buffer pointer(s)  point  outside  the process's  address
>               space.
>
> this isn't very clear: does this mean "all pointers are invalid"
> or "some pointers are invalid"?
> Also, what if pointers themselves are valid but length
> makes us go outside the address space?
>
> I'm guessing the simplest way is to clarify in the man page that
> passing invalid pointers / lengths is not guaranteed
> to result in EFAULT and that Linux makes no guarantees
> about the returned length in this case.
>
> Cc linux-man in case they can suggest some insights on this.
>
>> If you perform some of the copy successfully, you have to report that
>> length rather than just an error.
>>
>> Otherwise userland has no way to determine how much of the data was
>> successfully sourced.
>>
>> I'm not applying this, sorry.
> --
> 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] tun: handle copy failure in tun_put_user()

[Michael S. Tsirkin]

On Mon, Jan 20, 2014 at 05:32:02PM +0800, Jason Wang wrote:
> On 01/20/2014 04:43 PM, Michael S. Tsirkin wrote:
> > On Sun, Jan 19, 2014 at 07:48:56PM -0800, David Miller wrote:
> >> From: Jason Wang <jasowang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> Date: Mon, 20 Jan 2014 11:16:48 +0800
> >>
> >>> This patch return the error code of copy helpers in tun_put_user() instead of
> >>> ignoring them.
> >>>
> >>> Cc: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> Signed-off-by: Jason Wang <jasowang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > I'm not sure we need to worry about this too much.
> > But if yes, a bunch of places besides tun should be
> > changed.
> 
> Yes, I send the patch because the error processing here is different
> from what macvtap does. Macvtap just return error in this case and so do
> packet socket.

I suspect we just need to document that invalid address simply results
in unspecified behaviour.  We try to return EFAULT to help debugging
sometimes but it's on a best effort basis.
>From this point of view EFAULT seems easier to debug than truncating the packet.
In any case even if we change Linux - applications won't be able to rely
on this for a long while.
So maybe we shouldn't do anything.


> >  Consider for example udp_recvmsg: it
> > never seems to return any error except -EAGAIN.
> >
> > Is this a bug? Man page for recvmsg says:
> >      EFAULT The receive buffer pointer(s)  point  outside  the process's  address
> >               space.
> >
> > this isn't very clear: does this mean "all pointers are invalid"
> > or "some pointers are invalid"?
> > Also, what if pointers themselves are valid but length
> > makes us go outside the address space?
> >
> > I'm guessing the simplest way is to clarify in the man page that
> > passing invalid pointers / lengths is not guaranteed
> > to result in EFAULT and that Linux makes no guarantees
> > about the returned length in this case.
> >
> > Cc linux-man in case they can suggest some insights on this.
> >
> >> If you perform some of the copy successfully, you have to report that
> >> length rather than just an error.
> >>
> >> Otherwise userland has no way to determine how much of the data was
> >> successfully sourced.
> >>
> >> I'm not applying this, sorry.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html