Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 27 Jan 2018 10:16:53 -0500
From:      Pedro Giffuni <pfg@FreeBSD.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        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:  <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org>
In-Reply-To: <20180126214948.C1040@besplex.bde.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>

next in thread | previous in thread | raw e-mail | index | archive | help


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.

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?11937120-bbb4-5da1-f48c-240a6aeafbd9>