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

[Bruce Evans]

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.

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