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

> 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