From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pegase2.c-s.fr (pegase2.c-s.fr [93.17.235.10]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 8EBAF1F130D for ; Tue, 21 Jan 2025 11:50:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=93.17.235.10 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737460206; cv=none; b=ksrB61CWYi1380ShndnCcp3CnGXWjEhxySXlrhcCKdLjIE+Sra8/oPFLZKBqLNLk2ECQhoSfJFLuYG+5uq8gnIcXfN6GPsaiMUIuQ3r5y98HVqbqgLd0oNJup9XTs6P9aFK+ypwkE6MJUZpsTgClohVYhx7XJmqL6i3vRUporVQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737460206; c=relaxed/simple; bh=+zYxijhOz6t8scSS0/xR8wpKSGr7WgVip8z+Zuh2Iew=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=FUcg8VdRnxpc/Dhk/gL/LIR5Dl201oBnvyJ8cbKvMeKUI1m62/T/waHi3LWnH5VzjnW+7GcXsLoBeU2P1ID6PRnFF1Ktzq1Zyzfv+OBcg0/yb+2uEMxiE1RO8dPOLCXnkC/ibbcymlduQqdBFlqN6MT8Q0hd9D1Zer402nCRbIY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=csgroup.eu; spf=pass smtp.mailfrom=csgroup.eu; arc=none smtp.client-ip=93.17.235.10 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=csgroup.eu Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=csgroup.eu Received: from localhost (mailhub3.si.c-s.fr [172.26.127.67]) by localhost (Postfix) with ESMTP id 4YclMh1WGJz9sS7; Tue, 21 Jan 2025 12:28:48 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from pegase2.c-s.fr ([172.26.127.65]) by localhost (pegase2.c-s.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id aQHmO5nYyAco; Tue, 21 Jan 2025 12:28:48 +0100 (CET) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase2.c-s.fr (Postfix) with ESMTP id 4YclMh0WlCz9sRk; Tue, 21 Jan 2025 12:28:48 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id F38968B774; Tue, 21 Jan 2025 12:28:47 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id DuhPnq6i2FUi; Tue, 21 Jan 2025 12:28:47 +0100 (CET) Received: from [192.168.235.99] (unknown [192.168.235.99]) by messagerie.si.c-s.fr (Postfix) with ESMTP id BDDC88B763; Tue, 21 Jan 2025 12:28:46 +0100 (CET) Message-ID: <70cb8e5b-7ee5-494a-a02a-ef286f8c970c@csgroup.eu> Date: Tue, 21 Jan 2025 12:28:46 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/7] powerpc: properly negate error in syscall_set_return_value() To: Madhavan Srinivasan , "Dmitry V. Levin" Cc: Alexey Gladkov , Oleg Nesterov , Michael Ellerman , Eugene Syromyatnikov , Mike Frysinger , Renzo Davoli , Davide Berardi , strace-devel@lists.strace.io, Nicholas Piggin , Naveen N Rao , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org References: <20250113171054.GA589@strace.io> <6558110c-c2cb-4aa3-9472-b3496f71ebb8@csgroup.eu> <20250114170400.GB11820@strace.io> <20250120171249.GA17320@strace.io> <70dc8099-e725-4069-9b3a-af31578278e2@linux.ibm.com> Content-Language: fr-FR From: Christophe Leroy In-Reply-To: <70dc8099-e725-4069-9b3a-af31578278e2@linux.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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? >> >> >