On Fri, Jun 18, 2021 at 02:10:36PM -0700, Linus Torvalds wrote:
> > Just put the size of the encoded part first and be done with that.
> > Magical effect of the iovec sizes is a bloody bad idea.
>
> That makes everything uglier and more complicated, honestly. Then
> you'd have to do it in _two_ operations ("get the size, then get the
> rest"), *AND* you'd have to worry about all the corner-cases (ie
> people putting the structure in pieces across multiple iov entries.
Huh? All corner cases are already taken care of by copy_from_iter{,_full}().
What I'm proposing is to have the size as a field in 'encoded' and
do this
if (!copy_from_iter_full(&encoded, sizeof(encoded), &i))
return -EFAULT;
if (encoded.size > sizeof(encoded)) {
// newer than what we expect
if (!iov_iter_check_zeroes(&i, encoded.size - sizeof(encoded))
return -EINVAL;
} else if (encoded.size < sizeof(encoded)) {
// older than what we expect
iov_iter_revert(&i, sizeof(encoded) - encoded.size);
memset((void *)&encoded + encoded.size, 0, sizoef(encoded) - encoded.size);
}
I don't think it would be more complex, but that's a matter of taste;
I *really* doubt it would be any slower or have higher odds of bugs,
regardless of the corner cases.
And it certainly would be much smaller on the lib/iov_iter.c side -
implementation of iov_iter_check_zeroes() would be simply this:
bool iov_iter_check_zeroes(struct iov_iter *i, size_t size)
{
bool failed = false;
iterate_and_advance(i, bytes, base, len, off,
failed = (check_zeroed_user(base, len) != 1),
failed = (memchr_inv(base, 0, len) != NULL))
if (unlikely(failed))
iov_iter_revert(i, bytes);
return !failed;
}
And that's it, no need to do anything special for xarray, etc.
This + EXPORT_SYMBOL + extern in uio.h + snippet above in the
user...
I could buy an argument that for userland the need to add
encoded.size = sizeof(encoded);
or equivalent when initializing that thing would make life too complex,
but on the kernel side I'd say that Omar's variant is considerably more
complex than the above...
