From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 08:47:50
I've spent the last weekend crawling through the copy-and-calculate-csum
primitives on all architectures. It came up during iov_iter work; below
are the results of that review and, in the end, several questions about
the short-term tactics of a change that needs to be done for iov_iter-net.
The API apparently consists of 3 functions - present and exported on all
architectures:
1) csum_and_copy_from_user()
2) csum_and_copy_to_user()
3) csum_partial_copy_nocheck().
There are very few places _using_ those, all but one in net stack (the only
exception is in drivers/net).
The minimal implementations would be
__wsum csum_and_copy_from_user(const void __user *src, void *dst, int len,
__wsum sum, int *err_ptr)
{
if (unlikely(copy_from_user(dst, src, len) < 0)) {
*err_ptr = -EFAULT;
return <whatever>;
}
return csum_partial(dst, len, sum);
}
__wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
__wsum sum, int *err_ptr)
{
sum = csum_partial(src, len, sum);
if (unlikely(copy_to_user(dst, src, len) < 0)) {
*err_ptr = -EFAULT;
return <whatever>;
}
return sum;
}
__wsum csum_partial_copy_nocheck(const void *src, void *dst, int len,
__wsum sum)
{
memcpy(dst, src, len);
return csum_partial(dst, len, sum);
}
Note that we are *not* guaranteed that *err_ptr is touched in case of success
- not all architectures zero it in that case. Callers must (and do) zero it
before the call of csum_and_copy_{from,to}_user(). Furthermore, return value
in case of error is undefined. On the "from" side result is really "anything
whatsoever", on the "to" side most of the architectures end up returning ~0U.
However, even that is not guaranteed - e.g. avr32 and xtensa might return 0
in that case. All existing callers discard the return value in case of error -
most of them by jumping out of the scope of variable it's assigned to. The
only exception is skb_copy_and_csum_datagram(); in that case return value is
discarded by caller of skb_copy_and_csum_datagram() itself (also by jumping
out of scope of the variable it had been put into).
Generally, error in copying from userland ends up zeroing the rest of
destination. However, there are exceptions (which might be considered bugs)
*and* callers discard all the copied data in case of error anyway. There
are several call chains:
1) from skb_add_data(): calls skb_trim() immediately after the failure.
2) from skb_do_copy_data_nocache() called from skb_add_data_nocache():
__skb_trim() in skb_add_data_nocache()
3) from skb_do_copy_data_nocache() called from
skb_copy_to_page_nocache(): we do not increase skb->len until after successful
copying.
4) from skb_copy_to_page(): we do not increase skb->len until after
successful copying.
5) from csum_partial_copy_fromiovecend() from ip_generic_getfrag()
or ping_getfrag(): those are passed as getfrag callback to ip_make_skb(),
ip_append_data() or ip6_append_data(). ip_make_skb() and ip_append_data()
just pass it to __ip_append_data(), so we are left with two fairly similar
functions to analyse.
* __ip_append_data() has 3 places where getfrag() is called directly and
one where it's passed to ip_ufo_append_data(). Direct ones either free skb, or
do __skb_trim() or do not increment skb->len on error. ip_ufo_append_data()
passes it further to skb_append_datato_frags(), where we do not increment skb->len
and friends in case of getfrag() failure.
* ip6_append_data() the situation is identical, with ip6_ufo_append_data()
in place of ip_ufo_append_data().
So for all in-tree users of that sucker we are guaranteed to discard the whole
thing in case of error and this zeroing the tail is pointless.
Note also that the order of src and dest is opposite to normal for memcpy-like
functions. Compared to the rest of the ugliness it's trivial, but it's still
not nice.
IMO the calling conventions are atrocious.
And then there are architecture-specific warts. A relatively minor one is that
csum_partial_copy_nocheck() has a slightly saner name on some architectures -
there it's called csum_partial_copy(). Of course, those architectures have
#define csum_partial_copy_nocheck csum_partial_copy... Worse ones are related
to csum_and_copy_from_user() - on everything other than ppc64 it is an
inlined wrapper around csum_partial_copy_from_user(), which is what's actually
exported. On ppc64 csum_partial_copy_from_user() simply doesn't exist.
The _only_ difference between it and csum_and_copy_from_user() is that the
wrapper does access_ok() check. However, some architectures repeat that
access_ok() in csum_partial_copy_from_user() (and on x86 we have
#define csum_and_copy_from_user csum_partial_copy_from_user, with no
access_ok() in the wrapper). As it is, it's architecture-dependent whether
csum_partial_copy_from_user() does or does not access_ok(); on
alpha, frv, m32r, mn10300, parisc, s390, score and x86 it does, on the
rest it doesn't.
To make it even more fun, converting verify_iovec() and its compat equivalent
to use of {,compat_}rw_copy_check_uvector() would guarantee that all those
access_ok() are redundant to start with, both on send and receive side of
things. Note, BTW, that for read()/write()/readv()/writev() it's already
redundant. Moreover, we have a very good reason to do that anyway - conversion
of sendmsg/recvmsg to iov_iter primitives would pretty much require that,
since copy_{to,from}_iter() assume that iovec behind the iov_iter has been
validated wrt access_ok().
I do have a patch doing just that; the question is what to do with csum-and-copy
primitives. Originally I planned to simply strip those access_ok() from those
(both the explicit calls and use of copy_from_user() where we ought to use
__copy_from_user(), etc.), but that's not nice to potential out-of-tree callers
of those suckers. If any of those exist and manage to cope with the wonderful
calling conventions, that is. As it is, we have the total of 4 callers of
csum_and_copy_from_user() and 2 callers of csum_and_copy_to_user(), all in
networking code. Do we care about potential out-of-tree users existing and
getting screwed by such change? Davem, Linus?
Alternatively, we could introduce __csum_and_copy_{from,to}_user() that would
_not_ do access_ok() (and wouldn't be required to do zeroing the tail, etc.),
convert the existing callers to that and leave csum_and_copy_{to,from}_user()
as architecture-independent wrappers around those - "do access_ok(),
then try to call __csum_and... variant, then fall back to dumb implementation
if that fails". For almost all architectures csum_and_partial_copy_from_user()
would get stripped of access_ok() and renamed to __csum_and_copy_from_user();
for ppc64 we'd define __csum_and_copy_from_user(src,dst,len,sum,errp) as
csum_and_partial_copy_generic(src,dst,len,sum,errp,NULL) - they already
have that kind of code structure. This variant still buggers the out-of-tree
code using csum_and_partial_copy_from_user() directly, but users of
csum_and_copy_{from,to}_user() are left intact, such modules were already
broken on ppc64 *and* breakage is of obvious "it won't link" kind.
Comments, suggestions?
PS: there are some really amusing brainos - e.g. mn10300 csum_and_copy_to_user()
starts with this:
missing = copy_to_user(dst, src, len);
if (missing) {
memset(dst + len - missing, 0, missing);
*err_ptr = -EFAULT;
}
that's right, if copy_to_user() has returned non-zero, try to do
memset() on userland addresses it has failed to write into...
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 19:40:59
On Tue, Nov 18, 2014 at 08:47:45AM +0000, Al Viro wrote:
I do have a patch doing just that; the question is what to do with csum-and-copy
primitives. Originally I planned to simply strip those access_ok() from those
(both the explicit calls and use of copy_from_user() where we ought to use
__copy_from_user(), etc.), but that's not nice to potential out-of-tree callers
of those suckers. If any of those exist and manage to cope with the wonderful
calling conventions, that is. As it is, we have the total of 4 callers of
csum_and_copy_from_user() and 2 callers of csum_and_copy_to_user(), all in
networking code. Do we care about potential out-of-tree users existing and
getting screwed by such change? Davem, Linus?
FWIW, the beginning of series in question follows; removal of those
access_ok() is 3/5. The series is longer than that (see vfs.git#iov_iter-net
for a bit more, and there's more stuff in local queue still too much in flux
to push them out), but all the stuff relevant to validating iovecs on
sendmsg/recvmsg and getting rid of excessive access_ok() is in the first 5
commits.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 19:41:40
Kernel-side struct msghdr is (currently) using the same layout as
userland one, but it's not a one-to-one copy - even without considering
32bit compat issues, we have msg_iov, msg_name and msg_control copied
to kernel[1]. It's fairly localized, so we get away with a few functions
where that knowledge is needed (and we could shrink that set even
more). Pretty much everything deals with the kernel-side variant and
the few places that want userland one just use a bunch of force-casts
to paper over the differences.
The thing is, kernel-side definition of struct msghdr is *not* exposed
in include/uapi - libc doesn't see it, etc. So we can add struct user_msghdr,
with proper annotations and let the few places that ever deal with those
beasts use it for userland pointers. Saner typechecking aside, that will
allow to change the layout of kernel-side msghdr - e.g. replace
msg_iov/msg_iovlen there with struct iov_iter, getting rid of the need
to modify the iovec as we copy data to/from it, etc.
We could introduce kernel_msghdr instead, but that would create much more
noise - the absolute majority of the instances would need to have the
type switched to kernel_msghdr and definition of struct msghdr in
include/linux/socket.h is not going to be seen by userland anyway.
This commit just introduces user_msghdr and switches the few places that
are dealing with userland-side msghdr to it.
[1] actually, it's even trickier than that - we copy msg_control for
sendmsg, but keep the userland address on recvmsg.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/arm/kernel/sys_oabi-compat.c | 4 ++--
include/linux/socket.h | 16 +++++++++++++---
include/linux/syscalls.h | 6 +++---
net/compat.c | 4 ++--
net/socket.c | 29 ++++++++++++++++-------------
5 files changed, 36 insertions(+), 23 deletions(-)
@@ -53,10 +53,20 @@ struct msghdr {__kernel_size_tmsg_controllen;/* ancillary data buffer length */unsignedintmsg_flags;/* flags on received message */};++structuser_msghdr{+void__user*msg_name;/* ptr to socket address structure */+intmsg_namelen;/* size of socket address structure */+structiovec__user*msg_iov;/* scatter/gather array */+__kernel_size_tmsg_iovlen;/* # elements in msg_iov */+void__user*msg_control;/* ancillary data */+__kernel_size_tmsg_controllen;/* ancillary data buffer length */+unsignedintmsg_flags;/* flags on received message */+};/* For recvmmsg/sendmmsg */structmmsghdr{-structmsghdrmsg_hdr;+structuser_msghdrmsg_hdr;unsignedintmsg_len;};
@@ -319,8 +329,8 @@ extern int put_cmsg(struct msghdr*, int level, int type, int len, void *data);structtimespec;/* The __sys_...msg variants allow MSG_CMSG_COMPAT */-externlong__sys_recvmsg(intfd,structmsghdr__user*msg,unsignedflags);-externlong__sys_sendmsg(intfd,structmsghdr__user*msg,unsignedflags);+externlong__sys_recvmsg(intfd,structuser_msghdr__user*msg,unsignedflags);+externlong__sys_sendmsg(intfd,structuser_msghdr__user*msg,unsignedflags);externint__sys_recvmmsg(intfd,structmmsghdr__user*mmsg,unsignedintvlen,unsignedintflags,structtimespec*timeout);externint__sys_sendmmsg(intfd,structmmsghdr__user*mmsg,
@@ -1989,8 +1989,11 @@ struct used_address {};staticintcopy_msghdr_from_user(structmsghdr*kmsg,-structmsghdr__user*umsg)+structuser_msghdr__user*umsg){+/* We are relying on the (currently) identical layouts. Once+*thekernel-sidechanges,thisplacewillneedtobeupdated+*/if(copy_from_user(kmsg,umsg,sizeof(structmsghdr)))return-EFAULT;
@@ -2005,7 +2008,7 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,return0;}-staticint___sys_sendmsg(structsocket*sock,structmsghdr__user*msg,+staticint___sys_sendmsg(structsocket*sock,structuser_msghdr__user*msg,structmsghdr*msg_sys,unsignedintflags,structused_address*used_address){
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 19:42:12
use {compat_,}rw_copy_check_uvector(). As the result, we are
guaranteed that all iovecs seen in ->msg_iov by ->sendmsg()
and ->recvmsg() will pass access_ok(). The next commit removes
now redundant checks in callees...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/compat.c | 51 +++++++++++++++------------------------------------
net/core/iovec.c | 37 ++++++++++++++-----------------------
net/socket.c | 38 ++++++++------------------------------
3 files changed, 37 insertions(+), 89 deletions(-)
@@ -80,13 +53,15 @@ int get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg)}/* I've named the args so it is easy to tell whose space the pointers are in. */-intverify_compat_iovec(structmsghdr*kern_msg,structiovec*kern_iov,+intverify_compat_iovec(structmsghdr*kern_msg,structiovec*iov,structsockaddr_storage*kern_address,intmode){-inttot_len;+structcompat_iovec__user*p;+structiovec*res;+interr;if(kern_msg->msg_name&&kern_msg->msg_namelen){-if(mode==VERIFY_READ){+if(mode==WRITE){interr=move_addr_to_kernel(kern_msg->msg_name,kern_msg->msg_namelen,kern_address);
@@ -2032,24 +2032,14 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,returnerr;}-if(msg_sys->msg_iovlen>UIO_FASTIOV){-err=-EMSGSIZE;-if(msg_sys->msg_iovlen>UIO_MAXIOV)-gotoout;-err=-ENOMEM;-iov=kmalloc(msg_sys->msg_iovlen*sizeof(structiovec),-GFP_KERNEL);-if(!iov)-gotoout;-}-/* This will also move the address data into kernel space */-if(MSG_CMSG_COMPAT&flags){-err=verify_compat_iovec(msg_sys,iov,&address,VERIFY_READ);-}else-err=verify_iovec(msg_sys,iov,&address,VERIFY_READ);+if(MSG_CMSG_COMPAT&flags)+err=verify_compat_iovec(msg_sys,iovstack,&address,WRITE);+else+err=verify_iovec(msg_sys,iovstack,&address,WRITE);if(err<0)gotoout_freeiov;+iov=msg_sys->msg_iov;total_len=err;err=-ENOBUFS;
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 19:43:25
The following set of functions
skb_add_data_nocache
skb_copy_to_page_nocache
skb_do_copy_data_nocache
skb_copy_to_page
skb_add_data
csum_and_copy_from_user
skb_copy_and_csum_datagram
csum_and_copy_to_user
memcpy_fromiovec
memcpy_toiovec
memcpy_toiovecend
memcpy_fromiovecend
are never given a userland range that would not satisfy access_ok().
Proof:
1) skb_add_data_nocache() and skb_copy_to_page_nocache() are called only
by tcp_sendmsg() and given a range covered by ->msg_iov[].
2) skb_do_copy_data_nocache() is called only by skb_add_data_nocache() and
skb_copy_to_page_nocache() and range comes from their arguments.
3) skb_copy_to_page() is never called at all (dead code since 3.0; killed in
the next commit).
4) skb_add_data() is called by rxrpc_send_data() and tcp_send_syn_data(),
both passing it a range covered by ->msg_iov[].
5) all callers of csum_partial_copy_fromiovecend() are giving it iovecs from
->msg_iov[]. Proof: it is called by ip_generic_getfrag() and ping_getfrag(),
both are called only as callbacks by ip_append_data(), ip6_append_data() and
ip_make_skb() and argument passed to those callbacks in all such invocations
is ->msg_iov.
6) csum_and_copy_from_user() is called by skb_add_data(), skb_copy_to_page(),
skb_do_copy_data_nocache() and csum_partial_copy_fromiovecend(). In all cases
the range is covered by ->msg_iov[]
7) skb_copy_and_csum_datagram_iovec() is always getting an iovec from ->msg_iov.
Proof: it is called by tcp_copy_to_iovec(), which gives it tp->ucopy.iov,
and by several recvmsg instances (udp, raw, raw6) which give it ->msg_iov.
But tp->ucopy.iov is initialized only by ->msg_iov.
8) skb_copy_and_csum_datagram() is called only by itself (for fragments)
and by skb_copy_and_csum_datagram_iovec(). The range is covered by the range
passed to caller (in the first case) or by an iovec passed to
skb_copy_and_csum_datagram_iovec().
9) csum_and_copy_to_user() is called only by skb_copy_and_csum_datagram().
Range is covered by the range given to caller...
10) skb_copy_datagram_iovec() is always getting an iovec that would pass
access_ok() on all elements. Proof: cases when ->msg_iov or ->ucopy.iov are
passed are trivial. Other than those, we have
* ppp_read() (single-element iovec, range passed to ->read() has been
validated by caller)
* skb_copy_and_csum_datagram_iovec() (see (7)), itself (covered by
the ranges in array given to its caller)
* rds_tcp_inc_copy_to_user(), which is called only as
->inc_copy_to_user(), which is always given ->msg_iov.
11) aside of the callers of memcpy_toiovec() that immediately pass it ->msg_iov,
there are 3 call sites: one in __qp_memcpy_from_queue() (when called from
qp_memcpy_from_queue_iov()) and two in skb_copy_datagram_iovec(). The latter
is OK due to (10), the former has the call chain coming through
vmci_qpair_dequev() and vmci_transport_stream_dequeue(), which is called as
->stream_dequeue(), which is always given ->msg_iov.
Types in vmw_vmci blow, film at 11...
12) memcpy_toiovecend() is always given a subset of something we'd just given
to ->recvmsg() in ->msg_iov.
13) most of the memcpy_fromiovec() callers are explicitly passing it ->msg_iov.
There are few exceptions:
* l2cap_skbuff_fromiovec(). Called only as bluetooth
->memcpy_fromiovec(), which always gets ->msg_iov as argument.
* __qp_memcpy_to_queue(), from qp_memcpy_to_queue_iov(), from
vmci_qpair_enquev(), from vmci_transport_stream_enqueue(), which is always
called as ->stream_enqueue(), which always gets ->msg_iov. Don't ask me what
I think of vmware...
* ipxrtr_route_packet(), which is always given ->msg_iov by its caller.
* vmci_transport_dgram_enqueue(), which is always called as
->dgram_enqueue(), which always gets ->msg_iov.
* vhost get_indirect(), which is passing it iovec filled by
translate_desc(). Ranges are subsets of those that had been validated by
vq_memory_access_ok() back when we did vhost_set_memory().
14) zerocopy_sg_from_iovec() always gets a validated iovec.
Proof: callers are {tun,macvtap}_get_user(), which are called either from
->aio_write() (and given iovec validated by caller of ->aio_write()), or
from ->sendmsg() (and given ->msg_iov).
15) skb_copy_datagram_from_iovec() always gets an validated iovec.
Proof: for callers in macvtap and tun, same as in (14). Ones in net/unix
and net/packet are given ->msg_iov. Other than those, there's one
in zerocopy_sg_from_iovec() (see (14)) and one in skb_copy_datagram_from_iovec()
itself (fragments handling) - that one gets the iovec its caller was given.
16) callers of memcpy_fromiovecend() are
* {macvtap,tun}_get_user(). Same as (14).
* skb_copy_datagram_from_iovec(). See (15).
* raw_send_hdrinc(), raw6_send_hdrinv(). Both get ->msg_iov as argument
from their callers and pass it to memcpy_fromiovecend().
* sctp_user_addto_chunk(). Ditto.
* tipc_msg_build(). Again, iovec argument is always ->msg_iov.
* ip_generic_getfrag() and udplite_getfrag(). Same as (5).
* vhost_scsi_handle_vq() - that one gets vq->iov as iovec, and
vq->iov is filled ultimately by translate_desc(). Validated by
vq_memory_access_ok() back when we did vhost_set_memory().
And anything that might end up in ->msg_iov[] has to pass access_ok(). It's
trivial
for kernel_{send,recv}msg() users (there we are under set_fs(KERNEL_DS)), it's
verified by rw_copy_check_uvector() in sendmsg()/recvmsg()/sendmmsg()/recvmmsg()
and in the only place where we call ->sendmsg()/->recvmsg() not via net/socket.c
helpers (drivers/vhost/net.c) they are getting vq->iov. As mentioned above,
this one is guaranteed to pass the checks since it's filled by translate_desc(),
and ranges it fills are subsets of ranges that had been validated when we
did vhost_set_memory().
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/alpha/lib/csum_partial_copy.c | 5 ----
arch/frv/lib/checksum.c | 2 +-
arch/m32r/lib/csum_partial_copy.c | 2 +-
arch/mips/include/asm/checksum.h | 30 +++++++--------------
arch/mn10300/lib/checksum.c | 4 +--
arch/parisc/include/asm/checksum.h | 2 +-
arch/parisc/lib/checksum.c | 2 +-
arch/powerpc/lib/checksum_wrappers_64.c | 6 ++---
arch/s390/include/asm/checksum.h | 2 +-
arch/score/include/asm/checksum.h | 2 +-
arch/score/lib/checksum_copy.c | 2 +-
arch/sh/include/asm/checksum_32.h | 8 +-----
arch/sparc/include/asm/checksum_32.h | 43 ++++++++++++++-----------------
arch/x86/include/asm/checksum_32.h | 17 ++++--------
arch/x86/lib/csum-wrappers_64.c | 3 ---
arch/x86/um/asm/checksum.h | 2 +-
arch/x86/um/asm/checksum_32.h | 12 ++++-----
arch/xtensa/include/asm/checksum.h | 8 +-----
include/linux/skbuff.h | 2 +-
include/net/checksum.h | 14 +++-------
include/net/sock.h | 7 +++--
lib/iovec.c | 8 +++---
net/core/iovec.c | 6 ++---
23 files changed, 67 insertions(+), 122 deletions(-)
@@ -97,7 +97,7 @@ int csum_partial_copy_fromiovecend(unsigned char *kdata, struct iovec *iov,/* iov component is too short ... */if(par_len>copy){-if(copy_from_user(kdata,base,copy))+if(__copy_from_user(kdata,base,copy))gotoout_fault;kdata+=copy;base+=copy;
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 19:43:51
... and do the same on the compat side of things.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
include/linux/socket.h | 1 -
include/net/compat.h | 5 ++-
net/compat.c | 52 ++++++++++++---------------
net/core/iovec.c | 38 --------------------
net/socket.c | 93 +++++++++++++++++++++++++++---------------------
5 files changed, 77 insertions(+), 112 deletions(-)
@@ -46,44 +50,32 @@ int get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg)return-EFAULT;if(kmsg->msg_namelen>sizeof(structsockaddr_storage))kmsg->msg_namelen=sizeof(structsockaddr_storage);-kmsg->msg_name=compat_ptr(tmp1);-kmsg->msg_iov=compat_ptr(tmp2);kmsg->msg_control=compat_ptr(tmp3);-return0;-}-/* I've named the args so it is easy to tell whose space the pointers are in. */-intverify_compat_iovec(structmsghdr*kern_msg,structiovec*iov,-structsockaddr_storage*kern_address,intmode)-{-structcompat_iovec__user*p;-structiovec*res;-interr;+if(save_addr)+*save_addr=compat_ptr(uaddr);-if(kern_msg->msg_name&&kern_msg->msg_namelen){-if(mode==WRITE){-interr=move_addr_to_kernel(kern_msg->msg_name,-kern_msg->msg_namelen,-kern_address);+if(uaddr&&kmsg->msg_namelen){+if(!save_addr){+err=move_addr_to_kernel(compat_ptr(uaddr),+kmsg->msg_namelen,+kmsg->msg_name);if(err<0)returnerr;}-kern_msg->msg_name=kern_address;}else{-kern_msg->msg_name=NULL;-kern_msg->msg_namelen=0;+kmsg->msg_name=NULL;+kmsg->msg_namelen=0;}-if(kern_msg->msg_iovlen>UIO_MAXIOV)+if(kmsg->msg_iovlen>UIO_MAXIOV)return-EMSGSIZE;-p=(structcompat_iovec__user*)kern_msg->msg_iov;-err=compat_rw_copy_check_uvector(mode,p,kern_msg->msg_iovlen,-UIO_FASTIOV,iov,&res);+err=compat_rw_copy_check_uvector(save_addr?READ:WRITE,+compat_ptr(uiov),kmsg->msg_iovlen,+UIO_FASTIOV,*iov,iov);if(err>=0)-kern_msg->msg_iov=res;-elseif(res!=iov)-kfree(res);+kmsg->msg_iov=*iov;returnerr;}
@@ -1988,16 +1988,26 @@ struct used_address {unsignedintname_len;};-staticintcopy_msghdr_from_user(structmsghdr*kmsg,-structuser_msghdr__user*umsg)+staticssize_tcopy_msghdr_from_user(structmsghdr*kmsg,+structuser_msghdr__user*umsg,+structsockaddr__user**save_addr,+structiovec**iov){-/* We are relying on the (currently) identical layouts. Once-*thekernel-sidechanges,thisplacewillneedtobeupdated-*/-if(copy_from_user(kmsg,umsg,sizeof(structmsghdr)))+structsockaddr__user*uaddr;+structiovec__user*uiov;+ssize_terr;++if(!access_ok(VERIFY_READ,umsg,sizeof(*umsg))||+__get_user(uaddr,&umsg->msg_name)||+__get_user(kmsg->msg_namelen,&umsg->msg_namelen)||+__get_user(uiov,&umsg->msg_iov)||+__get_user(kmsg->msg_iovlen,&umsg->msg_iovlen)||+__get_user(kmsg->msg_control,&umsg->msg_control)||+__get_user(kmsg->msg_controllen,&umsg->msg_controllen)||+__get_user(kmsg->msg_flags,&umsg->msg_flags))return-EFAULT;-if(kmsg->msg_name==NULL)+if(!uaddr)kmsg->msg_namelen=0;if(kmsg->msg_namelen<0)
@@ -2005,7 +2015,31 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,if(kmsg->msg_namelen>sizeof(structsockaddr_storage))kmsg->msg_namelen=sizeof(structsockaddr_storage);-return0;++if(save_addr)+*save_addr=uaddr;++if(uaddr&&kmsg->msg_namelen){+if(!save_addr){+err=move_addr_to_kernel(uaddr,kmsg->msg_namelen,+kmsg->msg_name);+if(err<0)+returnerr;+}+}else{+kmsg->msg_name=NULL;+kmsg->msg_namelen=0;+}++if(kmsg->msg_iovlen>UIO_MAXIOV)+return-EMSGSIZE;++err=rw_copy_check_uvector(save_addr?READ:WRITE,+uiov,kmsg->msg_iovlen,+UIO_FASTIOV,*iov,iov);+if(err>=0)+kmsg->msg_iov=*iov;+returnerr;}staticint___sys_sendmsg(structsocket*sock,structuser_msghdr__user*msg,
@@ -2020,26 +2054,17 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,__attribute__((aligned(sizeof(__kernel_size_t))));/* 20 is size of ipv6_pktinfo */unsignedchar*ctl_buf=ctl;-interr,ctl_len,total_len;+intctl_len,total_len;+ssize_terr;-err=-EFAULT;-if(MSG_CMSG_COMPAT&flags){-if(get_compat_msghdr(msg_sys,msg_compat))-return-EFAULT;-}else{-err=copy_msghdr_from_user(msg_sys,msg);-if(err)-returnerr;-}+msg_sys->msg_name=&address;-/* This will also move the address data into kernel space */if(MSG_CMSG_COMPAT&flags)-err=verify_compat_iovec(msg_sys,iovstack,&address,WRITE);+err=get_compat_msghdr(msg_sys,msg_compat,NULL,&iov);else-err=verify_iovec(msg_sys,iovstack,&address,WRITE);+err=copy_msghdr_from_user(msg_sys,msg,NULL,&iov);if(err<0)gotoout_freeiov;-iov=msg_sys->msg_iov;total_len=err;err=-ENOBUFS;
@@ -2215,36 +2240,24 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,structioveciovstack[UIO_FASTIOV];structiovec*iov=iovstack;unsignedlongcmsg_ptr;-interr,total_len,len;+inttotal_len,len;+ssize_terr;/* kernel mode address */structsockaddr_storageaddr;/* user mode address pointers */structsockaddr__user*uaddr;-int__user*uaddr_len;+int__user*uaddr_len=COMPAT_NAMELEN(msg);-if(MSG_CMSG_COMPAT&flags){-if(get_compat_msghdr(msg_sys,msg_compat))-return-EFAULT;-}else{-err=copy_msghdr_from_user(msg_sys,msg);-if(err)-returnerr;-}+msg_sys->msg_name=&addr;-/* Save the user-mode address (verify_iovec will change the-*kernelmsghdrtousethekerneladdressspace)-*/-uaddr=(__forcevoid__user*)msg_sys->msg_name;-uaddr_len=COMPAT_NAMELEN(msg);if(MSG_CMSG_COMPAT&flags)-err=verify_compat_iovec(msg_sys,iovstack,&addr,READ);+err=get_compat_msghdr(msg_sys,msg_compat,&uaddr,&iov);else-err=verify_iovec(msg_sys,iovstack,&addr,READ);+err=copy_msghdr_from_user(msg_sys,msg,&uaddr,&iov);if(err<0)gotoout_freeiov;-iov=msg_sys->msg_iov;total_len=err;cmsg_ptr=(unsignedlong)msg_sys->msg_control;
On Tue, Nov 18, 2014 at 12:47 AM, Al Viro [off-list ref] wrote:
The minimal implementations would be
__wsum csum_and_copy_from_user(const void __user *src, void *dst, int len,
__wsum sum, int *err_ptr)
{
if (unlikely(copy_from_user(dst, src, len) < 0)) {
No. That "< 0" should be "!= 0". The user copy functions return a
positive value of how many bytes they *failed* to copy.
Note that we are *not* guaranteed that *err_ptr is touched in case of success
- not all architectures zero it in that case. Callers must (and do) zero it
before the call of csum_and_copy_{from,to}_user(). Furthermore, return value
in case of error is undefined.
Yeah, easy to misuse.
IMO the calling conventions are atrocious.
Yeah, not pretty. At the same time, the pain of changing what seems to
work might not be worth it.
And quite frankly, I *detest* your patch 3/5.
"access_ok()" isn't that expensive, and removing them as unnecessary
is fraught with errors. We've had several cases of "oops, we used
__get_user() in a loop, because it generates much better code, but
we'd forgotten to do access_ok(), so now people can read kernel data".
I really think that
(a) using "__get_user/__put_user/__memcpy_from/to_user" should be
avoided unless there is a clear and present performance issue
(b) when that performance issue is clear and unmistakable, you should
generally have the "access_ok()" check *locally* to the use of unsafe
ops, so that you can *locally* see that it's safe.
(c) if there is some upper-level check (ie the normal read/write
paths that make *sure* the addresses are fine), and there is some huge
performance issue that means that the local checks would be a problem,
then we should have a big comment about exactly where the checks are
done.
Your 3/5 violates pretty much all of these, imho.
The rest of the patches I have nothing against. I'm a bit worried that
this is stuff that can easily get stupid thinkos (ie exactly due to
things like the argument order thing), and I wonder how much it buys
us, but at least it seems to generally remove more lines than it adds,
and cleans some stuff up, so I'm not against it.
Linus
Linus
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-18 21:23:10
On Tue, Nov 18, 2014 at 12:49:13PM -0800, Linus Torvalds wrote:
On Tue, Nov 18, 2014 at 12:47 AM, Al Viro [off-list ref] wrote:
quoted
The minimal implementations would be
__wsum csum_and_copy_from_user(const void __user *src, void *dst, int len,
__wsum sum, int *err_ptr)
{
if (unlikely(copy_from_user(dst, src, len) < 0)) {
No. That "< 0" should be "!= 0". The user copy functions return a
positive value of how many bytes they *failed* to copy.
D'oh... Yes, indeed - sorry about the braino.
quoted
IMO the calling conventions are atrocious.
Yeah, not pretty. At the same time, the pain of changing what seems to
work might not be worth it.
And quite frankly, I *detest* your patch 3/5.
"access_ok()" isn't that expensive, and removing them as unnecessary
is fraught with errors. We've had several cases of "oops, we used
__get_user() in a loop, because it generates much better code, but
we'd forgotten to do access_ok(), so now people can read kernel data".
OK... If netdev folks can live with that for now, I've no problem with
dropping 3/5. However, I really think we need a variant of csum-and-copy
that would _not_ bother with access_ok() longer term. That can wait, though...
On Tue, Nov 18, 2014 at 1:23 PM, Al Viro [off-list ref] wrote:
OK... If netdev folks can live with that for now, I've no problem with
dropping 3/5. However, I really think we need a variant of csum-and-copy
that would _not_ bother with access_ok() longer term. That can wait, though...
iirc,access_ok() ends up being something like three or four
instructions. It's generally not very painful.
The main reason to ever use the "__" functions is for __get_user() or
__put_user() in a loop (or when doing multiple ones when
loading/storing a struct or a signal stack frame or whatever), and
then the real issue ends up being that the __get_user() is really
often just a single instruction, while "get_user()" is a function call
that does that access_ok().
So then moving the access_ok() outside the loop, or to above the
structure load, can make a *big* deal. But removing the access_ok()
entirely tends to not be a huge issue - it's just that you want to do
it *once*, instead of doing it over-and-over.
There might be some case you really want to remove it from the
function entirely so that the "access_ok()" is no longer close to the
accesses it checks, but I really think you want to have a goof
performance case for it, and a comment about it.
And no, we haven't really always followed those rules.
Btw, these days, on x86, we actually have a bigger issue with the
whole STAC/CLAC thing: it's a good safety measure, but doing
STAC/CLAC for each access is painful. So "__get_user()" and
"__put_user()" sadly aren't the single-instruction things they used to
be any more. The code sequences that really want tight code (think the
'struct stat' copying and the like) sadly cannot get it as it is..
Linus
From: David Miller <davem@davemloft.net> Date: 2014-11-19 20:25:59
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 18 Nov 2014 19:40:53 +0000
On Tue, Nov 18, 2014 at 08:47:45AM +0000, Al Viro wrote:
quoted
I do have a patch doing just that; the question is what to do with csum-and-copy
primitives. Originally I planned to simply strip those access_ok() from those
(both the explicit calls and use of copy_from_user() where we ought to use
__copy_from_user(), etc.), but that's not nice to potential out-of-tree callers
of those suckers. If any of those exist and manage to cope with the wonderful
calling conventions, that is. As it is, we have the total of 4 callers of
csum_and_copy_from_user() and 2 callers of csum_and_copy_to_user(), all in
networking code. Do we care about potential out-of-tree users existing and
getting screwed by such change? Davem, Linus?
FWIW, the beginning of series in question follows; removal of those
access_ok() is 3/5. The series is longer than that (see vfs.git#iov_iter-net
for a bit more, and there's more stuff in local queue still too much in flux
to push them out), but all the stuff relevant to validating iovecs on
sendmsg/recvmsg and getting rid of excessive access_ok() is in the first 5
commits.
Al I really like this series, especially patch #2.
Sorry for taking so long to review this, I just wanted to make sure we
got this right.
Can you give me a pull request for just these 5 patches? Then feel free
to post the next batch for review, I'm eager to see it as are others.
Thanks!
From: David Miller <davem@davemloft.net> Date: 2014-11-19 20:31:42
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 18 Nov 2014 21:23:07 +0000
On Tue, Nov 18, 2014 at 12:49:13PM -0800, Linus Torvalds wrote:
quoted
"access_ok()" isn't that expensive, and removing them as unnecessary
is fraught with errors. We've had several cases of "oops, we used
__get_user() in a loop, because it generates much better code, but
we'd forgotten to do access_ok(), so now people can read kernel data".
OK... If netdev folks can live with that for now, I've no problem with
dropping 3/5. However, I really think we need a variant of csum-and-copy
that would _not_ bother with access_ok() longer term. That can wait, though...
I think because of the way Al verifies things at the top level, and
how we structure access to these msg->msg_iov so strictly, these cases
of access_ok() really can safely go.
But that is just my opinion, and yes I do acknowledge that we've had
serious holes in this area in the past.
On Wed, Nov 19, 2014 at 12:31 PM, David Miller [off-list ref] wrote:
But that is just my opinion, and yes I do acknowledge that we've had
serious holes in this area in the past.
The serious holes have generally been exactly in the "upper layers
already check" camp, and then it turns out that some odd ioctl or
other thing ends up doing something odd and interesting.
If Al has actual performance profiles showing that the access_ok() is
a real problem, then fine. As a low-level optimization, I agree with
it. But not as a "let's just drop them, and make the security rules be
non-local and subtle, and require people to know the details of the
whole call-chain".
Seeing a "__get_user()" and just being able to glance up in the same
function and seeing the "access_ok()" is just a good safety net. And
means that people don't have to waste time thinking about or looking
for where the hell the security net really is.
Linus
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-19 21:17:19
On Wed, Nov 19, 2014 at 12:40:53PM -0800, Linus Torvalds wrote:
On Wed, Nov 19, 2014 at 12:31 PM, David Miller [off-list ref] wrote:
quoted
But that is just my opinion, and yes I do acknowledge that we've had
serious holes in this area in the past.
The serious holes have generally been exactly in the "upper layers
already check" camp, and then it turns out that some odd ioctl or
other thing ends up doing something odd and interesting.
If Al has actual performance profiles showing that the access_ok() is
a real problem, then fine. As a low-level optimization, I agree with
it. But not as a "let's just drop them, and make the security rules be
non-local and subtle, and require people to know the details of the
whole call-chain".
Seeing a "__get_user()" and just being able to glance up in the same
function and seeing the "access_ok()" is just a good safety net. And
means that people don't have to waste time thinking about or looking
for where the hell the security net really is.
Umm... It's not quite that bad - the thing is, for iov_iter-net series
we'll need copy_and_csum_{to,from}_iter() anyway and for iovec-backed
iov_iter instances we already ask the iovec to be validated wrt access_ok().
And this validation (already done by rw_copy_check_uvector()) is going to
be next to iov_iter_init() setting the iov_iter up.
Moreover, I'm planning to take iov_iter_init() into rw_copy_check_uvector().
That way setting ->iov is done from the same function that has just checked
all ranges. If you look at the callers, you'll see that almost all of them
are directly followed by iov_iter_init() and folding it in is a fairly
obvious cleanup.
The thing is, with ..._iter() variants added we are left with no other
in-tree callers of csum_and_copy_{to,from}_user(). I agree that dropping
those access_ok() is too early at this point - the analysis of paths
by which a range can reach them is scary right now. Moreover, it mostly
parallels the changes later in the series - ones that propagate a pointer
to iov_iter put into msdghdr in place of ->msg_iov/->msg_iovlen down to
the new primitives. _After_ those steps the analysis (see the horrors in
commit message of 3/5) becomes trivial.
So whether we end up removing those access_ok() or not, this is not the
time to do so. It still might make sense in the end, but not right now.
Frankly, my preference would be to provide __csum_and_copy_...() that do
not bother with access_ok() (and do so consistently between the architectures),
and do not bother with zeroing or trying to do an accurate csum in case of
error. With uniform implementation of csum_and_copy_...() that does
access_ok(), tries to call __csum_... variant and, in case of failure,
does __copy_..._user(), zeroes the tail in the "from" one and calculates
the csum by source of destination - whichever's kernel-side. I.e. do what
ppc64 is doing. That way we get obviously safe csum_and_copy_.._user(),
and consistent __ counterparts directly used by ..._iter() primitives.
Quite a bit of complexity becomes possible to remove from asm code,
while we are at it - zeroing isn't the worst of it, contortions needed to
calculate the csum accurately in error case are often nastier. So much
that e.g. arm doesn't even bother trying.
Again, this is a separate work - I agree with you regarding the overhead
being a non-issue for existing callers, and if somebody tries e.g. to send
64K from 32K-element vector of 2-byte ranges they'll have a _lot_ of other
overhead that will drown that of those access_ok(). In normal cases the
price of copying the data itself is going to swamp that of access_ok(),
of course.
IOW, consider the access_ok() changes withdrawn for now. It's too early
in the series for them and the only reason to pull them that high in
ordering had been the fear of overhead. Which is very unlikely to be
an issue. They won't come back (if they come back at all) until the
proof of correctness becomes absolutely trivial.
On Wed, Nov 19, 2014 at 12:31 PM, David Miller [off-list ref] wrote:
quoted
But that is just my opinion, and yes I do acknowledge that we've had
serious holes in this area in the past.
The serious holes have generally been exactly in the "upper layers
already check" camp, and then it turns out that some odd ioctl or
other thing ends up doing something odd and interesting.
If Al has actual performance profiles showing that the access_ok() is
a real problem, then fine. As a low-level optimization, I agree with
it. But not as a "let's just drop them, and make the security rules be
non-local and subtle, and require people to know the details of the
whole call-chain".
Seeing a "__get_user()" and just being able to glance up in the same
function and seeing the "access_ok()" is just a good safety net. And
means that people don't have to waste time thinking about or looking
for where the hell the security net really is.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-19 21:30:12
On Wed, Nov 19, 2014 at 04:17:44PM -0500, David Miller wrote:
quoted
Seeing a "__get_user()" and just being able to glance up in the same
function and seeing the "access_ok()" is just a good safety net. And
means that people don't have to waste time thinking about or looking
for where the hell the security net really is.
Fair enough.
OK, with 3/5 dropped 4/5 get a trivial conflict (removal of function in 4/5
vs. change in it in 3/5). With that dealt with, the sucker is in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem
Shortlog:
Al Viro (4):
separate kernel- and userland-side msghdr
{compat_,}verify_iovec(): switch to generic copying of iovecs
fold verify_iovec() into copy_msghdr_from_user()
bury skb_copy_to_page()
Diffstat:
arch/arm/kernel/sys_oabi-compat.c | 4 +--
include/linux/socket.h | 17 +++++++---
include/linux/syscalls.h | 6 ++--
include/net/compat.h | 5 ++-
include/net/sock.h | 23 -------------
net/compat.c | 83 +++++++++++++++-------------------------------
net/core/iovec.c | 47 --------------------------
net/socket.c | 140 +++++++++++++++++++++++++++++++++++++-----------------------------------------
8 files changed, 114 insertions(+), 211 deletions(-)
I'll post more for review after I finally get some sleep - up for bloody 27
hours by now ;-/
From: David Miller <davem@davemloft.net> Date: 2014-11-19 21:53:48
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Wed, 19 Nov 2014 21:30:07 +0000
On Wed, Nov 19, 2014 at 04:17:44PM -0500, David Miller wrote:
quoted
quoted
Seeing a "__get_user()" and just being able to glance up in the same
function and seeing the "access_ok()" is just a good safety net. And
means that people don't have to waste time thinking about or looking
for where the hell the security net really is.
Fair enough.
OK, with 3/5 dropped 4/5 get a trivial conflict (removal of function in 4/5
vs. change in it in 3/5). With that dealt with, the sucker is in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem
Shortlog:
Al Viro (4):
separate kernel- and userland-side msghdr
{compat_,}verify_iovec(): switch to generic copying of iovecs
fold verify_iovec() into copy_msghdr_from_user()
bury skb_copy_to_page()
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-20 21:48:00
On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
Pulled, thanks Al.
Umm... Not in net-next.git#master... Anyway, the next portion is in
vfs.git#iov_iter-net right now; I'll post it on netdev once I get some
sleep.
It's getting close to really interesting parts. Right now the main obstacle
is in iscsit_do_rx_data/iscsit_do_tx_data; what happens there is reuse of
iovec if kernel_sendmsg() gives a short write - it tries to send again, with
the same iovec and decremented length. Ditto on RX side (with kernel_recvmsg(),
obviously).
As far as I can see, these retries on the send side are simply broken -
normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
modify iovec in normal case. IOW, if you get 8K sent out of 80K, the next
time it'll try to send 72K - already sent piece + 64K following it, etc.
Could target-devel folks tell how realistic those resends are, in the
first place? Both with TX and RX sides... Is there any sane limit on
iovec size there, etc.
Note that while conversion to iov_iter will provide a very simple solution
(iovec remains unchanged, iterator advances and we just need to avoid
reinitializing it for subsequent iterations in those loops), it won't solve
the problem in older kernels; that code had been there since 2011 and
iov_iter conversion is far too invasive for -stable.
From: Eric Dumazet <hidden> Date: 2014-11-20 21:55:48
On Thu, 2014-11-20 at 21:47 +0000, Al Viro wrote:
As far as I can see, these retries on the send side are simply broken -
normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
modify iovec in normal case.
Arg... I sent this morning something doing this (against net-next tree)
Is it a problem ?
Or can we consider FASTOPEN being not normal case ? ;)
https://patchwork.ozlabs.org/patch/412776/
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-20 22:25:12
On Thu, Nov 20, 2014 at 01:55:42PM -0800, Eric Dumazet wrote:
On Thu, 2014-11-20 at 21:47 +0000, Al Viro wrote:
quoted
As far as I can see, these retries on the send side are simply broken -
normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
modify iovec in normal case.
Arg... I sent this morning something doing this (against net-next tree)
Is it a problem ?
Yes, it is. You are breaking several _other_ kernel_sendmsg() users.
They are already slightly broken, but that'll make breakage much more
common.
Please, don't - the right thing to do is to have iov_iter in msghdr
(we already have the kernel and userland ones with different types and
we do not assume their layouts to be identical - currently they are,
but it's easy to change), keep iovec constant in all cases and advance
->msg_iter. Also in all cases.
Note that direct manipulations of what's currently in ->msg_iov are
wrong - all those loops over vector elements, etc., belong in low-level
primitives. The main missing ones right now are csum_and_copy_{from,to}_iter()
- I have those in local queue, but I'm still trying to get a reasonably
clean mm/iov_iter.c without ridiculous amounts of boilerplating. A bit more
massage is needed there...
Seriously, take a look at vfs.git#iov_iter-net; it's preparations for the
one that'll introduce ->msg_iter. Right now that branch has local iov_iter
declared and initialized in several ->sendmsg() and ->recvmsg() instances and
fed to primitives that work with it; after the conversion it'll be in
msg->msg_iter and it will be initialized by sock_sendmsg()/sock_recvmsg().
The tricky part is how to get through that without temporary breaking the
existing sendmsg/recvmsg users in the kernel *and* without a patch size from
hell. I more or less see how to carve the remaining steps into
reasonably-sized chunks; iscsi is one of the tricky ones and it, AFAICS,
is genuinely broken in mainline and will need fixes that can go into -stable.
And no, your solution doesn't work. Sorry. You'll break e.g. smb_send_kvec()
that way. ceph_tcp_sendmsg() as well, IIRC.
From: Eric Dumazet <hidden> Date: 2014-11-20 22:54:00
On Thu, 2014-11-20 at 22:25 +0000, Al Viro wrote:
Yes, it is. You are breaking several _other_ kernel_sendmsg() users.
They are already slightly broken, but that'll make breakage much more
common.
Please, don't - the right thing to do is to have iov_iter in msghdr
(we already have the kernel and userland ones with different types and
we do not assume their layouts to be identical - currently they are,
but it's easy to change), keep iovec constant in all cases and advance
->msg_iter. Also in all cases.
Note that direct manipulations of what's currently in ->msg_iov are
wrong - all those loops over vector elements, etc., belong in low-level
primitives. The main missing ones right now are csum_and_copy_{from,to}_iter()
- I have those in local queue, but I'm still trying to get a reasonably
clean mm/iov_iter.c without ridiculous amounts of boilerplating. A bit more
massage is needed there...
Seriously, take a look at vfs.git#iov_iter-net; it's preparations for the
one that'll introduce ->msg_iter. Right now that branch has local iov_iter
declared and initialized in several ->sendmsg() and ->recvmsg() instances and
fed to primitives that work with it; after the conversion it'll be in
msg->msg_iter and it will be initialized by sock_sendmsg()/sock_recvmsg().
The tricky part is how to get through that without temporary breaking the
existing sendmsg/recvmsg users in the kernel *and* without a patch size from
hell. I more or less see how to carve the remaining steps into
reasonably-sized chunks; iscsi is one of the tricky ones and it, AFAICS,
is genuinely broken in mainline and will need fixes that can go into -stable.
And no, your solution doesn't work. Sorry. You'll break e.g. smb_send_kvec()
that way. ceph_tcp_sendmsg() as well, IIRC.
Nowhere in tcp_sendmsg() the iov had const qualifier.
If it was declared as const, this discussion would not happen,
we would know we are not allowed to modify it.
iov_iter is nice, but not a single time it is used in net/
Please make sure to add const where appropriate.
Thanks.
From: David Miller <davem@davemloft.net> Date: 2014-11-20 23:23:44
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Thu, 20 Nov 2014 21:47:53 +0000
On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
quoted
Pulled, thanks Al.
Umm... Not in net-next.git#master...
Sorry, I may have done my usual: pull at the office compute, fire up
build testing, go home without pushing back out to kernel.org
:-/
I'll check first thing tomorrow morning.
From: Nicholas A. Bellinger <hidden> Date: 2014-11-21 04:18:28
Hi Al & Co,
On Thu, 2014-11-20 at 21:47 +0000, Al Viro wrote:
On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
quoted
Pulled, thanks Al.
Umm... Not in net-next.git#master... Anyway, the next portion is in
vfs.git#iov_iter-net right now; I'll post it on netdev once I get some
sleep.
Thanks for your detailed analysis + work on this.
It's getting close to really interesting parts. Right now the main obstacle
is in iscsit_do_rx_data/iscsit_do_tx_data; what happens there is reuse of
iovec if kernel_sendmsg() gives a short write - it tries to send again, with
the same iovec and decremented length. Ditto on RX side (with kernel_recvmsg(),
obviously).
As far as I can see, these retries on the send side are simply broken -
normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
modify iovec in normal case. IOW, if you get 8K sent out of 80K, the next
time it'll try to send 72K - already sent piece + 64K following it, etc.
AFAIK, short writes have not been actively getting triggered.
This is likely due to iscsit_do_tx_data() being used for sending 48 byte
PDU header, and small payloads in ISCSI_OP_LOGIN_RSP, ISCSI_OP_TEXT_RSP,
and ISCSI_OP_NOOP_IN control PDUs.
All bulk data READ payloads are sent via iscsit_fe_sendpage_sg() and
only use iscsit_do_tx_data() for leading PDU header.
On the receive side, kernel_recvmsg() is called with MSG_WAITALL that
has been masking this bug..
Could target-devel folks tell how realistic those resends are, in the
first place? Both with TX and RX sides... Is there any sane limit on
iovec size there, etc.
Of the three control type PDU using this codepath, the transfer lengths
are currently limited to <= 32K + header across 2 kvecs. The simplest
fix would probably be to fail the connection when send/recv returns a
value other than requested transfer length for these special cases.
For correctly handling short writes with your new work, what's the
preferred way to do this..?
--nab
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-21 08:50:04
On Thu, Nov 20, 2014 at 02:53:55PM -0800, Eric Dumazet wrote:
quoted
And no, your solution doesn't work. Sorry. You'll break e.g. smb_send_kvec()
that way. ceph_tcp_sendmsg() as well, IIRC.
Nowhere in tcp_sendmsg() the iov had const qualifier.
*nod*
If it was declared as const, this discussion would not happen,
we would know we are not allowed to modify it.
iov_iter is nice, but not a single time it is used in net/
Please make sure to add const where appropriate.
The situation is a fairly long-standing mess. Take a look at e.g.
do_sock_write() - what we have there is
static ssize_t do_sock_write(struct msghdr *msg, struct kiocb *iocb,
struct file *file, const struct iovec *iov,
unsigned long nr_segs)
...
msg->msg_iov = (struct iovec *)iov;
...
return __sock_sendmsg(iocb, sock, msg, size);
That's where const is stripped off. The reason why memcpy_fromiovec()
is modifying iovec is that it we obviously want subsequent calls to keep
picking new and new parts. And on sendmsg(2) path iovec is discarded
after the method call anyway, so we get away with "let's do whatever's
more convenient internally, caller won't care anyway".
Unfortunately, it means that other callers (kernel_sendmsg() users, that is)
end up with very unpleasant situation - they can't even predict if iovec
will be fully drained, partially drained or left completely unchanged.
It's protocol-dependent *and* it depends on the codepath taken in ->sendmsg().
Note that "partially drained" is also a real-world case - e.g.
ping_v4_sendmsg() ends up draining the first sizeof(struct icmphdr) and leaves
the rest there.
In some cases it doesn't hurt much - throwaway struct iovec or a short array
of such which is fed to sendmsg and immediately discarded. The rest either
relies on assumption about the behaviour for (known) protocol, which end
up being incorrect more often than not, or grumbles, makes a throwaway copy
of its iovec array and after the call either drains the original itself or
advances an iov_iter pointing to it. There's quite a collection of unhappy
comments in those callers about this situation. On the recvmsg side the
picture is the same.
We would be better off with iov_iter passed to __sock_{send,recv}msg() (as
a part of struct msghdr, instead of ->msg_iov/->msg_iovlen) and always
advanced to match the amount of data actually picked from it. With iovec
behind it remaining constant. That would work just as well as the current
variant for sendmsg(2)/recvmsg(2)/etc., be a lot more convenient for
kernel_{send,recv}msg() callers and would allow a lot of other fun stuff.
The problem, of course, is how to get through the conversion without a huge
patch from hell. I'm reasonably certain that we can do that on the method
side of things - right now the amount of places aware of ->msg_iov in that
branch is much lower than in mainline. The fun part is keeping the users
of kernel_{send,recv}msg() from breaking while we do the rest of transition.
The ones that use throwaway iovecs are fine - they don't assume any particular
behaviour wrt iovec draining. A bunch of those will be able to use new
warranties later on, but those are separate patches that should go after the
switch to new rules. Besides those we have the following:
iscsit_do_tx_data(). Sends over TCP, assumes iovec drained.
iscsit_do_rx_data(). Recieves over TCP, assumes iovec drained.
smb_send_kvec(). Sends, assumes iovec unchanged.
rxrpc_reject_packets(). Sends, assumes iovec unchanged.
ceph_tcp_sendmsg(). Sends, assumes iovec unchanged.
The last three are not a problem, obviously - they are not quite correct
right now, but with those changes we get no regressions and the closer
we are to complete conversion, the better off they are. Which leaves
us with iscsit_do_[rt]x_data().
For tcp_sendmsg() conversion we need skb_add_data_nocache() and
skb_copy_to_page_nocache() variants that would take iov_iter as data
source. Then the loop over iovec members and while (seglen > 0) loop
inside it get conflated (with iov_iter_count() instead of seglen).
Another thing is tcp_sendmsg_fastopen() and tcp_send_rcvq(). The latter
should just use copy_from_iter() instead of memcpy_from_iovec(), the former
is dealt with by making tcp_send_syn_data() use the same copy_from_iter()
instead of memcpy_from_iovecend().
All of that depends on ->msg_iter being already introduced and it doesn't break
iscsi_do_tx_data() any worse than it's currently broken. Moreover, right after
it we'll be able to fix iscsi_do_tx_data() for good, simply by stopping to mess
with ->msg_iter after the first pass through the loop in there.
skb_add_data_nocache() and skb_copy_to_page_nocache() are both wrappers for
skb_do_copy_data_nocache(), which is used only by those two *and* they are
used only by tcp_sendmsg(). So there's no other code to be disrupted by
changes in those. What skb_do_copy_data_nocache() is doing is a mix of
copy_from_user() (trivial - we just use copy_from_iter() instead),
csum_and_copy_from_user() (new primitive - csum_and_copy_from_iter()) and
__copy_from_user_nocache() (also a new primitive, easily added, but we
are really getting to the point where the amount of boilerplate in iov_iter.c
becomes painful). Incidentally, xip_file_write() could benefit from the last
one as well, giving us full ->write_iter() for those...
As for the recvmsg() part of story, we'll need a new helper there as well -
csum_and_copy_to_iter(), for use in skb_copy_and_csum_datagram_iovec()
analogue that would take iov_iter. Which will very shortly replace the
iovec one. We'll need tp->ucopy.msg instead of tp->ucopy.iov for that;
that can be done as the first step, actually (ucopy.msg is always
->msg_iov of some msghdr with sufficiently long lifetime). That +
introduction of helpers leaves us with moderately-sized patch converting
tcp_recvmsg() to new semantics. And unfortunately the same patch will have
to include a (fairly simple) chunk in iscsi_do_rx_data() - same "don't
mess with ->msg_iter on subsequent iterations" thing.
At that point we'll have all kernel_{send,recv}msg() users happy with the
new semantics and are free to switch ->sendmsg()/->recvmsg() instances to
use of iov_iter primitives, not worrying about messing the callers up.
From that point it's a smooth sailing - a bunch of iovec helpers will become
dead code shortly after that, etc.
One more moderately interesting spot will be around AF_ALG stuff - we'll need
to teach iov_iter_get_pages{,_alloc}() to do the right thing for kvec-backed
iterators. Not hard to do, fortunately.
Overall, I think I have the whole series plotted in enough details to be
reasonably certain we can pull it off. Right now I'm dealing with
mm/iov_iter.c stuff; the amount of boilerplate source is already high enough
and with those extra primitives it'll get really unpleasant.
What we need there is something templates-like, as much as I hate C++, and
I'm still not happy with what I have at the moment... Hopefully I'll get
that in more or less tolerable form today.
From: Eric Dumazet <hidden> Date: 2014-11-21 15:01:30
On Fri, 2014-11-21 at 08:49 +0000, Al Viro wrote:
Another thing is tcp_sendmsg_fastopen() and tcp_send_rcvq(). The latter
should just use copy_from_iter() instead of memcpy_from_iovec(), the former
is dealt with by making tcp_send_syn_data() use the same copy_from_iter()
instead of memcpy_from_iovecend().
Well, another problem I already mentioned is that tcp_send_rcvq() does a
single alloc_skb() with @size directly coming from user space. This
certainly can try allocation of dozen of Megabytes.
Not good.
From: David Miller <davem@davemloft.net> Date: 2014-11-21 17:26:24
From: David Miller <davem@davemloft.net>
Date: Thu, 20 Nov 2014 18:23:39 -0500 (EST)
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Thu, 20 Nov 2014 21:47:53 +0000
quoted
On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
quoted
Pulled, thanks Al.
Umm... Not in net-next.git#master...
Sorry, I may have done my usual: pull at the office compute, fire up
build testing, go home without pushing back out to kernel.org
:-/
I'll check first thing tomorrow morning.
I've resolved this now, sorry for the inconvenience Al.
From: David Laight <hidden> Date: 2014-11-21 17:43:50
From: Al Viro
...
We would be better off with iov_iter passed to __sock_{send,recv}msg() (as
a part of struct msghdr, instead of ->msg_iov/->msg_iovlen) and always
advanced to match the amount of data actually picked from it. With iovec
behind it remaining constant. That would work just as well as the current
variant for sendmsg(2)/recvmsg(2)/etc., be a lot more convenient for
kernel_{send,recv}msg() callers and would allow a lot of other fun stuff.
Callers of kernel_send/recvmsg() could easily be using a wrapper
function that creates the 'msghdr'.
When the want to send the remaining part of a buffer the old iterator
will no longer be available - just the original iov and the required offset.
So it would be useful if the iterator could be initialised to a byte
offset down the iov[].
Are there any current code paths where the iov[] is modified but
ends up being something other than 'the remaining data'?
If not then code can check whether iov[0].len has changed, and
skip the 'advance' is it has.
(I've an out-of-tree driver that assumes the iov[] isn't changed.)
David
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-21 19:39:21
On Fri, Nov 21, 2014 at 05:42:55PM +0000, David Laight wrote:
Callers of kernel_send/recvmsg() could easily be using a wrapper
function that creates the 'msghdr'.
When the want to send the remaining part of a buffer the old iterator
will no longer be available - just the original iov and the required offset.
Er... So why not copy a struct iov_iter to/from msg->msg_iter, then?
It's not as it had been particulary large - 5 words isn't much...
I'm not at all sure that _anything_ has valid reasons for draining iovecs.
Maintaining a struct iov_iter and modifying it is easy and actually faster...
Right now the main examples outside of net/* are due to unfortunate
limitations of ->sendmsg() - until now it had no way to be told that
desired data starts at offset. With ->msg_iter it obviously becomes
possible...
On Fri, Nov 21, 2014 at 11:39 AM, Al Viro [off-list ref] wrote:
I'm not at all sure that _anything_ has valid reasons for draining iovecs.
Maintaining a struct iov_iter and modifying it is easy and actually faster...
For new code, I agree. But the whole "draining iovec's" is a fairly
common old model.
Linus
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-22 03:27:25
On Fri, Nov 21, 2014 at 08:49:56AM +0000, Al Viro wrote:
Overall, I think I have the whole series plotted in enough details to be
reasonably certain we can pull it off. Right now I'm dealing with
mm/iov_iter.c stuff; the amount of boilerplate source is already high enough
and with those extra primitives it'll get really unpleasant.
What we need there is something templates-like, as much as I hate C++, and
I'm still not happy with what I have at the moment... Hopefully I'll get
that in more or less tolerable form today.
Folks, I would really like comments on the patch below. It's an attempt
to reduce the amount of boilerplate code in mm/iov_iter.c; no new primitives
added, just trying to reduce the amount of duplication in there. I'm not
too fond of the way it currently looks, to put it mildly. It seems to
work, it's reasonably straightforward and it even generates slightly better
code than before, but I would _very_ welcome any tricks that would allow to
make it not so tasteless. I like the effect on line count (+124-358), but...
It defines two iterators (for iovec-backed and bvec-backed ones) and converts
a bunch of primitives to those. The last argument is an expression evaluated
for a bunch of ranges; for bvec one it's void, for iovec - size_t; if it
evaluates to non-0, we treat it as read/write/whatever short by that many
bytes and do not proceed any further.
Any suggestions are welcome.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-22 04:29:01
OK, here's the next bunch. Sorry about the delay, iov_iter.c stuff
took most of the day (and it's not included in this pile). Please, review.
Al Viro (17):
new helper: skb_copy_and_csum_datagram_msg()
new helper: memcpy_from_msg()
switch ipxrtr_route_packet() from iovec to msghdr
new helper: memcpy_to_msg()
switch drivers/net/tun.c to ->read_iter()
switch macvtap to ->read_iter()
new helpers: skb_copy_datagram_from_iter() and zerocopy_sg_from_iter()
{macvtap,tun}_get_user(): switch to iov_iter
kill zerocopy_sg_from_iovec()
switch AF_PACKET and AF_UNIX to skb_copy_datagram_from_iter()
switch sctp_user_addto_chunk() and sctp_datamsg_from_user() to passing iov_iter
tipc_sendmsg(): pass msghdr instead of its ->msg_iov
tipc_msg_build(): pass msghdr instead of its ->msg_iov
vmci_transport: switch ->enqeue_dgram, ->enqueue_stream and ->dequeue_stream to msghdr
[atm] switch vcc_sendmsg() to copy_from_iter()
rds: switch ->inc_copy_to_user() to passing iov_iter
rds: switch rds_message_copy_from_user() to iov_iter
Patches themselves are in followups...
@@ -1549,7 +1549,7 @@ static int ax25_sendmsg(struct kiocb *iocb, struct socket *sock,skb_reserve(skb,size-len);/* User data follows immediately after the AX.25 data */-if(memcpy_fromiovec(skb_put(skb,len),msg->msg_iov,len)){+if(memcpy_from_msg(skb_put(skb,len),msg,len)){err=-EFAULT;kfree_skb(skb);gotoout;
@@ -346,8 +346,7 @@ static int pppol2tp_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghskb_put(skb,2);/* Copy user data into skb */-error=memcpy_fromiovec(skb_put(skb,total_len),m->msg_iov,-total_len);+error=memcpy_from_msg(skb_put(skb,total_len),m,total_len);if(error<0){kfree_skb(skb);gotoerror_put_sess_tun;
@@ -1113,7 +1113,7 @@ static int nr_sendmsg(struct kiocb *iocb, struct socket *sock,skb_put(skb,len);/* User data follows immediately after the NET/ROM transport header */-if(memcpy_fromiovec(skb_transport_header(skb),msg->msg_iov,len)){+if(memcpy_from_msg(skb_transport_header(skb),msg,len)){kfree_skb(skb);err=-EFAULT;gotoout;
@@ -1001,7 +1001,7 @@ no_mem:/* Helper to create ABORT with a SCTP_ERROR_USER_ABORT error. */structsctp_chunk*sctp_make_abort_user(conststructsctp_association*asoc,-conststructmsghdr*msg,+structmsghdr*msg,size_tpaylen){structsctp_chunk*retval;
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-22 04:33:56
allows to switch macvtap and tun from ->aio_write() to ->write_iter()
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
drivers/net/macvtap.c | 43 ++++++++++++++++++++-----------------------
drivers/net/tun.c | 43 +++++++++++++++++++++++--------------------
2 files changed, 43 insertions(+), 43 deletions(-)
@@ -640,12 +640,12 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,/* Get packet from user space buffer */staticssize_tmacvtap_get_user(structmacvtap_queue*q,structmsghdr*m,-conststructiovec*iv,unsignedlongtotal_len,-size_tcount,intnoblock)+structiov_iter*from,intnoblock){intgood_linear=SKB_MAX_HEAD(NET_IP_ALIGN);structsk_buff*skb;structmacvlan_dev*vlan;+unsignedlongtotal_len=iov_iter_count(from);unsignedlonglen=total_len;interr;structvirtio_net_hdrvnet_hdr={0};
@@ -764,16 +763,12 @@ err:returnerr;}-staticssize_tmacvtap_aio_write(structkiocb*iocb,conststructiovec*iv,-unsignedlongcount,loff_tpos)+staticssize_tmacvtap_write_iter(structkiocb*iocb,structiov_iter*from){structfile*file=iocb->ki_filp;-ssize_tresult=-ENOLINK;structmacvtap_queue*q=file->private_data;-result=macvtap_get_user(q,NULL,iv,iov_length(iv,count),count,-file->f_flags&O_NONBLOCK);-returnresult;+returnmacvtap_get_user(q,NULL,from,file->f_flags&O_NONBLOCK);}/* Put packet to the user space buffer */
@@ -1012,28 +1012,29 @@ static struct sk_buff *tun_alloc_skb(struct tun_file *tfile,/* Get packet from user space buffer */staticssize_ttun_get_user(structtun_struct*tun,structtun_file*tfile,-void*msg_control,conststructiovec*iv,-size_ttotal_len,size_tcount,intnoblock)+void*msg_control,structiov_iter*from,+intnoblock){structtun_pipi={0,cpu_to_be16(ETH_P_IP)};structsk_buff*skb;+size_ttotal_len=iov_iter_count(from);size_tlen=total_len,align=NET_SKB_PAD,linear;structvirtio_net_hdrgso={0};intgood_linear;-intoffset=0;intcopylen;boolzerocopy=false;interr;u32rxhash;+ssize_tn;if(!(tun->flags&TUN_NO_PI)){if(len<sizeof(pi))return-EINVAL;len-=sizeof(pi);-if(memcpy_fromiovecend((void*)&pi,iv,0,sizeof(pi)))+n=copy_from_iter(&pi,sizeof(pi),from);+if(n!=sizeof(pi))return-EFAULT;-offset+=sizeof(pi);}if(tun->flags&TUN_VNET_HDR){
@@ -1063,6 +1065,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,good_linear=SKB_MAX_HEAD(align);if(msg_control){+structiov_iteri=*from;/* There are 256 bytes to be copied in skb, so there is*enoughroomforskbexpandheadincaseitisused.*Therestofthebufferismappedfromuserspace.
@@ -2661,8 +2661,6 @@ int skb_copy_datagram_from_iovec(struct sk_buff *skb, int offset,intlen);intskb_copy_datagram_from_iter(structsk_buff*skb,intoffset,structiov_iter*from,intlen);-intzerocopy_sg_from_iovec(structsk_buff*skb,conststructiovec*frm,-intoffset,size_tcount);intskb_copy_datagram_iter(conststructsk_buff*from,intoffset,structiov_iter*to,intsize);intzerocopy_sg_from_iter(structsk_buff*skb,structiov_iter*frm);
@@ -644,76 +644,15 @@ fault:EXPORT_SYMBOL(skb_copy_datagram_from_iter);/**-*zerocopy_sg_from_iovec-Buildazerocopydatagramfromaniovec+*zerocopy_sg_from_iter-Buildazerocopydatagramfromaniov_iter*@skb:buffertocopy-*@from:iovectortocopyfrom-*@offset:offsetintheiovectortostartcopyingfrom-*@count:amountofvectorstocopytobufferfrom+*@from:thesourcetocopyfrom**Thefunctionwillfirstcopyuptoheadlen,andthenpintheuserspace*pagesandbuildfragsthroughthem.**Returns0,-EFAULTor-EMSGSIZE.-*Note:theiovecisnotmodifiedduringthecopy*/-intzerocopy_sg_from_iovec(structsk_buff*skb,conststructiovec*from,-intoffset,size_tcount)-{-intlen=iov_length(from,count)-offset;-intcopy=min_t(int,skb_headlen(skb),len);-intsize;-inti=0;--/* copy up to skb headlen */-if(skb_copy_datagram_from_iovec(skb,0,from,offset,copy))-return-EFAULT;--if(len==copy)-return0;--offset+=copy;-while(count--){-structpage*page[MAX_SKB_FRAGS];-intnum_pages;-unsignedlongbase;-unsignedlongtruesize;--/* Skip over from offset and copied */-if(offset>=from->iov_len){-offset-=from->iov_len;-++from;-continue;-}-len=from->iov_len-offset;-base=(unsignedlong)from->iov_base+offset;-size=((base&~PAGE_MASK)+len+~PAGE_MASK)>>PAGE_SHIFT;-if(i+size>MAX_SKB_FRAGS)-return-EMSGSIZE;-num_pages=get_user_pages_fast(base,size,0,&page[i]);-if(num_pages!=size){-release_pages(&page[i],num_pages,0);-return-EFAULT;-}-truesize=size*PAGE_SIZE;-skb->data_len+=len;-skb->len+=len;-skb->truesize+=truesize;-atomic_add(truesize,&skb->sk->sk_wmem_alloc);-while(len){-intoff=base&~PAGE_MASK;-intsize=min_t(int,len,PAGE_SIZE-off);-skb_fill_page_desc(skb,i,page[i],off,size);-base+=size;-len-=size;-i++;-}-offset=0;-++from;-}-return0;-}-EXPORT_SYMBOL(zerocopy_sg_from_iovec);-intzerocopy_sg_from_iter(structsk_buff*skb,structiov_iter*from){intlen=iov_iter_count(from);
@@ -279,12 +280,10 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,gotoerrout;}-err=sctp_user_addto_chunk(chunk,offset,len,msgh->msg_iov);+err=sctp_user_addto_chunk(chunk,len,from);if(err<0)gotoerrout_chunk_free;-offset+=len;-/* Put the chunk->skb back into the form expected by send. */__skb_pull(chunk->skb,(__u8*)chunk->chunk_hdr-(__u8*)chunk->skb->data);
@@ -317,7 +316,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,gotoerrout;}-err=sctp_user_addto_chunk(chunk,offset,over,msgh->msg_iov);+err=sctp_user_addto_chunk(chunk,over,from);/* Put the chunk->skb back into the form expected by send. */__skb_pull(chunk->skb,(__u8*)chunk->chunk_hdr
@@ -1491,26 +1491,26 @@ static void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,*chunkisnotbigenough.*Returnsakernelerrvalue.*/-intsctp_user_addto_chunk(structsctp_chunk*chunk,intoff,intlen,-structiovec*data)+intsctp_user_addto_chunk(structsctp_chunk*chunk,intlen,+structiov_iter*from){-__u8*target;-interr=0;+void*target;+ssize_tcopied;/* Make room in chunk for data. */target=skb_put(chunk->skb,len);/* Copy data (whole iovec) into chunk */-if((err=memcpy_fromiovecend(target,data,off,len)))-gotoout;+copied=copy_from_iter(target,len,from);+if(copied!=len)+return-EFAULT;/* Adjust the chunk length field. */chunk->chunk_hdr->length=htons(ntohs(chunk->chunk_hdr->length)+len);chunk->chunk_end=skb_tail_pointer(chunk->skb);-out:-returnerr;+return0;}/* Helper function to assign a TSN if needed. This assumes that both
@@ -1947,7 +1950,7 @@ static int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,}/* Break the message into multiple chunks of maximum size. */-datamsg=sctp_datamsg_from_user(asoc,sinfo,msg,msg_len);+datamsg=sctp_datamsg_from_user(asoc,sinfo,&from);if(IS_ERR(datamsg)){err=PTR_ERR(datamsg);gotoout_free;
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-22 04:38:30
... and make it handle multi-segment iovecs - deals with that
"fix this later" issue for free. A bit of shame, really - it
had been there since 2.3.15pre3 when the whole thing went into the
tree, practically a historical artefact by now...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/atm/common.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
From: David Miller <davem@davemloft.net> Date: 2014-11-22 07:24:51
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 22 Nov 2014 04:28:57 +0000
OK, here's the next bunch. Sorry about the delay, iov_iter.c stuff
took most of the day (and it's not included in this pile). Please, review.
I read over this stuff twice and this series looks fine to me.
Since this is the weekend... maybe wait until Monday for other feedback
then give me a pull request?
Thanks Al.
On Fri, Nov 21, 2014 at 8:28 PM, Al Viro [off-list ref] wrote:
OK, here's the next bunch.
Looks like good patches to me. Not that I actually _tested_ it, or
even have a good test-case (yeah, that "historical" ATM fix? I don't
think anybody cares ;), but it all seemed sane.
Linus
[...]
You need to leave this break at the bottom of the loop body, or change
it to:
do {
...
} while (!skb);
Ben.
--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
From: Ben Hutchings <hidden> Date: 2014-11-24 00:28:06
On Sat, 2014-11-22 at 04:33 +0000, Al Viro wrote:
quoted hunk
allows to switch macvtap and tun from ->aio_write() to ->write_iter()
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
drivers/net/macvtap.c | 43 ++++++++++++++++++++-----------------------
drivers/net/tun.c | 43 +++++++++++++++++++++++--------------------
2 files changed, 43 insertions(+), 43 deletions(-)
@@ -640,12 +640,12 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,/* Get packet from user space buffer */staticssize_tmacvtap_get_user(structmacvtap_queue*q,structmsghdr*m,-conststructiovec*iv,unsignedlongtotal_len,-size_tcount,intnoblock)+structiov_iter*from,intnoblock){intgood_linear=SKB_MAX_HEAD(NET_IP_ALIGN);structsk_buff*skb;structmacvlan_dev*vlan;+unsignedlongtotal_len=iov_iter_count(from);unsignedlonglen=total_len;interr;structvirtio_net_hdrvnet_hdr={0};
Does skb_copy_datagram_from_iter() really need a len parameter? Here it
is equal to iov_iter_count(from).
[...]
quoted hunk
--- a/drivers/net/tun.c+++ b/drivers/net/tun.c
@@ -1012,28 +1012,29 @@ static struct sk_buff *tun_alloc_skb(struct tun_file *tfile,/* Get packet from user space buffer */staticssize_ttun_get_user(structtun_struct*tun,structtun_file*tfile,-void*msg_control,conststructiovec*iv,-size_ttotal_len,size_tcount,intnoblock)+void*msg_control,structiov_iter*from,+intnoblock){structtun_pipi={0,cpu_to_be16(ETH_P_IP)};structsk_buff*skb;+size_ttotal_len=iov_iter_count(from);size_tlen=total_len,align=NET_SKB_PAD,linear;structvirtio_net_hdrgso={0};intgood_linear;-intoffset=0;intcopylen;boolzerocopy=false;interr;u32rxhash;+ssize_tn;if(!(tun->flags&TUN_NO_PI)){if(len<sizeof(pi))return-EINVAL;len-=sizeof(pi);-if(memcpy_fromiovecend((void*)&pi,iv,0,sizeof(pi)))+n=copy_from_iter(&pi,sizeof(pi),from);+if(n!=sizeof(pi))return-EFAULT;-offset+=sizeof(pi);}if(tun->flags&TUN_VNET_HDR){
/* There are 256 bytes to be copied in skb, so there is
* enough room for skb expand head in case it is used.
* The rest of the buffer is mapped from userspace.
@@ -1071,7 +1074,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (copylen > good_linear) copylen = good_linear; linear = copylen;- if (iov_pages(iv, offset + copylen, count) <= MAX_SKB_FRAGS)+ iov_iter_advance(&i, copylen);+ if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
Again, the maxpages argument should be MAX_SKB_FRAGS + 1.
[...]
Again len is equal to iov_iter_count(from), so I think that parameter is
redundant.
Ben.
--
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
[...]
Never mind, I can see that patches 9 and 10 recycle the _iovec
functions' kernel-doc comments.
Ben.
--
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
From: Ben Hutchings <hidden> Date: 2014-11-24 01:06:34
On Mon, 2014-11-24 at 00:27 +0000, Ben Hutchings wrote:
On Sat, 2014-11-22 at 04:33 +0000, Al Viro wrote:
[...]
Does skb_copy_datagram_from_iter() really need a len parameter? Here it
is equal to iov_iter_count(from).
[...]
Again len is equal to iov_iter_count(from), so I think that parameter is
redundant.
Having read further patches, I see that unix_stream_sendmsg() is the one
exception where the length parameter is different. But maybe the common
case (len = iter_iov_count(iov)) deserves a wrapper function?
Ben.
--
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
[...]
It looks like rds_page_copy{,_from,_to}_user() are all unused after this
change, so you could delete them.
Ben.
--
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
Why is this condition needed, given we told iov_iter_get_pages() to
limit to MAX_SKB_FRAGS pages?
We don't want to send truncated packets and there's no other way to put
those pages since it was not in the frag array.
No, his point is that it could never happen. It could, actually - what's
confusing here (and that's inherited from zerocopy_from_iovec()) is
that 'i' is a lousy name for that variable. It's actually "how many fragments
have we already put there?" and it is not reset when we go into the next
iteration of outer loop.
FWIW, I've just renamed it into 'frag', put
if (frag == MAX_SKB_FRAGS)
return -EMSGSIZE;
*before* iov_iter_get_pages(), passing MAX_SKB_FRAGS - frag as the
limit on number of pages in that call. Voila - logics with put_page()
disappears and the inner loop is less obfuscated.
There was another bug in that function - iov_iter_get_pages() does *not*
advance the iterator; the caller needs to do iov_iter_advance() itself.
Also fixed...
From: David Laight <hidden> Date: 2014-11-24 10:04:14
From: Al Viro
On Fri, Nov 21, 2014 at 05:42:55PM +0000, David Laight wrote:
quoted
Callers of kernel_send/recvmsg() could easily be using a wrapper
function that creates the 'msghdr'.
When the want to send the remaining part of a buffer the old iterator
will no longer be available - just the original iov and the required offset.
Er... So why not copy a struct iov_iter to/from msg->msg_iter, then?
It's not as it had been particulary large - 5 words isn't much...
I'm not at all sure that _anything_ has valid reasons for draining iovecs.
Maintaining a struct iov_iter and modifying it is easy and actually faster...
Right now the main examples outside of net/* are due to unfortunate
limitations of ->sendmsg() - until now it had no way to be told that
desired data starts at offset. With ->msg_iter it obviously becomes
possible...
It may well be easier for code that only has to run in a new kernel.
But for code that has run in old kernels as well you still need
to modify the iov[].
This is also true of userspace - there is no way of completing
a partial transfer without modifying the iov[] to allow for the
partial transfer.
Note that I'm not suggesting that any of the 'write' functions
should ever modify an iov[].
David
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-24 10:15:51
On Mon, Nov 24, 2014 at 12:27:42AM +0000, Ben Hutchings wrote:
quoted
copylen = vnet_hdr.hdr_len ? vnet_hdr.hdr_len : GOODCOPY_LEN;
if (copylen > good_linear)
copylen = good_linear;
linear = copylen;
- if (iov_pages(iv, vnet_hdr_len + copylen, count)
- <= MAX_SKB_FRAGS)
+ i = *from;
+ iov_iter_advance(&i, copylen);
+ if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
The maxpages argument should be MAX_SKB_FRAGS + 1 as we don't need the
exact number.
In principle, that's true, but... Do we really care? It only buys you
anything if you have a monstrously fragmented iovec. And if you end up
spending too considerable amount of time in that loop in iov_iter_npages,
you are by definition on the slow path - it *will* fail, since we do not
even try to merge adjacent iovec segments. Never had...
From: David Laight <hidden> Date: 2014-11-24 10:28:16
From: Al Viro
quoted hunk
On Fri, Nov 21, 2014 at 08:49:56AM +0000, Al Viro wrote:
quoted
Overall, I think I have the whole series plotted in enough details to be
reasonably certain we can pull it off. Right now I'm dealing with
mm/iov_iter.c stuff; the amount of boilerplate source is already high enough
and with those extra primitives it'll get really unpleasant.
What we need there is something templates-like, as much as I hate C++, and
I'm still not happy with what I have at the moment... Hopefully I'll get
that in more or less tolerable form today.
Folks, I would really like comments on the patch below. It's an attempt
to reduce the amount of boilerplate code in mm/iov_iter.c; no new primitives
added, just trying to reduce the amount of duplication in there. I'm not
too fond of the way it currently looks, to put it mildly. It seems to
work, it's reasonably straightforward and it even generates slightly better
code than before, but I would _very_ welcome any tricks that would allow to
make it not so tasteless. I like the effect on line count (+124-358), but...
It defines two iterators (for iovec-backed and bvec-backed ones) and converts
a bunch of primitives to those. The last argument is an expression evaluated
for a bunch of ranges; for bvec one it's void, for iovec - size_t; if it
evaluates to non-0, we treat it as read/write/whatever short by that many
bytes and do not proceed any further.
Any suggestions are welcome.
You are assigning to parameters, this can get confusing.
Unless these are return values this probably doesn't make sense.
Might be better to 'pass by reference', the generated code is
likely to be the same - but it is clearer to the reader.
+ left = STEP; \
+ len -= left; \
+ skip += len; \
+ n -= len; \
Using 'buf' and 'len' in __copy_to_user() is very non-obvious.
It might be better if they were fixed names, maybe _ioiter_buf and _ioiter_len.
If this code can use the gcc extension that allows #defines that contain {}
to return values, the it would be better as:
bytes_left = iterate_iovec(i, bytes, true,
__copy_to_user(_ioiter_buf, (from += _ioiter_len) - _ioiter_len,
_ioiter_len);
return bytes_left;
Possibly also two wrapper #defines that supply the true/false parameter.
David
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-25 02:40:24
On Sat, Nov 22, 2014 at 02:24:44AM -0500, David Miller wrote:
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 22 Nov 2014 04:28:57 +0000
quoted
OK, here's the next bunch. Sorry about the delay, iov_iter.c stuff
took most of the day (and it's not included in this pile). Please, review.
I read over this stuff twice and this series looks fine to me.
Since this is the weekend... maybe wait until Monday for other feedback
then give me a pull request?
I'll probably repost the patches with updates folded in first...
FWIW, the current situation is
* all but one ->recvmsg() instances switched to iov_iter primitives
(the exception is one of the AF_ALG instances and I know what to do with it).
Consequently, they do not drain iovecs anymore and simply advance ->msg_iter.
* kvec-based iov_iter work just fine without set_fs(KERNEL_DS). And
so do ->recvmsg() instances that had received such iov_iter.
* memcpy_to_iovec() and skb_copy_datagram_iovec() are gone - no users
left.
I'm about to start on ->sendmsg() side of the things. The interesting
question is whether tcp_send_syn_data() is doing the right thing if it
runs into EFAULT when copying iovec from userland.
As it is, it gives up on the skb it has allocated and falls back to normal
handshake. I can duplicate that behaviour, all right, but why not simply
do skb_trim(syn_data, <actually copied>) and do fallback only if nothing
got copied at all? Is there any problem with that?
The reason why I'm asking is that it's easier to just use copy_from_iter()
and let the damn thing advance. Not a lot of pain to preserve the original
iov_iter (all 5 words of it) and copy it back in case of failure, but I'd
rather understood what's wrong with simpler approach...
Anyway, the current branch is in vfs.git#iov_iter-net. Current balance is
about -0.5KLoC and that's not counting the simplifications that will become
possible in callers of kernel_sendmg/kernel_recvmsg... I'll post the beginning
of that queue (the same 17 commits in the beginning) later tonight and wait
for review...
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2014-11-25 14:03:05
From: Al Viro <viro@zeniv.linux.org.uk>
... and make it handle multi-segment iovecs - deals with that
"fix this later" issue for free. A bit of shame, really - it
had been there since 2.3.15pre3 when the whole thing went into the
tree, practically a historical artefact by now...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/atm/common.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
@@ -279,12 +280,10 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,gotoerrout;}-err=sctp_user_addto_chunk(chunk,offset,len,msgh->msg_iov);+err=sctp_user_addto_chunk(chunk,len,from);if(err<0)gotoerrout_chunk_free;-offset+=len;-/* Put the chunk->skb back into the form expected by send. */__skb_pull(chunk->skb,(__u8*)chunk->chunk_hdr-(__u8*)chunk->skb->data);
@@ -317,7 +316,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,gotoerrout;}-err=sctp_user_addto_chunk(chunk,offset,over,msgh->msg_iov);+err=sctp_user_addto_chunk(chunk,over,from);/* Put the chunk->skb back into the form expected by send. */__skb_pull(chunk->skb,(__u8*)chunk->chunk_hdr
@@ -1491,26 +1491,26 @@ static void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,*chunkisnotbigenough.*Returnsakernelerrvalue.*/-intsctp_user_addto_chunk(structsctp_chunk*chunk,intoff,intlen,-structiovec*data)+intsctp_user_addto_chunk(structsctp_chunk*chunk,intlen,+structiov_iter*from){-__u8*target;-interr=0;+void*target;+ssize_tcopied;/* Make room in chunk for data. */target=skb_put(chunk->skb,len);/* Copy data (whole iovec) into chunk */-if((err=memcpy_fromiovecend(target,data,off,len)))-gotoout;+copied=copy_from_iter(target,len,from);+if(copied!=len)+return-EFAULT;/* Adjust the chunk length field. */chunk->chunk_hdr->length=htons(ntohs(chunk->chunk_hdr->length)+len);chunk->chunk_end=skb_tail_pointer(chunk->skb);-out:-returnerr;+return0;}/* Helper function to assign a TSN if needed. This assumes that both
@@ -1947,7 +1950,7 @@ static int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,}/* Break the message into multiple chunks of maximum size. */-datamsg=sctp_datamsg_from_user(asoc,sinfo,msg,msg_len);+datamsg=sctp_datamsg_from_user(asoc,sinfo,&from);if(IS_ERR(datamsg)){err=PTR_ERR(datamsg);gotoout_free;
@@ -2687,6 +2687,11 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len);intskb_vlan_pop(structsk_buff*skb);intskb_vlan_push(structsk_buff*skb,__be16vlan_proto,u16vlan_tci);+staticinlineintmemcpy_from_msg(void*data,structmsghdr*msg,intlen)+{+returnmemcpy_fromiovec(data,msg->msg_iov,len);+}+structskb_checksum_ops{__wsum(*update)(constvoid*mem,intlen,__wsumwsum);__wsum(*combine)(__wsumcsum,__wsumcsum2,intoffset,intlen);
@@ -1549,7 +1549,7 @@ static int ax25_sendmsg(struct kiocb *iocb, struct socket *sock,skb_reserve(skb,size-len);/* User data follows immediately after the AX.25 data */-if(memcpy_fromiovec(skb_put(skb,len),msg->msg_iov,len)){+if(memcpy_from_msg(skb_put(skb,len),msg,len)){err=-EFAULT;kfree_skb(skb);gotoout;
@@ -346,8 +346,7 @@ static int pppol2tp_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghskb_put(skb,2);/* Copy user data into skb */-error=memcpy_fromiovec(skb_put(skb,total_len),m->msg_iov,-total_len);+error=memcpy_from_msg(skb_put(skb,total_len),m,total_len);if(error<0){kfree_skb(skb);gotoerror_put_sess_tun;
@@ -1113,7 +1113,7 @@ static int nr_sendmsg(struct kiocb *iocb, struct socket *sock,skb_put(skb,len);/* User data follows immediately after the NET/ROM transport header */-if(memcpy_fromiovec(skb_transport_header(skb),msg->msg_iov,len)){+if(memcpy_from_msg(skb_transport_header(skb),msg,len)){kfree_skb(skb);err=-EFAULT;gotoout;
@@ -1001,7 +1001,7 @@ no_mem:/* Helper to create ABORT with a SCTP_ERROR_USER_ABORT error. */structsctp_chunk*sctp_make_abort_user(conststructsctp_association*asoc,-conststructmsghdr*msg,+structmsghdr*msg,size_tpaylen){structsctp_chunk*retval;