| Message ID | 20231013-papr-sys_rtas-vs-lockdown-v2-3-ead01ce01722@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/pseries: new character devices for system parameters and VPD | expand |
> Take the papr-vpd driver's internal mutex when sys_rtas performs > ibm,get-vpd calls. This prevents sys_rtas(ibm,get-vpd) calls from > interleaving with sequences performed by the driver, ensuring that > such sequences are not disrupted. .... > @@ -1861,6 +1862,28 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > goto copy_return; > } > > + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) { > + /* > + * ibm,get-vpd potentially needs to be invoked > + * multiple times to obtain complete results. > + * Interleaved ibm,get-vpd sequences disrupt each > + * other. > + * > + * /dev/papr-vpd doesn't have this problem and users > + * do not need to be aware of each other to use it > + * safely. > + * > + * We can prevent this call from disrupting a > + * /dev/papr-vpd-initiated sequence in progress by > + * reaching into the driver to take its internal > + * lock. Unfortunately there is no way to prevent > + * interference in the other direction without > + * resorting to even worse hacks. > + */ > + pr_notice_once("Calling ibm,get-vpd via sys_rtas is allowed but deprecated. Use /dev/papr-vpd instead.\n"); > + papr_vpd_mutex_lock(); > + } > + > buff_copy = get_errorlog_buffer(); > > raw_spin_lock_irqsave(&rtas_lock, flags); > @@ -1870,6 +1893,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > do_enter_rtas(&rtas_args); > args = rtas_args; > > + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) > + papr_vpd_mutex_unlock(); > + The mutex ought to nest entirely outside rtas_lock, this releases it too early. Anyway, I'm considering a different way to get the synchronization right between drivers like papr-vpd and sys_rtas. Instead of having sys_rtas acquire the driver's internal lock, rtas.c should provide a way for code like papr-vpd to temporarily lock the syscall path. Something like this: // rtas.c + static DEFINE_MUTEX(rtas_syscall_lock); + void rtas_syscall_lock(void) { mutex_lock(&rtas_syscall_lock); } + void rtas_syscall_unlock(void) { mutex_unlock(&rtas_syscall_lock); } SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) { + rtas_syscall_lock(); ... do_enter_rtas(&rtas_args); ... + rtas_syscall_unlock(); return 0; } // papr-vpd.c static void vpd_sequence_begin(struct vpd_sequence *seq, const struct papr_location_code *loc_code) { static struct papr_location_code static_loc_code; papr_vpd_mutex_lock(); + rtas_syscall_lock(); static_loc_code = *loc_code; *seq = (struct vpd_sequence) { .params = { .work_area = rtas_work_area_alloc(SZ_4K), .loc_code = &static_loc_code, .sequence = 1, }, }; } /** * vpd_sequence_end() - Finalize a VPD retrieval sequence. * @seq: Sequence state. * * Releases resources obtained by vpd_sequence_begin(). */ static void vpd_sequence_end(struct vpd_sequence *seq) { rtas_work_area_free(seq->params.work_area); + rtas_syscall_unlock(); papr_vpd_mutex_unlock(); } This is a sketch to communicate the idea. The locking in the real code could be finer, perhaps a mutex per RTAS function.
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index eddc031c4b95..70ae118d2a13 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -37,6 +37,7 @@ #include <asm/machdep.h> #include <asm/mmu.h> #include <asm/page.h> +#include <asm/papr-vpd.h> #include <asm/rtas-work-area.h> #include <asm/rtas.h> #include <asm/time.h> @@ -1861,6 +1862,28 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) goto copy_return; } + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) { + /* + * ibm,get-vpd potentially needs to be invoked + * multiple times to obtain complete results. + * Interleaved ibm,get-vpd sequences disrupt each + * other. + * + * /dev/papr-vpd doesn't have this problem and users + * do not need to be aware of each other to use it + * safely. + * + * We can prevent this call from disrupting a + * /dev/papr-vpd-initiated sequence in progress by + * reaching into the driver to take its internal + * lock. Unfortunately there is no way to prevent + * interference in the other direction without + * resorting to even worse hacks. + */ + pr_notice_once("Calling ibm,get-vpd via sys_rtas is allowed but deprecated. Use /dev/papr-vpd instead.\n"); + papr_vpd_mutex_lock(); + } + buff_copy = get_errorlog_buffer(); raw_spin_lock_irqsave(&rtas_lock, flags); @@ -1870,6 +1893,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) do_enter_rtas(&rtas_args); args = rtas_args; + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) + papr_vpd_mutex_unlock(); + /* A -1 return code indicates that the last command couldn't be completed due to a hardware error. */ if (be32_to_cpu(args.rets[0]) == -1)