svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Warner Losh]

On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <pfg at 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
>
>