Date: Sat, 27 Jan 2018 09:14:11 -0700 From: Warner Losh <imp@bsdimp.com> To: Pedro Giffuni <pfg@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs Message-ID: <CANCZdfqq7eS5qMWVrG=U0aJJuNc%2BGmqvxa9=_0BbNr0tAqpx=Q@mail.gmail.com> In-Reply-To: <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org> References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> <20180126053133.R3207@besplex.bde.org> <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> <20180126214948.C1040@besplex.bde.org> <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <pfg@freebsd.org> wrote:
On 01/26/18 06:36, Bruce Evans wrote:
> On Thu, 25 Jan 2018, Pedro Giffuni wrote:
>
> On 25/01/2018 14:24, Bruce Evans wrote:
>>
>>> ...
>>> This code only works because (if?) nfs is the only caller and nfs never
>>> passes insane values.
>>>
>>>
>> I am starting to think that we should simply match uio_resid and set it
>> to ssize_t.
>> Returning the value to int is certainly not the solution.
>>
>
> Of course using the correct type (int) is part of the solution.
>
> uio_must be checked before it is used for cookies, and after checking it,
> it
> is small so it fits easily in an int. It must also checked to be
> nonnegative,
> so that it doesn't suffer unsigned poisoning when it is promoted, so it
> would
> also fit in a u_int, but using u_int to store it is silly as using 1U
> instead
> of 1 for a count of 1.
>
> The bounds checking is something like:
>
> if (ap->uio_resid < 0)
> ap->uio_resid = 0;
> if (ap->a_ncookies != NULL) {
> if (ap->uio_resid >= 64 * 1024)
> ap->uio_resid = 64 * 1024;
> ncookies = ap->uio_resid;
> }
>
> This checks for negative values for all cases and converts to 0 (EOF) to
> preserve historical behaviour for the syscall case and to avoid overflow
> for the cookies case (in case the caller is buggy). The correct handling
> is to return EINVAL, but EOF is good enough.
>
> In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
> or corrupt it by assigning it to an int or u_int.
>
> Limit uio_resid from above only in the cookies case. The final limit
> should
> be about 128K (whatever nfs uses) or maybe 1M. Don't return EINVAL above
> the limit, since nfs probably wouldn't know how to handle that (by retrying
> with a smaller size). Test its handling of short counts instead. It is
> expected than nfs asks for 128K and we supply at most 64K. The supply is
> always reduced at EOF. Hopefully nfs doesn't treat the short count as EOF.
> It should retry until we supply 0.
>
> Hmm ...
We have never checked the upper bound there, which doesn't mean it was
right.
I found MAXPHYS, which seems a more reasonable limit used in the kernel for
uio_resid.
I am checking the patch compiles and doesn't give surprises.
MAXPHYS is almost the right thing to check. There's per device limits for
normal I/O, but that doesn't seem to be a strict limit for readdir.
Warner
Pedro.
After limiting uio_resid, assign it to the int ncookies.
>
> This doesn't fix the abuse of the ncookies counter to hold the size of the
> cookies array in bytes for this and the next couple of statements.
>
> Normally the bounds checking should be at the top level, with at most
> KASSERT()s at lower levels, but here the levels are mixed, and it isn't
> clear if kernel callers have already checked, and it doesn't cost much to
> do much the same checking for the kernel callers as for the syscall callers.
>
> Perhaps the 128K limit is good for all cases (this depends on callers not
> having buggy short count handling). Directories of this size are very
> rare (don't forget to create very large ones when you test this). Doing
> anything with directories of this size tends to be slow anyway, and the
> slowness has nothing to do with reading only 128K instead of SSIZE_MAX
> bytes at a time.
>
> readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
> in the unionfs case it seems to try to read the whole direction. It
> malloc()s the buffer in both cases. Blindy malloc()ing or mmap()ing
> a buffer large enough for a whole file or directory is no good, since
> in theory even directory sizes can be much larger than memory.
>
> Bruce
>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqq7eS5qMWVrG=U0aJJuNc%2BGmqvxa9=_0BbNr0tAqpx=Q>