lkml.org 
[lkml]   [2020]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers
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()?

\
 
 \ /
  Last update: 2020-03-23 22:43    [W:0.075 / U:5.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site