From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from vmicros1.altlinux.org (vmicros1.altlinux.org [194.107.17.57]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E60B11EE001 for ; Mon, 20 Jan 2025 17:12:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=194.107.17.57 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737393180; cv=none; b=bubyTVpcKmgPBtvi6wSaqkGDpiEnX9hakndTGSpoBxU1eV4Ovs5I53jEZTFNqc/x4lzpLcOLonX70Ebanq6kTgknYBolX4b42rZPeDM7QcLmJb9jRqEPQf69y1vjP8LLyfInbfPBwBpZxUu9SGVBR0GyhsPySm8/WAzncUD38Sg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737393180; c=relaxed/simple; bh=EfTWVMCstRVM3TfMHhi5vSNwOVC12C/FN+g0OTv1Dgs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BiUluqO8PNRyIorNKbF12lgi1Y69ZqDtMAHr4IH7FV9QhKYf/RxgTIvemfTP6vg9lYp/tS0HpA78Gdy9mbCU/KWxim5VwkOa8SHH4yJGysfqxxKSj5LlFir2sALgp4BpEQ/etNiR5iAqqX9XpZv1EKvkHZeV9ExygOJUo7b7wF0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=strace.io; spf=pass smtp.mailfrom=altlinux.org; arc=none smtp.client-ip=194.107.17.57 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=strace.io Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=altlinux.org Received: from mua.local.altlinux.org (mua.local.altlinux.org [192.168.1.14]) by vmicros1.altlinux.org (Postfix) with ESMTP id 954AD72C8F5; Mon, 20 Jan 2025 20:12:49 +0300 (MSK) Received: by mua.local.altlinux.org (Postfix, from userid 508) id 898057CCB3A; Mon, 20 Jan 2025 19:12:49 +0200 (IST) Date: Mon, 20 Jan 2025 19:12:49 +0200 From: "Dmitry V. Levin" To: Christophe Leroy Cc: Alexey Gladkov , Oleg Nesterov , Michael Ellerman , Eugene Syromyatnikov , Mike Frysinger , Renzo Davoli , Davide Berardi , strace-devel@lists.strace.io, Madhavan Srinivasan , Nicholas Piggin , Naveen N Rao , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/7] powerpc: properly negate error in syscall_set_return_value() Message-ID: <20250120171249.GA17320@strace.io> References: <20250113171054.GA589@strace.io> <6558110c-c2cb-4aa3-9472-b3496f71ebb8@csgroup.eu> <20250114170400.GB11820@strace.io> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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. > >>> > >>> 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? -- ldv