[v4,06/13] powerpc/rtas: Serialize firmware activation sequences

From: Nathan Lynch <nathanl@linux.ibm.com>

Use the function lock API to prevent interleaving call sequences of
the ibm,activate-firmware RTAS function, which typically requires
multiple calls to complete the update. While the spec does not
specifically prohibit interleaved sequences, there's almost certainly
no advantage to allowing them.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 4 ++++
 1 file changed, 4 insertions(+)

Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
writes:

> From: Nathan Lynch <nathanl@linux.ibm.com>
>
> Use the function lock API to prevent interleaving call sequences of
> the ibm,activate-firmware RTAS function, which typically requires
> multiple calls to complete the update. While the spec does not
> specifically prohibit interleaved sequences, there's almost certainly
> no advantage to allowing them.
>

Can we document what is the equivalent thing the userspace does? 

Reviewed-by: Aneesh Kumar K.V (IBM) <aneesh.kumar@kernel.org>

> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 52f2242d0c28..e38ba05ad613 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -1753,10 +1753,14 @@ void rtas_activate_firmware(void)
>  		return;
>  	}
>  
> +	rtas_function_lock(RTAS_FN_IBM_ACTIVATE_FIRMWARE);
> +
>  	do {
>  		fwrc = rtas_call(token, 0, 1, NULL);
>  	} while (rtas_busy_delay(fwrc));
>  
> +	rtas_function_unlock(RTAS_FN_IBM_ACTIVATE_FIRMWARE);
> +
>  	if (fwrc)
>  		pr_err("ibm,activate-firmware failed (%i)\n", fwrc);
>  }
>
> -- 
> 2.41.0

"Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:
> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
> writes:
>
>>
>> Use the function lock API to prevent interleaving call sequences of
>> the ibm,activate-firmware RTAS function, which typically requires
>> multiple calls to complete the update. While the spec does not
>> specifically prohibit interleaved sequences, there's almost certainly
>> no advantage to allowing them.
>>
>
> Can we document what is the equivalent thing the userspace does?

I'm not sure what we would document.

As best I can tell, the activate_firmware command in powerpc-utils does
not make any effort to protect its use of the ibm,activate-firmware RTAS
function. The command is not intended to be run manually and I guess
it's relying on the platform's management console to serialize its
invocations.

drmgr (also from powerpc-utils) has some dead code for LPM that calls
ibm,activate-firmware; it should probably be removed. The command uses a
lock file to serialize all of its executions.

Something that could happen with interleaved ibm,activate-firmware
sequences is something like this:

1. Process A initiates an ibm,activate-firmware sequence and receives a
   "retry" status (-2/990x).
2. Process B calls ibm,activate-firmware and receives the "done" status
   (0), concluding the sequence A began.
3. Process A, unaware of B, calls ibm,activate-firmware again,
   inadvertently beginning a new sequence.

Seems mostly benign to me except that process A could fail to make
progress indefinitely under the right circumstances.

Nathan Lynch <nathanl@linux.ibm.com> writes:

> "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:
>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
>> writes:
>>
>>>
>>> Use the function lock API to prevent interleaving call sequences of
>>> the ibm,activate-firmware RTAS function, which typically requires
>>> multiple calls to complete the update. While the spec does not
>>> specifically prohibit interleaved sequences, there's almost certainly
>>> no advantage to allowing them.
>>>
>>
>> Can we document what is the equivalent thing the userspace does?
>
> I'm not sure what we would document.
>
> As best I can tell, the activate_firmware command in powerpc-utils does
> not make any effort to protect its use of the ibm,activate-firmware RTAS
> function. The command is not intended to be run manually and I guess
> it's relying on the platform's management console to serialize its
> invocations.
>
> drmgr (also from powerpc-utils) has some dead code for LPM that calls
> ibm,activate-firmware; it should probably be removed. The command uses a
> lock file to serialize all of its executions.
>
> Something that could happen with interleaved ibm,activate-firmware
> sequences is something like this:
>
> 1. Process A initiates an ibm,activate-firmware sequence and receives a
>    "retry" status (-2/990x).
> 2. Process B calls ibm,activate-firmware and receives the "done" status
>    (0), concluding the sequence A began.
> 3. Process A, unaware of B, calls ibm,activate-firmware again,
>    inadvertently beginning a new sequence.
>

So this patch won't protect us against a parallel userspace invocation.
We can add static bool call_in_progress to track the ongoing
ibm,activate-firmware call from userspace? My only concern is we are
adding locks to protect against parallel calls in the kernel, but at the
same time, we ignore any userspace call regarding the same. We should at
least document this if this is not important to be fixed.

-aneesh

"Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:

> Nathan Lynch <nathanl@linux.ibm.com> writes:
>
>> "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:
>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
>>> writes:
>>>
>>>>
>>>> Use the function lock API to prevent interleaving call sequences of
>>>> the ibm,activate-firmware RTAS function, which typically requires
>>>> multiple calls to complete the update. While the spec does not
>>>> specifically prohibit interleaved sequences, there's almost certainly
>>>> no advantage to allowing them.
>>>>
>>>
>>> Can we document what is the equivalent thing the userspace does?
>>
>> I'm not sure what we would document.
>>
>> As best I can tell, the activate_firmware command in powerpc-utils does
>> not make any effort to protect its use of the ibm,activate-firmware RTAS
>> function. The command is not intended to be run manually and I guess
>> it's relying on the platform's management console to serialize its
>> invocations.
>>
>> drmgr (also from powerpc-utils) has some dead code for LPM that calls
>> ibm,activate-firmware; it should probably be removed. The command uses a
>> lock file to serialize all of its executions.
>>
>> Something that could happen with interleaved ibm,activate-firmware
>> sequences is something like this:
>>
>> 1. Process A initiates an ibm,activate-firmware sequence and receives a
>>    "retry" status (-2/990x).
>> 2. Process B calls ibm,activate-firmware and receives the "done" status
>>    (0), concluding the sequence A began.
>> 3. Process A, unaware of B, calls ibm,activate-firmware again,
>>    inadvertently beginning a new sequence.
>>
>
> So this patch won't protect us against a parallel userspace
> invocation.

It does protect in-kernel sequences from disruption by sys_rtas-based
sequences. Patch 5/13 "Facilitate high-level call sequences" makes it so
sys_rtas-based invocations of ibm,activate-firmware acquire
rtas_ibm_activate_firmware_lock.

> We can add static bool call_in_progress to track the ongoing
> ibm,activate-firmware call from userspace?

We can't reliably maintain any such state in the kernel. A user of
sys_rtas could exit with a sequence in progress, or it could simply
decline to complete a sequence it has initiated for any reason. This is
one of the fundamental problems with directly exposing more complex RTAS
functions to user space.

> My only concern is we are adding locks to protect against parallel
> calls in the kernel, but at the same time, we ignore any userspace
> call regarding the same. We should at least document this if this is
> not important to be fixed.

It's not accurate to say we're ignoring user space calls. Patch 5/13
makes it so that sys_rtas(ibm,activate-firmware) will serialize on the
same lock used here.

Nathan Lynch <nathanl@linux.ibm.com> writes:
> "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:
>
>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>
>>> "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes:
>>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org>
>>>> writes:
>>>>
>>>>>
>>>>> Use the function lock API to prevent interleaving call sequences of
>>>>> the ibm,activate-firmware RTAS function, which typically requires
>>>>> multiple calls to complete the update. While the spec does not
>>>>> specifically prohibit interleaved sequences, there's almost certainly
>>>>> no advantage to allowing them.
>>>>>
>>>>
>>>> Can we document what is the equivalent thing the userspace does?
>>>
>>> I'm not sure what we would document.
>>>
>>> As best I can tell, the activate_firmware command in powerpc-utils does
>>> not make any effort to protect its use of the ibm,activate-firmware RTAS
>>> function. The command is not intended to be run manually and I guess
>>> it's relying on the platform's management console to serialize its
>>> invocations.
>>>
>>> drmgr (also from powerpc-utils) has some dead code for LPM that calls
>>> ibm,activate-firmware; it should probably be removed. The command uses a
>>> lock file to serialize all of its executions.
>>>
>>> Something that could happen with interleaved ibm,activate-firmware
>>> sequences is something like this:
>>>
>>> 1. Process A initiates an ibm,activate-firmware sequence and receives a
>>>    "retry" status (-2/990x).
>>> 2. Process B calls ibm,activate-firmware and receives the "done" status
>>>    (0), concluding the sequence A began.
>>> 3. Process A, unaware of B, calls ibm,activate-firmware again,
>>>    inadvertently beginning a new sequence.
>>>
>>
>> So this patch won't protect us against a parallel userspace
>> invocation.
>
> It does protect in-kernel sequences from disruption by sys_rtas-based
> sequences. Patch 5/13 "Facilitate high-level call sequences" makes it so
> sys_rtas-based invocations of ibm,activate-firmware acquire
> rtas_ibm_activate_firmware_lock.
>
>> We can add static bool call_in_progress to track the ongoing
>> ibm,activate-firmware call from userspace?
>
> We can't reliably maintain any such state in the kernel. A user of
> sys_rtas could exit with a sequence in progress, or it could simply
> decline to complete a sequence it has initiated for any reason. This is
> one of the fundamental problems with directly exposing more complex RTAS
> functions to user space.

That said, I should resurrect "powerpc/rtas: consume retry statuses in
sys_rtas()":

https://lore.kernel.org/linuxppc-dev/20230220-rtas-queue-for-6-4-v1-8-010e4416f13f@linux.ibm.com/

That ought to have the effect of perfectly serializing all
ibm,activate-firmware sequences regardless of how they're initiated.

But I'd like to leave that until later instead of adding to this series.