| Message ID | 1390187808-5462-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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. -- 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
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. 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- 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
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@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. 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@vger.kernel.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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)