Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight <david.laight.linux@gmail.com>]

On Sun, 9 Feb 2025 09:40:05 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sun, 9 Feb 2025 at 02:56, David Laight <david.laight.linux@gmail.com> wrote:
> >
> > Code can then be changed:
> > -               if (!user_read_access_begin(from, sizeof(*from)))
> > +               if (!masked_user_read_access_begin(&from, sizeof(*from)))
> >                         return -EFAULT;  
> 
> I really dislike the use of "pass pointer to simple variable you are
> going to change" interfaces which is why I didn't do it this way.

For real functions they do generate horrid code.
But since this is a define the *&from is optimised away.
Definitely better than just passing 'from' and having it unexpectedly
changed (why did C++ allow that one?).

> It's actually one of my least favorite parts of C - and one of the
> things that Rust got right - because the whole "error separate from
> return value" is such an important thing for graceful error handling.

Especially since the ABI almost always let a register pair be returned.
Shame it can't be used for anything other than double-length integers.

> And it's also why we use error pointers in the kernel: because I
> really *hated* all the cases where people were returning separate
> errors and results. The kernel error pointer thing may seem obvious
> and natural to people now, but it was a fairly big change at the time.

I've a lurking plan to change getsockopt() to return error/length and
move all the user copies into the syscall wrapper.
Made more complicated by code that wants to return an error and a length.
(They'll probably need packing into a single value that is negative - so
that the decode is in the slow path.)

> I'd actually much prefer the "goto efault" model that
> "unsafe_get/put_user()" uses than passing in the other return value
> with a pointer.
> 
> I wish we had a good error handling model.
> 
> Not the async crap that is exceptions with try/catch (or
> setjmp/longjmp - the horror). Just nice synchronous error handling
> that doesn't require the whole "hide return value as a in-out
> argument".
> 
> I know people think 'goto' and labels are bad. But I seriously think
> they are better and more legible constructs than the 'return value in
> arguments'.

I've had to fix some 'day-job' code which had repeated 'if (!error)'
clauses to avoid early return (never mind goto).
Typically at least one path got the error handling wrong.
At least explicit 'return error' or 'goto return_error' are easy
to validate.

> 
> Yes, you can make spaghetti code with goto and labels. But 'return
> value in arguments' is worse, because it makes the *data flow* harder
> to see.

Hidden returns are a real nightmare - you can't even guess whether any
locking (etc) is done.
At least hidden goto are visible.

Let me see if I can to a 'hidden goto' version.

	David

> 
>           Linus