Messages in this thread | | | Date | Mon, 23 Mar 2020 21:42:48 +0000 | From | Al Viro <> | Subject | Re: [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()?
| |