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

[Madhavan Srinivasan <maddy@linux.ibm.com>]

On 1/21/25 4:58 PM, Christophe Leroy wrote:
> 
> 
> 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 ?
> 

yeah, I guess.
This is from the a Power8 pSeries lpar without this patch

# ./set_syscall_info 
TAP version 13
1..1
# Starting 1 tests from 1 test cases.
#  RUN           global.set_syscall_info ...
# set_syscall_info.c:428:set_syscall_info:wait #5: unexpected stop signal 11
# set_syscall_info: Test terminated by assertion
#          FAIL  global.set_syscall_info
not ok 1 global.set_syscall_info
# FAILED: 0 / 1 tests passed.
# Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0

Maddy

> 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?
>>>
>>>
>>
>