Nathan Lynch <nath...@linux.ibm.com> writes: > Michael Ellerman <m...@ellerman.id.au> writes: >> Nathan Lynch <nath...@linux.ibm.com> writes: >>> Michael Ellerman <m...@ellerman.id.au> writes: >>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm....@kernel.org> >>>> writes: >>>>> From: Nathan Lynch <nath...@linux.ibm.com> >>>>> >>>>> On RTAS platforms there is a general restriction that the OS must not >>>>> enter RTAS on more than one CPU at a time. This low-level >>>>> serialization requirement is satisfied by holding a spin >>>>> lock (rtas_lock) across most RTAS function invocations. >>>> ... >>>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c >>>>> index 1fc0b3fffdd1..52f2242d0c28 100644 >>>>> --- a/arch/powerpc/kernel/rtas.c >>>>> +++ b/arch/powerpc/kernel/rtas.c >>>>> @@ -581,6 +652,28 @@ static const struct rtas_function >>>>> *rtas_token_to_function(s32 token) >>>>> return NULL; >>>>> } >>>>> >>>>> +static void __rtas_function_lock(struct rtas_function *func) >>>>> +{ >>>>> + if (func && func->lock) >>>>> + mutex_lock(func->lock); >>>>> +} >>>> >>>> This is obviously going to defeat most static analysis tools. >>> >>> I guess it's not that obvious to me :-) Is it because the mutex_lock() >>> is conditional? I'll improve this if it's possible. >> >> Well maybe I'm not giving modern static analysis tools enough credit :) >> >> But what I mean that it's not easy to reason about what the function >> does in isolation. ie. all you can say is that it may or may not lock a >> mutex, and you can't say which mutex. > > I've pulled the thread on this a little bit and here is what I can do: > > * Discard rtas_lock_function() and rtas_unlock_function() and make the > function mutexes extern as needed. As of now only > rtas_ibm_get_vpd_lock will need to be exposed. This enables us to put > __acquires(), __releases(), and __must_hold() annotations in > papr-vpd.c since it explicitly manipulates the mutex. > > * Then sys_rtas() becomes the only site that needs > __rtas_function_lock() and __rtas_function_unlock(), which can be > open-coded and commented (and, one hopes, not emulated elsewhere). > > This will look something like: > > SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > { > struct rtas_function *func = rtas_token_to_function(token); > > if (func->lock) > mutex_lock(func->lock); > > [ ... acquire rtas_lock, enter RTAS, fetch any errors ... ] > > if (func->lock) > mutex_unlock(func->lock); > > The indirection seems unavoidable since we're working backwards from a > token value (supplied by the user and not known at build time) to the > function descriptor. > > Is that tolerable for now?
Yeah. Thanks for looking into it. I wasn't unhappy with the original version, but just slightly uneasy about the locking via pointer. But that new proposal sounds good, more code will have static lock annotations, and only sys_rtas() which is already weird, will have the dynamic stuff. > Alternatively, sys_rtas() could be refactored into locking and > non-locking paths, e.g. > > static long __do_sys_rtas(struct rtas_function *func) > { > // [ ... acquire rtas_lock, enter RTAS, fetch any error etc ... ] > } > > static long do_sys_rtas(struct rtas_function *func, struct mutex *mtx) > { > mutex_lock(mtx); > ret = __do_sys_rtas(func); > mutex_unlock(mtx); > > return ret; > } > > SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > { > // real code does copy_from_user etc > struct rtas_function *func = rtas_token_to_function(uargs->token); > long ret; > > // [ ... input validation and filtering ... ] > > if (func->lock) > ret = do_sys_rtas(func, func->lock); > else > ret = __do_sys_rtas(func); > > // [ ... copy out results ... ] > > return ret; > } You could go even further and switch on the token, and handle each case separately so that you can then statically take the appropriate lock. But that's probably overkill. cheers