Re: [PATCH v2 1/7] powerpc: properly negate error in syscall_set_return_value()

[Christophe Leroy]

Le 21/01/2025 à 12:13, Madhavan Srinivasan a écrit :
> On 1/20/25 10:42 PM, Dmitry V. Levin wrote:
> > On Mon, Jan 20, 2025 at 02:51:38PM +0100, Christophe Leroy wrote:
> > > Le 14/01/2025 à 18:04, Dmitry V. Levin a écrit :
> > > > On Mon, Jan 13, 2025 at 06:34:44PM +0100, Christophe Leroy wrote:
> > > > > Le 13/01/2025 à 18:10, Dmitry V. Levin a écrit :
> > > > > > Bring syscall_set_return_value() in sync with syscall_get_error(),
> > > > > > and let upcoming ptrace/set_syscall_info selftest pass on powerpc.
>
> Sorry for getting to this thread late.
>
> Tried the series without this patch in
>
> 1) power9 PowerNV system and in power10 pSeries lpar
>
> # ./set_syscall_info
> TAP version 13
> 1..1
> # Starting 1 tests from 1 test cases.
> #  RUN           global.set_syscall_info ...
> #            OK  global.set_syscall_info
> ok 1 global.set_syscall_info
> # PASSED: 1 / 1 tests passed.
> # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> and in both case set_syscall_info passes.
> Will look at it further.

I guess it works because power9/10 are using scv not sc for system call, 
hence using the new ABI ?

Christophe

> Maddy
>
> > > > > > This reverts commit 1b1a3702a65c ("powerpc: Don't negate error in
> > > > > > syscall_set_return_value()").
> > > > >
> > > > > There is a clear detailed explanation in that commit of why it needs to
> > > > > be done.
> > > > >
> > > > > If you think that commit is wrong you have to explain why with at least
> > > > > the same level of details.
> > > >
> > > > OK, please have a look whether this explanation is clear and detailed enough:
> > > >
> > > > =======
> > > > powerpc: properly negate error in syscall_set_return_value()
> > > >
> > > > When syscall_set_return_value() is used to set an error code, the caller
> > > > specifies it as a negative value in -ERRORCODE form.
> > > >
> > > > In !trap_is_scv case the error code is traditionally stored as follows:
> > > > gpr[3] contains a positive ERRORCODE, and ccr has 0x10000000 flag set.
> > > > Here are a few examples to illustrate this convention.  The first one
> > > > is from syscall_get_error():
> > > >           /*
> > > >            * If the system call failed,
> > > >            * regs->gpr[3] contains a positive ERRORCODE.
> > > >            */
> > > >           return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
> > > >
> > > > The second example is from regs_return_value():
> > > >           if (is_syscall_success(regs))
> > > >                   return regs->gpr[3];
> > > >           else
> > > >                   return -regs->gpr[3];
> > > >
> > > > The third example is from check_syscall_restart():
> > > >           regs->result = -EINTR;
> > > >           regs->gpr[3] = EINTR;
> > > >           regs->ccr |= 0x10000000;
> > > >
> > > > Compared with these examples, the failure of syscall_set_return_value()
> > > > to assign a positive ERRORCODE into regs->gpr[3] is clearly visible:
> > > > 	/*
> > > > 	 * In the general case it's not obvious that we must deal with
> > > > 	 * CCR here, as the syscall exit path will also do that for us.
> > > > 	 * However there are some places, eg. the signal code, which
> > > > 	 * check ccr to decide if the value in r3 is actually an error.
> > > > 	 */
> > > > 	if (error) {
> > > > 		regs->ccr |= 0x10000000L;
> > > > 		regs->gpr[3] = error;
> > > > 	} else {
> > > > 		regs->ccr &= ~0x10000000L;
> > > > 		regs->gpr[3] = val;
> > > > 	}
> > > >
> > > > This fix brings syscall_set_return_value() in sync with syscall_get_error()
> > > > and lets upcoming ptrace/set_syscall_info selftest pass on powerpc.
> > > >
> > > > Fixes: 1b1a3702a65c ("powerpc: Don't negate error in syscall_set_return_value()").
> > > > =======
> > >
> > > I think there is still something going wrong.
> > >
> > > do_seccomp() sets regs->gpr[3] = -ENOSYS; by default.
> > >
> > > Then it calls __secure_computing() which returns what __seccomp_filter()
> > > returns.
> > >
> > > In case of error, __seccomp_filter() calls syscall_set_return_value()
> > > with a negative value then returns -1
> > >
> > > do_seccomp() is called by do_syscall_trace_enter() which returns -1 when
> > > do_seccomp() doesn't return 0.
> > >
> > > do_syscall_trace_enter() is called by system_call_exception() and
> > > returns -1, so syscall_exception() returns regs->gpr[3]
> > >
> > > In entry_32.S, transfer_to_syscall, syscall_exit_prepare() is then
> > > called with the return of syscall_exception() as first parameter, which
> > > leads to:
> > >
> > > 	if (unlikely(r3 >= (unsigned long)-MAX_ERRNO) && is_not_scv) {
> > > 		if (likely(!(ti_flags & (_TIF_NOERROR | _TIF_RESTOREALL)))) {
> > > 			r3 = -r3;
> > > 			regs->ccr |= 0x10000000; /* Set SO bit in CR */
> > > 		}
> > > 	}
> >
> > Note the "unlikely" keyword here reminding us once more that in !scv case
> > regs->gpr[3] does not normally have -ERRORCODE form.
> >
> > > By chance, because you have already changed the sign of gpr[3], the
> > > above test fails and nothing is done to r3, and because you have also
> > > already set regs->ccr it works.
> > >
> > > But all this looks inconsistent with the fact that do_seccomp sets
> > > -ENOSYS as default value
> > >
> > > Also, when do_seccomp() returns 0, do_syscall_trace_enter() check the
> > > syscall number and when it is wrong it goes to skip: which sets
> > > regs->gpr[3] = -ENOSYS;
> >
> > It looks like do_seccomp() and do_syscall_trace_enter() get away by sheer
> > luck, implicitly relying on syscall_exit_prepare() transparently fixing
> > regs->gpr[3] for them.
> >
> > > So really I think it is not in line with your changes to set positive
> > > value in gpr[3].
> > >
> > > Maybe your change is still correct but it needs to be handled completely
> > > in that case.
> >
> > By the way, is there any reasons why do_seccomp() and
> > do_syscall_trace_enter() don't use syscall_set_return_value() yet?