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? 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; }