Date: Tue, 16 Jul 2019 14:52:38 -0600 From: Alan Somers <asomers@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: John Baldwin <jhb@freebsd.org>, "Conrad E. Meyer" <cem@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349391 - head/sys/kern Message-ID: <CAOtMX2gVQs7j%2BNnszvSeaEA5MTDLKS7%2BRz1W-xaq_DR=1VHgXg@mail.gmail.com> In-Reply-To: <20190716180618.C962@besplex.bde.org> References: <201906251944.x5PJiNaA093352@repo.freebsd.org> <CAG6CVpUscojbGvA0=BmZPhSR%2B5Yt5ah8SQKrBaouUNDpu9tw9A@mail.gmail.com> <3b2ade4e-d19d-31e4-7ee2-c0085a00e53e@FreeBSD.org> <20190716180618.C962@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jul 16, 2019 at 3:48 AM Bruce Evans <brde@optusnet.com.au> wrote: > > On Mon, 15 Jul 2019, John Baldwin wrote: > > > On 7/14/19 12:08 PM, Conrad Meyer wrote: > >> > >> This change restores the possible overflow beyond IO_SEQMAX that the > >> removed conditional prevented. > >> > >> On Tue, Jun 25, 2019 at 12:44 PM Alan Somers <asomers@freebsd.org> wrote: > > I thought the same for a while on Tue, Jan 25, then didn't reply since I > saw a non-problem while writing the reply. Actually, all versions are > broken. > > >>> Author: asomers > >>> Date: Tue Jun 25 19:44:22 2019 > >>> New Revision: 349391 > >>> URL: https://svnweb.freebsd.org/changeset/base/349391 > >>> > >>> --- head/sys/kern/vfs_vnops.c Tue Jun 25 19:36:01 2019 (r349390) > >>> +++ head/sys/kern/vfs_vnops.c Tue Jun 25 19:44:22 2019 (r349391) > >>> @@ -499,10 +499,8 @@ sequential_heuristic(struct uio *uio, struct file *fp) > >>> * closely related to the best I/O size for real disks than > >>> * to any block size used by software. > >>> */ > >>> - fp->f_seqcount += MIN(IO_SEQMAX, > >>> + fp->f_seqcount += lmin(IO_SEQMAX, > >>> howmany(uio->uio_resid, 16384)); > >>> - if (fp->f_seqcount > IO_SEQMAX) > >>> - fp->f_seqcount = IO_SEQMAX; > >>> return (fp->f_seqcount << IO_SEQSHIFT); > >>> } > > Overflow occurs in all versions in howmany() when uio_resid is near > its limit of SSIZE_MAX. Then adding (65536 - 1) to uio_resid overflows. > The behaviour is undefined, but usually the addition gives a large > negative value and howmany gives a not so large negative value after > dividing by 65536. > > In the previous version, MIN() preserves signedness and the type of > howmany(), which is always long (since ssize_t happens to be long on all > supported arches). The large negative value is less than IO_SEQMAX sinc > that has a correct type (signed int) and is non-negative. So MIN() preserves > the large negative value. This is blindly added to f_seqcount, giving > another overflow on 64-bit arches and a garbage negative value on 32-bit > arches if f_seqcount was not garbage. The removed conditional only fixes > up large positive values, so has no effect in this case. Finally, left > shifting the garbage gives further undefined behaviour. > > The second overflow on 64-bit arches is because the not so large negative > value is still much larger than can be represented in f_seqcount. > Values near -SSIZE_MAX have 63 value bits, so dividing by 16384 leaves > 47 value bits, but f_seqcount has only 31 value bits. > > In this version, lmin() happens to give the same result as MIN() on all > supported machines, because ssize_t happens to have the same representation > on all supported machines. So overflows occur as before for large uio_resid. > > So this change makes no differences for the 2 overflows for uio_resid near > SSIZE_MAX. But removing the conditional restores an overflow that actually > was fixed in a previous recent commit. > > >> Perhaps instead this should be: > >> > >> fp->f_seqcount = lmin(IO_SEQMAX, > >> fp->f_seqcount + howmany(...)); > > This works to restore the check in the removed conditional, but is a bit > subtle and leaves the 2 old overflow bugs unfixed. > > > While I agree with your assessment of the removed conditional, I think the > > suggestion above can have a funky overflow where f_seqcount + howmany(...) ends > > up less than IO_SEQMAX and this expression would then be used instead of IO_SEQMAX? > > This only happens when howmany() itself overflows. Otherwise, dividing by > 16384 reduces the value enough to add f_seqcount to howmany(...) without > overflow. > > > The first lmin seems designed to minimize what we add in the hope that since the > > existing value is bounded to '0...IO_SEQMAX' the result of the += can only be in > > the range '0...2*IO_SEQMAX'. > > One addition doesn't overflow, and the shift could be clamped later, but > overflow still occurs after approx. INT_MAX / IO_SEQMAX additions, so it > is better to clamp earlier. > > > I'm not sure which variants are most readable: > > > > A) > > fp->f_seqcount += lmin(IO_SEQMAX, > > howmany(resid, 16384)); > > if (fp->f_seqcount > IO_SEQMAX) > > fp->f_seqcount = IO_SEQMAX; > > This change is because I said that using the unsafe macro MIN() is a style > bug. I also said that using the lmin() family is too hard, since it is > not type generic. I didn't notice the overflow problem in howmany(). > > This has another style bug -- using lmin() and then open-coding the clamp > f->f_seqcount = imax(f->f_seqcount, IO_SEQMAX). This is sort of backwards -- > we use open code for the simple case because it is simple with either > spelling, but we use lmin() for the complicated case to avoid spelling out > the howmany() expression twice. This obscures the 2 overflow bugs in the > howmany() expression and many delicate type promotions and type equivalences > that are needed for lmin() to work. > > > B) > > fp->f_seqcount += lmin(IO_SEQMAX, > > howmany(resid, 16384)); > > fp->f_seqcount = lmin(fp->f_seqcount, IO_SEQMAX); > > Consistent style, but another has another type error. f_seqcount has type > int. Of course, lmin() handles ints too, but to see that it works you > still have to check that f_seqcount is representable as long and once you > do that it is easier to see that imin() is courrect. > > > C) > > if (fp->f_seqcount + howmany(resid, 16384) < fp->f_seqcount) > > fp->f_seqcount = IO_SEQMAX; > > else > > fp->f_seqcount = lmin(IO_SEQMAX, > > fp->f_seqcount + howmany(resid, 16384)); > > > > I'm probably not a fan of C). > > On supported arches, it recovers from overflow in howmany(), but only if > the compiler doesn't optimize away the recovery. > > The first part can be done better: > > if (uio->uio_resid >= IO_SEQMAX * 16384) > fp->f_seqcount = IO_SEQMAX; > > Then for the second part, any version works since all values are small and > non-negative, but I prefer to restore the the version that I rewrote 15-20 > years ago and committed 11 years ago (r175106): > > fp->f_seqcount += howmany(uio->uio_resid, 16384); > if (fp->f_seqcount > IO_SEQMAX) > fp->f_seqcount = IO_SEQMAX; > > My changes to this were to use 16384 instead of BKVASIZE and howmany() > instead of its expansion. Changing to howmany() was a mistake since > it hides the first overflow. When I rewrote it, uio_resid had type > int so it was small enough after division by 16384, but not small enough > to add (16384 - 1) to. I didn't change the open-coded clamp on f_seqcount > in this because it was good enough. > > Bruce Bruce, Is this how you want it? Index: sys/kern/vfs_vnops.c =================================================================== --- sys/kern/vfs_vnops.c (revision 349977) +++ sys/kern/vfs_vnops.c (working copy) @@ -499,8 +499,13 @@ * closely related to the best I/O size for real disks than * to any block size used by software. */ - fp->f_seqcount += lmin(IO_SEQMAX, - howmany(uio->uio_resid, 16384)); + if (uio->uio_resid >= IO_SEQMAX * 16384) + fp->f_seqcount = IO_SEQMAX; + else { + fp->f_seqcount += howmany(uio->uio_resid, 16384); + if (fp->f_seqcount > IO_SEQMAX) + fp->f_seqcount = IO_SEQMAX; + } return (fp->f_seqcount << IO_SEQSHIFT); } -Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2gVQs7j%2BNnszvSeaEA5MTDLKS7%2BRz1W-xaq_DR=1VHgXg>