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

[Nathan Lynch]

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

> Nathan Lynch <nathanl at linux.ibm.com> writes:
>
>> "Aneesh Kumar K.V (IBM)" <aneesh.kumar at kernel.org> writes:
>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com at 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.