From owner-svn-src-all@FreeBSD.ORG Mon Jan 30 19:07:04 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D931C106564A; Mon, 30 Jan 2012 19:07:04 +0000 (UTC) (envelope-from das@FreeBSD.ORG) Received: from zim.MIT.EDU (ZIM.MIT.EDU [18.95.3.101]) by mx1.freebsd.org (Postfix) with ESMTP id A6AF28FC0A; Mon, 30 Jan 2012 19:07:04 +0000 (UTC) Received: from zim.MIT.EDU (localhost [127.0.0.1]) by zim.MIT.EDU (8.14.5/8.14.2) with ESMTP id q0UJ73j1044841; Mon, 30 Jan 2012 14:07:03 -0500 (EST) (envelope-from das@FreeBSD.ORG) Received: (from das@localhost) by zim.MIT.EDU (8.14.5/8.14.2/Submit) id q0UJ73xx044840; Mon, 30 Jan 2012 14:07:03 -0500 (EST) (envelope-from das@FreeBSD.ORG) Date: Mon, 30 Jan 2012 14:07:03 -0500 From: David Schultz To: Kostik Belousov Message-ID: <20120130190703.GA44663@zim.MIT.EDU> Mail-Followup-To: Kostik Belousov , Bruce Evans , Gleb Smirnoff , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org References: <20120126233110.C960@besplex.bde.org> <20120126153641.GA68112@FreeBSD.org> <20120127194612.H1547@besplex.bde.org> <20120127091244.GZ2726@deviant.kiev.zoral.com.ua> <20120127194221.GA25723@zim.MIT.EDU> <20120128123748.GD2726@deviant.kiev.zoral.com.ua> <20120129001225.GA32220@zim.MIT.EDU> <20120129062327.GK2726@deviant.kiev.zoral.com.ua> <20120129223904.GA37483@zim.MIT.EDU> <20120130063034.GU2726@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120130063034.GU2726@deviant.kiev.zoral.com.ua> Cc: svn-src-head@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, Gleb Smirnoff , src-committers@FreeBSD.ORG, Bruce Evans Subject: Re: svn commit: r230583 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Jan 2012 19:07:04 -0000 On Mon, Jan 30, 2012, Kostik Belousov wrote: > On Sun, Jan 29, 2012 at 05:39:04PM -0500, David Schultz wrote: > > On Sun, Jan 29, 2012, Kostik Belousov wrote: > > > On Sat, Jan 28, 2012 at 07:12:25PM -0500, David Schultz wrote: > > > > On Sat, Jan 28, 2012, Kostik Belousov wrote: > > > > > On Fri, Jan 27, 2012 at 02:42:21PM -0500, David Schultz wrote: > > > > > > The correct limit on the maximum size of a single read/write is > > > > > > SSIZE_MAX, but FreeBSD uses INT_MAX. It's not safe to raise the > > > > > > limit yet, though, because of bugs in several filesystems. For > > > > > > example, FFS copies uio_resid into a local variable of type int. > > > > > > I have some old patches that fix some of these issues for FFS and > > > > > > cd9660, but surely there are more places I didn't notice. > > > > > > > > > > > Absolutely agree. > > > > > > > > > > http://people.freebsd.org/~kib/misc/uio_resid.5.patch > > > > > > > > Nice. You found a lot more than I've got in my tree, and you even > > > > fixed the return values. There are at least a few more places to > > > > fix. For instance, cd9660 and the NFS client pass uio_resid or > > > > iov_len to min(), which operates on ints. (Incidentally, C11 > > > > generics ought to make it possible to write type-generic min() > > > > and max() functions.) > > > > > > Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch > > > changed them to MIN(). > > > > This looks good to me. I tried to think of other places that you > > might have missed, and the only one that occurred to me is the > Might ? I think this is a blatant understate. > > > pipe code. sys_pipe.c has an `int orig_resid' and lots of bogus > > casts of iov_len and uio_resid to type u_int. Some look harmless, > > although it appears that writing a multiple of 2^32 bytes might > > result in pipe_build_write_buffer() allocating a 0-length buffer. > > > > My only reservation is that raising the limit could unmask a > > kernel buffer overflow if we missed something, but I guess we have > > to cross that bridge some day anyway. > Yes, and it is an obvious reason why I am chicken to commit this for > so long time. One more place, if this is reasonable to count as 'one' > place, are the cdevsw methods. devfs passes uio down to the drivers. That's why I'm glad I'm not committing it. :) A more conservative change (also known as "kicking the can down the road") would be to add a VFS flag, e.g., VFCF_LONGIO, and only set it on file systems that have been thoroughly reviewed. The VFS layer could cap the size at INT_MAX for file systems without the flag. > diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c > index 9edcb74..332ec37 100644 > --- a/sys/kern/sys_pipe.c > +++ b/sys/kern/sys_pipe.c [...] > @@ -757,14 +757,14 @@ pipe_build_write_buffer(wpipe, uio) > struct pipe *wpipe; > struct uio *uio; > { > - u_int size; > + size_t size; > int i; > > PIPE_LOCK_ASSERT(wpipe, MA_NOTOWNED); > KASSERT(wpipe->pipe_state & PIPE_DIRECTW, > ("Clone attempt on non-direct write pipe!")); > > - size = (u_int) uio->uio_iov->iov_len; > + size = uio->uio_iov->iov_len; > if (size > wpipe->pipe_buffer.size) > size = wpipe->pipe_buffer.size; The transfer can't be bigger than the max pipe buffer size (64k), so `size = (int)MIN(uio->uio_iov->iov_len, wpipe->pipe_buffer.size)' should suffice. The same comment applies elsewhere in the file.