Re: [PATCH v4 05/13] powerpc/rtas: Facilitate high-level call sequences

[Michael Ellerman <mpe@ellerman.id.au>]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>>>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>>>>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>>>>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
>>>>>>> writes:
>>>>>>>> From: Nathan Lynch <nathanl@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.octave@gmail.com/
> https://lore.kernel.org/all/20200601184552.23128-5-jbi.octave@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

Oof. Sorry to send you on that goose chase.

> 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.

OK. There are other static analysers than sparse, and even for lockdep
having more of the locks taken statically is probably still preferable.

But feel free to ignore me and just repost what you had :)

cheers