Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers

From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	the arch/x86 maintainers <x86@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers
Date: Mon, 23 Mar 2020 21:42:48 +0000	[thread overview]
Message-ID: <20200323214248.GF23230@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CAHk-=wgQjm2=Z6e9ZLffsNmnc_e2wz_W3SYTD2_EXZT7yYYbRA@mail.gmail.com>

On Mon, Mar 23, 2020 at 11:53:39AM -0700, Linus Torvalds wrote:
> On Mon, Mar 23, 2020 at 11:39 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > -static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> > +static __always_inline int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> 
> Please rename this at the same time (to "unsafe_ia32_setup_sigcontext()").
> 
> I absolutely _hate_ how we have historically split the "__get_user()"
> calls from the "access_ok()" calls, and then have had bugs when we had
> ways to reach the user access without checking it.
> 
> Yes, we have static checking for the unsafe stuff in objtool now, but
> I still want this to be explicit on the source level too: if you do
> unsafe user accesses, you make it very very explicit in the naming, so
> that you can't possibly even by mistake have a "let's call this
> function withou having done the user_access_begin()" calls.

Umm...  OK, but I wonder if unsafe_... makes the right naming conventions
for such cases.  Note that towards the end of that series we get

#define unsafe_put_sigcontext(sc, fpstate, regs, set, label)    \
do {                                                            \
        if (setup_sigcontext(sc, fpstate, regs, set->sig[0]))   \
                goto label;                                     \
} while(0);

and that's not an uncommon pattern.  We generally have unsafe_...
mean "doesn't return anything, takes a label, needs to be called
from under user_access_begin" and I suspect that it would make sense
to have another recognizable naming pattern for "it must be called
from under user_access_begin() and you need to look at return value".

In this case we could grit teeth and turn that sucker into a macro.
But what about e.g. lifting user_access_{begin,end}() out of
raw_copy_from_user()?  unsafe_copy_from_user() would imply
"all or nothing" kind of calling conventions, like e.g.
unsafe_copy_to_user() currently does.  Which is fine in some
situations, and it's a good helper to have, but we definitely
want a "how much is left to copy" variant as well.

Hmm...  raw_setup_sigcontext(), perhaps, along with the
macro above for unsafe_put_sigcontext()?