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

[Nathan Lynch]

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?

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