RE: [PATCH] Convert filldir[64]() from __put_user() to unsafe_put_user()

[David Laight]

From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Sent: 07 October 2019 19:11
...
> I've been very close to just removing __get_user/__put_user several
> times, exactly because people do completely the wrong thing with them
> - not speeding code up, but making it unsafe and buggy.

They could do the very simple check that 'user_ptr+size < kernel_base'
rather than the full window check under the assumption that access_ok()
has been called and that the likely errors are just overruns.

> The new "user_access_begin/end()" model is much better, but it also
> has actual STATIC checking that there are no function calls etc inside
> the region, so it forces you to do the loop properly and tightly, and
> not the incorrect "I checked the range somewhere else, now I'm doing
> an unsafe copy".
> 
> And it actually speeds things up, unlike the access_ok() games.

I've code that does:
	if (!access_ok(...))
		return -EFAULT;
	...
	for (...) {
		if (__get_user(tmp_u64, user_ptr++))
			return -EFAULT;
		writeq(tmp_u64, io_ptr++);
	}
(Although the code is more complex because not all transfers are multiples of 8 bytes.)

With user_access_begin/end() I'd probably want to put the copy loop
inside a function (which will probably get inlined) to avoid convoluted
error processing.
So you end up with:
	if (!user_access_ok())
		return _EFAULT;
	user_access_begin();
	rval = do_copy_code(...);
	user_access_end();
	return rval;
Which, at the source level (at least) breaks your 'no function calls' rule.
The writeq() might also break it as well.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)