Re: [PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
Nathan Lynch writes: > "Aneesh Kumar K.V (IBM)" writes: > >> Nathan Lynch writes: >> >>> "Aneesh Kumar K.V (IBM)" writes: Nathan Lynch via B4 Relay 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-010e4416f...@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.
Re: [PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
"Aneesh Kumar K.V (IBM)" writes: > Nathan Lynch writes: > >> "Aneesh Kumar K.V (IBM)" writes: >>> Nathan Lynch via B4 Relay >>> 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.
Re: [PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
Nathan Lynch writes: > "Aneesh Kumar K.V (IBM)" writes: >> Nathan Lynch via B4 Relay >> 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
Re: [PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
"Aneesh Kumar K.V (IBM)" writes: > Nathan Lynch via B4 Relay > 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.
Re: [PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
Nathan Lynch via B4 Relay
writes:
> From: Nathan Lynch
>
> 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)
> Signed-off-by: Nathan Lynch
> ---
> 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
[PATCH v4 06/13] powerpc/rtas: Serialize firmware activation sequences
From: Nathan Lynch
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
---
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
