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

[Nathan Lynch]

Nathan Lynch <nathanl at linux.ibm.com> writes:
> Michael Ellerman <mpe at ellerman.id.au> writes:
>> Nathan Lynch <nathanl at linux.ibm.com> writes:
>>> Michael Ellerman <mpe at ellerman.id.au> writes:
>>>> Nathan Lynch <nathanl at linux.ibm.com> writes:
>>>>> Michael Ellerman <mpe at ellerman.id.au> writes:
>>>>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com at kernel.org>
>>>>>> writes:
>>>>>>> From: Nathan Lynch <nathanl at 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

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.