| Message ID | 20230906120855.28331-1-msuchanek@suse.de (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] powerpc/rtas: Make it possible to disable sys_rtas | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | fail | boot (ppc64le_guest_defconfig, powernv+p8+tcg, powernv+p9+tcg, qemu-system-ppc64, ppc64le-rootfs.... failed at step Run qemu-powernv+p8+tcg with korg-5.5.0 build kernel. |
Michal Suchanek <msuchanek@suse.de> writes: > Additional patch suggestion to go with the rtas devices: > > ----------------------------------------------------------------------- > > With most important rtas functions available through different > interfaces the sys_rtas interface can be disabled completely. > > Do not remove it for now to make it possible to run older versions of > userspace tools that don't support other interfaces. Thanks. I hope making sys_rtas on/off-configurable will make sense eventually, and I expect this series to get us closer to that. But to me it seems too early and too coarse. A kernel built with RTAS_SYSCALL=n is not something I'd want to support or run in production soon. It would break too many known use cases, and likely some unknown ones as well. It could be more useful in the near term to construct a configurable list of RTAS functions that sys_rtas is allowed to expose. Something like: if PPC_RTAS config RTAS_SYSCALL_ALLOWS_SET_INDICATOR bool "sys_rtas allows calling set-indicator" default y config RTAS_SYSCALL_ALLOWS_GET_SENSOR_STATE bool "sys_rtas allows calling get-sensor-state" default y config RTAS_SYSCALL_ALLOWS_GET_VPD bool "sys_rtas allows calling ibm,get-vpd" default y ... etc etc endif Distro kernels could configure their allowed set of calls according to the capabilities of the user space components they ship, with the expectation that they will be able to shrink that set as user space adopts the preferred ABIs over time. That's just a sketch of an idea though, and I'm not sure it needs to be part of this series.
On Wed, Sep 06, 2023 at 02:34:59PM -0500, Nathan Lynch wrote: > Michal Suchanek <msuchanek@suse.de> writes: > > > Additional patch suggestion to go with the rtas devices: > > > > ----------------------------------------------------------------------- > > > > With most important rtas functions available through different > > interfaces the sys_rtas interface can be disabled completely. > > > > Do not remove it for now to make it possible to run older versions of > > userspace tools that don't support other interfaces. > > Thanks. I hope making sys_rtas on/off-configurable will make sense > eventually, and I expect this series to get us closer to that. But to me > it seems too early and too coarse. A kernel built with RTAS_SYSCALL=n is > not something I'd want to support or run in production soon. It would > break too many known use cases, and likely some unknown ones as well. There are about 3 known use cases that absolutely need access by other means than sys_rtas to work with lockdown, and about other 3 that would work either way. That's not so staggering that it could not be implemented in the kernel from the start. How long it will take for the known userspace users to catch up is anotehr questio but again it's something that can be addressed. Making it possible to turn off sys_rtas will make it easier to uncover the not yet known cases. What people want to support depends a lot on what is converted, and also the situation of the distribution in question. Fast-rollong distributions may want only the new interface quite soon, and so may distributions that are starting development of new release. All this makes sense only if there is a plan to discontinue sys_rtas entirely. For the simple calls that don't need data buffers it's still usable. > It could be more useful in the near term to construct a configurable > list of RTAS functions that sys_rtas is allowed to expose. If we really need this level of datail I guess it is too early. Thanks Michal
Michal Suchánek <msuchanek@suse.de> writes: > On Wed, Sep 06, 2023 at 02:34:59PM -0500, Nathan Lynch wrote: >> Michal Suchanek <msuchanek@suse.de> writes: >> >> > Additional patch suggestion to go with the rtas devices: >> > >> > ----------------------------------------------------------------------- >> > >> > With most important rtas functions available through different >> > interfaces the sys_rtas interface can be disabled completely. >> > >> > Do not remove it for now to make it possible to run older versions of >> > userspace tools that don't support other interfaces. >> >> Thanks. I hope making sys_rtas on/off-configurable will make sense >> eventually, and I expect this series to get us closer to that. But to me >> it seems too early and too coarse. A kernel built with RTAS_SYSCALL=n is >> not something I'd want to support or run in production soon. It would >> break too many known use cases, and likely some unknown ones as well. > > There are about 3 known use cases that absolutely need access by other > means than sys_rtas to work with lockdown, and about other 3 that would > work either way. How do you figure that? I count 11 librtas APIs that use work areas (and therefore /dev/mem) that are definitely broken under lockdown. Maybe a couple of them are unused. > That's not so staggering that it could not be implemented in the kernel > from the start. > How long it will take for the known userspace users to catch up is > anotehr questio but again it's something that can be addressed. > > Making it possible to turn off sys_rtas will make it easier to uncover > the not yet known cases. This is also true of making the configuration more granular than on or off. You would be free to disallow all calls if desired. > What people want to support depends a lot on what is converted, and also > the situation of the distribution in question. Fast-rollong > distributions may want only the new interface quite soon, and so may > distributions that are starting development of new release. > > All this makes sense only if there is a plan to discontinue sys_rtas > entirely. For the simple calls that don't need data buffers it's still > usable. I don't understand. How would it remain usable for the simple calls if it can be completely disabled? >> It could be more useful in the near term to construct a configurable >> list of RTAS functions that sys_rtas is allowed to expose. > > If we really need this level of datail I guess it is too early. I'm not sure we do, like I said it's just an idea at this point.
On Thu, Sep 07, 2023 at 11:52:44AM -0500, Nathan Lynch wrote: > Michal Suchánek <msuchanek@suse.de> writes: > > On Wed, Sep 06, 2023 at 02:34:59PM -0500, Nathan Lynch wrote: > >> Michal Suchanek <msuchanek@suse.de> writes: > >> > >> > Additional patch suggestion to go with the rtas devices: > >> > > >> > ----------------------------------------------------------------------- > >> > > >> > With most important rtas functions available through different > >> > interfaces the sys_rtas interface can be disabled completely. > >> > > >> > Do not remove it for now to make it possible to run older versions of > >> > userspace tools that don't support other interfaces. > >> > >> Thanks. I hope making sys_rtas on/off-configurable will make sense > >> eventually, and I expect this series to get us closer to that. But to me > >> it seems too early and too coarse. A kernel built with RTAS_SYSCALL=n is > >> not something I'd want to support or run in production soon. It would > >> break too many known use cases, and likely some unknown ones as well. > > > > There are about 3 known use cases that absolutely need access by other > > means than sys_rtas to work with lockdown, and about other 3 that would > > work either way. > > How do you figure that? I count 11 librtas APIs that use work areas (and > therefore /dev/mem) that are definitely broken under lockdown. Maybe a > couple of them are unused. I am referring to this list of known uses: https://github.com/ibm-power-utilities/librtas/issues/29 rtas_get_vpd is used by lsvpd (through libvpd and librtas) rtas_platform_dump and rtas_get_indices is used by ppc64-diag rtas_cfg_connector is used by powerpc-utils and is planned to be replaced by in-kernel solution That covers the complex cases. rtas_set_sysparm is used by ppc64-diag and powerpc-utils rtas_set_dynamic_indicator, rtas_get_dynamic_sensor are used by ppc64-diag (related to rtas_get_indices) rtas_errinjct + open/close are used by powerpc-utils That's the simple cases. Additional discussion here https://github.com/linuxppc/issues/issues/252 > > That's not so staggering that it could not be implemented in the kernel > > from the start. > > How long it will take for the known userspace users to catch up is > > anotehr questio but again it's something that can be addressed. > > > > Making it possible to turn off sys_rtas will make it easier to uncover > > the not yet known cases. > > This is also true of making the configuration more granular than on or > off. You would be free to disallow all calls if desired. > > > What people want to support depends a lot on what is converted, and also > > the situation of the distribution in question. Fast-rollong > > distributions may want only the new interface quite soon, and so may > > distributions that are starting development of new release. > > > > All this makes sense only if there is a plan to discontinue sys_rtas > > entirely. For the simple calls that don't need data buffers it's still > > usable. > > I don't understand. How would it remain usable for the simple calls if > it can be completely disabled? Either the goal is to completely remove sys_rtas, and then an option for disabling it is helpful for uncovering unexpected problems. Or thre isn't a goal of sys_rtas removal, and then there is no point in having an option to disable it, and it can be used for calls that don't need buffers indefinitely. Thanks Michal
>> > There are about 3 known use cases that absolutely need access by other >> > means than sys_rtas to work with lockdown, and about other 3 that would >> > work either way. >> >> How do you figure that? I count 11 librtas APIs that use work areas (and >> therefore /dev/mem) that are definitely broken under lockdown. Maybe a >> couple of them are unused. > > I am referring to this list of known uses: > > https://github.com/ibm-power-utilities/librtas/issues/29 > > rtas_get_vpd is used by lsvpd (through libvpd and librtas) > rtas_platform_dump and rtas_get_indices is used by ppc64-diag > rtas_cfg_connector is used by powerpc-utils and is planned to be > replaced by in-kernel solution > > That covers the complex cases. > > rtas_set_sysparm is used by ppc64-diag and powerpc-utils > rtas_set_dynamic_indicator, rtas_get_dynamic_sensor are used by > ppc64-diag (related to rtas_get_indices) > rtas_errinjct + open/close are used by powerpc-utils > > That's the simple cases. None of these would work "either way" with lockdown. They all use work area buffer arguments and must move away from sys_rtas and /dev/mem. The librtas issue you refer to makes that clear. >> > That's not so staggering that it could not be implemented in the kernel >> > from the start. >> > How long it will take for the known userspace users to catch up is >> > anotehr questio but again it's something that can be addressed. >> > >> > Making it possible to turn off sys_rtas will make it easier to uncover >> > the not yet known cases. >> >> This is also true of making the configuration more granular than on or >> off. You would be free to disallow all calls if desired. >> >> > What people want to support depends a lot on what is converted, and also >> > the situation of the distribution in question. Fast-rollong >> > distributions may want only the new interface quite soon, and so may >> > distributions that are starting development of new release. >> > >> > All this makes sense only if there is a plan to discontinue sys_rtas >> > entirely. For the simple calls that don't need data buffers it's still >> > usable. >> >> I don't understand. How would it remain usable for the simple calls if >> it can be completely disabled? > > Either the goal is to completely remove sys_rtas, and then an option for > disabling it is helpful for uncovering unexpected problems. Or thre > isn't a goal of sys_rtas removal, and then there is no point in having > an option to disable it, and it can be used for calls that don't need > buffers indefinitely. I don't agree that those are the only two options, but removal of sys_rtas is not going to be a goal of this series. The goal is to provide alternative ABIs that are compatible with lockdown.
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index eddc031c4b95..5854a8bb5731 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -1684,6 +1684,7 @@ noinstr struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log return NULL; } +#ifdef PPC_RTAS_SYSCALL /* * The sys_rtas syscall, as originally designed, allows root to pass * arbitrary physical addresses to RTAS calls. A number of RTAS calls @@ -1893,6 +1894,7 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) return 0; } +#endif static void __init rtas_function_table_init(void) { diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig index 1fd253f92a77..9563e38188d5 100644 --- a/arch/powerpc/platforms/Kconfig +++ b/arch/powerpc/platforms/Kconfig @@ -150,6 +150,15 @@ config RTAS_FLASH tristate "Firmware flash interface" depends on PPC64 && RTAS_PROC +config RTAS_SYSCALL + bool "Legacy syscall interface to RTAS" + depends on PPC_RTAS + default y + help + Enables support for the legacy sys_rtas interface. Calls that need to + pass data buffers use /dev/mem directly which is not compatible with + lockdown. For now some tools still need this interface to work. + config MMIO_NVRAM bool
Additional patch suggestion to go with the rtas devices: ----------------------------------------------------------------------- With most important rtas functions available through different interfaces the sys_rtas interface can be disabled completely. Do not remove it for now to make it possible to run older versions of userspace tools that don't support other interfaces. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- arch/powerpc/kernel/rtas.c | 2 ++ arch/powerpc/platforms/Kconfig | 9 +++++++++ 2 files changed, 11 insertions(+)