| Message ID | 20260831065441.48654-2-nnmlinux@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc/eeh: Add RTAS-based error injection support on pSeries | expand |
On 31/08/26 12:24, Narayana Murty N wrote: > ibm,open-errinjct uses a non-standard RTAS return layout: > > rets[0] = session token (output parameter) > rets[1] = status code > > Unlike all other RTAS functions which use: > > rets[0] = status code > rets[1..] = output parameters > > Add rtas_token_is_open_errinjct() to identify this call, and > rtas_status_from_args() to extract status from the correct position. > > Add an early guard in rtas_call() that rejects ibm,open-errinjct > invocations where nret < 2, since reading rets[1] would be out of > bounds: > > if (rtas_token_is_open_errinjct(token) && nret < 2) { > WARN_ON_ONCE(1); > return RTAS_INVALID_PARAMETER; > } > > Adjust the output-copy loop so that for ibm,open-errinjct: > > return value = rets[1] (status) > outputs[0] = rets[0] (session token) > > For all other functions the existing convention is preserved: > > return value = rets[0] (status) > outputs[0..] = rets[1..] (non-status outputs) > > Move the "/* A -1 return code... */" comment immediately before the > ret == -1 check so it documents the check it guards. > > Also fix sys_rtas() last-error status detection: ibm,open-errinjct > places status at rets[1], so the -1 sentinel check must use rets[1] > for that function rather than always using rets[0]. > > Reference: OpenPOWER PAPR documentation > https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8 > Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com> > --- > arch/powerpc/kernel/rtas.c | 78 +++++++++++++++++++++++++++++++++----- > 1 file changed, 68 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 8d81c1e7a8db..7131870655c6 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -1117,6 +1117,28 @@ static bool token_is_restricted_errinjct(s32 token) > token == rtas_function_token(RTAS_FN_IBM_ERRINJCT); > } > > +/* > + * ibm,open-errinjct uses a non-standard return layout: > + * rets[0] = session token (output parameter) > + * rets[1] = status code > + * > + * All other RTAS functions use the standard layout: > + * rets[0] = status code > + * rets[1..] = output parameters > + */ > +static inline bool rtas_token_is_open_errinjct(int token) > +{ > + return token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT); > +} > + > +static int rtas_status_from_args(int token, struct rtas_args *args, int nret) > +{ > + if (rtas_token_is_open_errinjct(token)) > + return be32_to_cpu(args->rets[1]); > + > + return nret > 0 ? be32_to_cpu(args->rets[0]) : 0; > +} > + > /** > * rtas_call() - Invoke an RTAS firmware function. > * @token: Identifies the function being invoked. > @@ -1198,6 +1220,16 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) > return -1; > } > > + /* > + * ibm,open-errinjct returns rets[0]=session_token, rets[1]=status. > + * We need nret >= 2 to read status from rets[1]. Reject early if > + * the caller forgot to account for the extra return cell. > + */ > + if (rtas_token_is_open_errinjct(token) && nret < 2) { > + WARN_ON_ONCE(1); > + return RTAS_INVALID_PARAMETER; Nit: I would prefer -EINVAL instead. RTAS_INVALID_PARAMETER is RTAS error code but here kernel is validating the parameter so I think -EINVAL would be better. > + } > + > if ((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)) { > WARN_ON_ONCE(1); > return -1; > @@ -1213,15 +1245,33 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) > va_rtas_call_unlocked(args, token, nargs, nret, list); > va_end(list); > > + ret = rtas_status_from_args(token, args, nret); > + > /* 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) > + * be completed due to a hardware error. > + */ > + if (ret == -1) > buff_copy = __fetch_rtas_last_error(NULL); > > - if (nret > 1 && outputs != NULL) > - for (i = 0; i < nret-1; ++i) > - outputs[i] = be32_to_cpu(args->rets[i + 1]); > - ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0; > + /* > + * Copy non-status outputs to the caller's buffer. > + * > + * For ibm,open-errinjct the layout is: > + * rets[0] = session token -> outputs[0] > + * rets[1] = status (returned, not copied) > + * > + * For all other RTAS functions: > + * rets[0] = status (returned, not copied) > + * rets[1..nret-1] -> outputs[0..nret-2] > + */ > + if (outputs != NULL) { > + if (rtas_token_is_open_errinjct(token)) { > + outputs[0] = be32_to_cpu(args->rets[0]); > + } else if (nret > 1) { > + for (i = 0; i < nret - 1; ++i) > + outputs[i] = be32_to_cpu(args->rets[i + 1]); > + } > + } > > lockdep_unpin_lock(&rtas_lock, cookie); > raw_spin_unlock_irqrestore(&rtas_lock, flags); > @@ -1942,10 +1992,18 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > do_enter_rtas(&rtas_args); > args = rtas_args; > > - /* 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) > - errbuf = __fetch_rtas_last_error(buff_copy); > + /* > + * A -1 return code indicates that the last command couldn't > + * be completed due to a hardware error. ibm,open-errinjct > + * places status at rets[1] rather than rets[0]; check the > + * correct position for the -1 sentinel. > + */ > + { > + __be32 status_cell = (token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT) && > + nret >= 2) ? args.rets[1] : args.rets[0]; > + if (be32_to_cpu(status_cell) == -1) > + errbuf = __fetch_rtas_last_error(buff_copy); Do we know what happens when the ibm,open-errinjct RTAS call is made with nret < 2? The reason I’m asking is that, even with the above changes, args.rets[0] is used as the return code if the ibm,open-errinjct call is made with nret < 2. I like the approach you took in rtas_call() of pre-validating nret for the ibm,open-errinjct RTAS call and returning early if it is less than 2. I think we can use a similar approach here as well. If we do that, the above code changes will be much cleaner. In that case, we don't have to figure out how RTAS processes ibm,open-errinjct with nret < 2. The only concern I have is that this change would alter the system call behavior. Right now, the kernel accepts nret < 2 for ibm,open-errinjct and makes the RTAS call, but with the above suggested change, the kernel would return early if nret < 2. The prominent user of this system call is librtas, which passes nret = 2 for ibm,open-errinjct: https://github.com/ibm-power-utilities/librtas/blob/d321a1f5ae3d528ba027fc748d1cc1123dd4ae29/librtas_src/syscall_calls.c#L488 Also, as per PAPR, users are supposed to pass nret = 2 for this RTAS call. So I think it should be fine to validate nret in sys_rtas for ibm,open-errinjct and return early if it is found to be less than 2. Since this is a change in system call behavior, I want to be a little cautious. So, I’d like to hear your thoughts and would also like to know what others think about making the above change. Thanks, Sourabh Jain > + } > > lockdep_unpin_lock(&rtas_lock, cookie); > raw_spin_unlock_irqrestore(&rtas_lock, flags);
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 8d81c1e7a8db..7131870655c6 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -1117,6 +1117,28 @@ static bool token_is_restricted_errinjct(s32 token) token == rtas_function_token(RTAS_FN_IBM_ERRINJCT); } +/* + * ibm,open-errinjct uses a non-standard return layout: + * rets[0] = session token (output parameter) + * rets[1] = status code + * + * All other RTAS functions use the standard layout: + * rets[0] = status code + * rets[1..] = output parameters + */ +static inline bool rtas_token_is_open_errinjct(int token) +{ + return token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT); +} + +static int rtas_status_from_args(int token, struct rtas_args *args, int nret) +{ + if (rtas_token_is_open_errinjct(token)) + return be32_to_cpu(args->rets[1]); + + return nret > 0 ? be32_to_cpu(args->rets[0]) : 0; +} + /** * rtas_call() - Invoke an RTAS firmware function. * @token: Identifies the function being invoked. @@ -1198,6 +1220,16 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) return -1; } + /* + * ibm,open-errinjct returns rets[0]=session_token, rets[1]=status. + * We need nret >= 2 to read status from rets[1]. Reject early if + * the caller forgot to account for the extra return cell. + */ + if (rtas_token_is_open_errinjct(token) && nret < 2) { + WARN_ON_ONCE(1); + return RTAS_INVALID_PARAMETER; + } + if ((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)) { WARN_ON_ONCE(1); return -1; @@ -1213,15 +1245,33 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) va_rtas_call_unlocked(args, token, nargs, nret, list); va_end(list); + ret = rtas_status_from_args(token, args, nret); + /* 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) + * be completed due to a hardware error. + */ + if (ret == -1) buff_copy = __fetch_rtas_last_error(NULL); - if (nret > 1 && outputs != NULL) - for (i = 0; i < nret-1; ++i) - outputs[i] = be32_to_cpu(args->rets[i + 1]); - ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0; + /* + * Copy non-status outputs to the caller's buffer. + * + * For ibm,open-errinjct the layout is: + * rets[0] = session token -> outputs[0] + * rets[1] = status (returned, not copied) + * + * For all other RTAS functions: + * rets[0] = status (returned, not copied) + * rets[1..nret-1] -> outputs[0..nret-2] + */ + if (outputs != NULL) { + if (rtas_token_is_open_errinjct(token)) { + outputs[0] = be32_to_cpu(args->rets[0]); + } else if (nret > 1) { + for (i = 0; i < nret - 1; ++i) + outputs[i] = be32_to_cpu(args->rets[i + 1]); + } + } lockdep_unpin_lock(&rtas_lock, cookie); raw_spin_unlock_irqrestore(&rtas_lock, flags); @@ -1942,10 +1992,18 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) do_enter_rtas(&rtas_args); args = rtas_args; - /* 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) - errbuf = __fetch_rtas_last_error(buff_copy); + /* + * A -1 return code indicates that the last command couldn't + * be completed due to a hardware error. ibm,open-errinjct + * places status at rets[1] rather than rets[0]; check the + * correct position for the -1 sentinel. + */ + { + __be32 status_cell = (token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT) && + nret >= 2) ? args.rets[1] : args.rets[0]; + if (be32_to_cpu(status_cell) == -1) + errbuf = __fetch_rtas_last_error(buff_copy); + } lockdep_unpin_lock(&rtas_lock, cookie); raw_spin_unlock_irqrestore(&rtas_lock, flags);
ibm,open-errinjct uses a non-standard RTAS return layout: rets[0] = session token (output parameter) rets[1] = status code Unlike all other RTAS functions which use: rets[0] = status code rets[1..] = output parameters Add rtas_token_is_open_errinjct() to identify this call, and rtas_status_from_args() to extract status from the correct position. Add an early guard in rtas_call() that rejects ibm,open-errinjct invocations where nret < 2, since reading rets[1] would be out of bounds: if (rtas_token_is_open_errinjct(token) && nret < 2) { WARN_ON_ONCE(1); return RTAS_INVALID_PARAMETER; } Adjust the output-copy loop so that for ibm,open-errinjct: return value = rets[1] (status) outputs[0] = rets[0] (session token) For all other functions the existing convention is preserved: return value = rets[0] (status) outputs[0..] = rets[1..] (non-status outputs) Move the "/* A -1 return code... */" comment immediately before the ret == -1 check so it documents the check it guards. Also fix sys_rtas() last-error status detection: ibm,open-errinjct places status at rets[1], so the -1 sentinel check must use rets[1] for that function rather than always using rets[0]. Reference: OpenPOWER PAPR documentation https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8 Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com> --- arch/powerpc/kernel/rtas.c | 78 +++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 10 deletions(-)