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 <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. > > OK, I'll work that up then.
Well, apparently the annotations aren't useful with mutexes; see these threads: https://lore.kernel.org/all/8e8d93ee2125c739caabe5986f40fa2156c8b4ce.1579893447.git.jbi.oct...@gmail.com/ https://lore.kernel.org/all/20200601184552.23128-5-jbi.oct...@gmail.com/ And indeed I can't get sparse to accept them when added to the papr-vpd code: $ make C=2 arch/powerpc/platforms/pseries/papr-vpd.o CHECK scripts/mod/empty.c CALL scripts/checksyscalls.sh CHECK arch/powerpc/platforms/pseries/papr-vpd.c arch/powerpc/platforms/pseries/papr-vpd.c:235:13: warning: context imbalance in 'vpd_sequence_begin' - wrong count at exit arch/powerpc/platforms/pseries/papr-vpd.c:269:13: warning: context imbalance in 'vpd_sequence_end' - wrong count at exit I don't think it's my own mistake since I see existing code with the same problem, such as net/core/sock.c: static void *proto_seq_start(struct seq_file *seq, loff_t *pos) __acquires(proto_list_mutex) { mutex_lock(&proto_list_mutex); return seq_list_start_head(&proto_list, *pos); } static void proto_seq_stop(struct seq_file *seq, void *v) __releases(proto_list_mutex) { mutex_unlock(&proto_list_mutex); } which yields: net/core/sock.c:4018:13: warning: context imbalance in 'proto_seq_start' - wrong count at exit net/core/sock.c:4030:13: warning: context imbalance in 'proto_seq_stop' - wrong count at exit So I'll give up on static annotations for this series and look for opportunities to add lockdep_assert_held() etc.