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

Christophe Leroy
2025-01-20T14:20:03Z

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.
>>>
>>> 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 */
}
}

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;

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.

Christophe