Re: Linux powerpc new system call instruction and ABI
Matheus Castanho writes: > Dmitry V. Levin writes: > >> On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote: >>> Florian Weimer writes: >>> > * Matheus Castanho via Libc-alpha: >>> >> From: Nicholas Piggin >>> >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes >>> >> >>> >> When using scv on templated ASM syscalls, current code interprets any >>> >> negative return value as error, but the only valid error codes are in >>> >> the range -4095..-1 according to the ABI. >>> >> >>> >> Reviewed-by: Matheus Castanho >>> > >>> > Please reference bug 27892 in the commit message. I'd also appreciate a >>> > backport to the 2.33 release branch (where you need to add NEWS manually >>> > to add the bug reference). >>> >>> No problem. [BZ #27892] appended to the commit title. I'll make sure to >>> backport to 2.33 as well. >> >> Could you also mention in the commit message that the change fixes >> 'signal.gen.test' strace test where it was observed initially? > > Sure, no problem. I'll commit it later today. Since the patch falls into the less-than-15-LOC category and this is Nick's first contribution to glibc, looks like he doesn't need a copyright assignment. Pushed to master as 7de36744ee1325f35d3fe0ca079dd33c40b12267 Backported to 2.33 via commit 0ef0e6de7fdfa18328b09ba2afb4f0112d4bdab4 Thanks, Matheus Castanho
Re: Linux powerpc new system call instruction and ABI
Dmitry V. Levin writes: > On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote: >> Florian Weimer writes: >> > * Matheus Castanho via Libc-alpha: >> >> From: Nicholas Piggin >> >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes >> >> >> >> When using scv on templated ASM syscalls, current code interprets any >> >> negative return value as error, but the only valid error codes are in >> >> the range -4095..-1 according to the ABI. >> >> >> >> Reviewed-by: Matheus Castanho >> > >> > Please reference bug 27892 in the commit message. I'd also appreciate a >> > backport to the 2.33 release branch (where you need to add NEWS manually >> > to add the bug reference). >> >> No problem. [BZ #27892] appended to the commit title. I'll make sure to >> backport to 2.33 as well. > > Could you also mention in the commit message that the change fixes > 'signal.gen.test' strace test where it was observed initially? Sure, no problem. I'll commit it later today. -- Matheus Castanho
Re: Linux powerpc new system call instruction and ABI
On Fri, May 21, 2021 at 05:00:36PM -0300, Matheus Castanho wrote: > Florian Weimer writes: > > * Matheus Castanho via Libc-alpha: > >> From: Nicholas Piggin > >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes > >> > >> When using scv on templated ASM syscalls, current code interprets any > >> negative return value as error, but the only valid error codes are in > >> the range -4095..-1 according to the ABI. > >> > >> Reviewed-by: Matheus Castanho > > > > Please reference bug 27892 in the commit message. I'd also appreciate a > > backport to the 2.33 release branch (where you need to add NEWS manually > > to add the bug reference). > > No problem. [BZ #27892] appended to the commit title. I'll make sure to > backport to 2.33 as well. Could you also mention in the commit message that the change fixes 'signal.gen.test' strace test where it was observed initially? -- ldv
Re: Linux powerpc new system call instruction and ABI
Florian Weimer writes: > * Matheus Castanho via Libc-alpha: > >> From: Nicholas Piggin >> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes >> >> When using scv on templated ASM syscalls, current code interprets any >> negative return value as error, but the only valid error codes are in >> the range -4095..-1 according to the ABI. >> >> Reviewed-by: Matheus Castanho > > Please reference bug 27892 in the commit message. I'd also appreciate a > backport to the 2.33 release branch (where you need to add NEWS manually > to add the bug reference). No problem. [BZ #27892] appended to the commit title. I'll make sure to backport to 2.33 as well. Thanks -- Matheus Castanho
Re: Linux powerpc new system call instruction and ABI
* Matheus Castanho via Libc-alpha: > From: Nicholas Piggin > Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes > > When using scv on templated ASM syscalls, current code interprets any > negative return value as error, but the only valid error codes are in > the range -4095..-1 according to the ABI. > > Reviewed-by: Matheus Castanho Please reference bug 27892 in the commit message. I'd also appreciate a backport to the 2.33 release branch (where you need to add NEWS manually to add the bug reference). Thanks, Florian
Re: Linux powerpc new system call instruction and ABI
Nicholas Piggin writes: > Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm: >> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am: >>> Hi, >>> >>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >>> [...] - Error handling: The consensus among kernel, glibc, and musl is to move to using negative return values in r3 rather than CR0[SO]=1 to indicate error, which matches most other architectures, and is closer to a function call. >>> >>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was >>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and >>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used. >>> This includes syscall_get_error() and all its users including >>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable >>> when scv is used. >>> >>> See also https://bugzilla.redhat.com/1929836 >> >> I see, thanks. Using latest strace from github.com, the attached kernel >> patch makes strace -k check results a lot greener. >> >> Some of the remaining failing tests look like this (I didn't look at all >> of them yet): >> >> signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, >> 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> ) = 50 >> signal(SIGUSR1, SIG_IGN)= 0xfacefeeddeadbeef >> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown >> errno 559038737) = 41 >> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737 >> ) = 26 >> exit_group(1) = ? >> >> I think the problem is glibc testing for -ve, but it should be comparing >> against -4095 (+cc Matheus) >> >> #define RET_SCV \ >> cmpdi r3,0; \ >> bgelr+; \ >> neg r3,r3; > > This glibc patch at least gets that signal test working. Haven't run the > full suite yet because of trouble making it work with a local glibc > install... > > Thanks, > Nick > > --- > > diff --git a/sysdeps/powerpc/powerpc64/sysdep.h > b/sysdeps/powerpc/powerpc64/sysdep.h > index c57bb1c05d..1ea4c3b917 100644 > --- a/sysdeps/powerpc/powerpc64/sysdep.h > +++ b/sysdeps/powerpc/powerpc64/sysdep.h > @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ > #endif > > #define RET_SCV \ > -cmpdi r3,0; \ > -bgelr+; \ > +li r9,-4095; \ > +cmpld r3,r9; \ > +bltlr+; \ > neg r3,r3; > > #define RET_SC \ Hi Nick, I agree the current code is accepting more values as errors than it should. This change looks good to me. All glibc tests are passing with it. I also built strace and checked one of the failing tests against a glibc with your patch: ~/src/strace/tests$ uname -r 5.10.16-1-default ~/src/strace/tests$ /lib64/libc.so.6 GNU C Library (GNU libc) release release version 2.33 (git 9826b03b74). [...] ~/src/strace/tests$ ./signal.gen.test errno2name.c:461: unknown errno 559038737: Unknown error 559038737 signal.gen.test: failed test: ../signal failed with code 1 ~/src/strace/tests$ ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) errno2name.c:461: unknown errno 559038737: Unknown error 559038737 Running with glibc containing the patch: ~/src/strace/tests$ ~/build/glibc/testrun.sh ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef signal(-559038737, SIG_IGN) = -1 EINVAL (Invalid argument) +++ exited with 0 +++ If the patch below looks OK to you and no one objects, I'll commit it to glibc on Monday. Thanks, Matheus Castanho --- From: Nicholas Piggin Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes When using scv on templated ASM syscalls, current code interprets any negative return value as error, but the only valid error codes are in the range -4095..-1 according to the ABI. Reviewed-by: Matheus Castanho --- sysdeps/powerpc/powerpc64/sysdep.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h index c57bb1c05d..1ea4c3b917 100644 --- a/sysdeps/powerpc/powerpc64/sysdep.h +++ b/sysdeps/powerpc/powerpc64/sysdep.h @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ #endif #define RET_SCV \ -cmpdi r3,0; \ -bgelr+; \ +li r9,-4095; \ +cmpld r3,r9; \ +bltlr+; \ neg r3,r3; #define RET_SC \ -- 2.31.1
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 20, 2021 12:59 pm: > On Thu, May 20, 2021 at 12:45:57PM +1000, Nicholas Piggin wrote: >> Excerpts from Dmitry V. Levin's message of May 20, 2021 11:06 am: >> > On Wed, May 19, 2021 at 07:48:47PM -0400, Rich Felker wrote: >> >> On Wed, May 19, 2021 at 06:09:25PM +, Joakim Tjernlund wrote: >> > [...] >> >> > W.r.t breaking ABI, isn't that what PowerPC is trying to do with the >> >> > new syscall I/F? >> >> >> >> No, it's a new independent interface. >> > >> > Unfortunately, being a new independent interface doesn't mean it isn't >> > an ABI break. In fact, it was a severe ABI break, and this thread is >> > an attempt to find a hotfix. >> >> It is an ABI break, that was known. The ptrace info stuff I fixed with >> the patch earlier was obviously a bug in my initial implementation and >> not intended (sorry my ptrace testing was not sufficient, and thanks for >> reporting it, by the way). > > Could you check whether tools/testing/selftests/ptrace/get_syscall_info.c > passes again with your fix, please? It does. Thanks, Nick > If yes, then PTRACE_GET_SYSCALL_INFO is fixed. > > By the way, kernel tracing and audit subsystems also use those functions > from asm/syscall.h and asm/ptrace.h, so your ptrace fix is likely to fix > these subsystems as well.
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 20, 2021 1:06 pm: > On Thu, May 20, 2021 at 12:40:36PM +1000, Nicholas Piggin wrote: > [...] >> > Looks like struct pt_regs.trap already contains the information that could >> > be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then >> > it's scv. Is my reading of arch/powerpc/include/asm/ptrace.h correct? >> >> Hmm, I think it is. Certainly in the kernel regs struct it is, I had in >> my mind that we put it to 0xc00 when populating the user struct for >> compatibility, but it seems not. So I guess this would work. > > OK, can we state that (pt_regs.trap & ~0xf) == 0x3000 is a part of the scv > ABI, so it's not going to change and could be relied upon by userspace? > Could this be documented in Documentation/powerpc/syscall64-abi.rst, > please? Yeah I think we can do that. The kernel doesn't care what is put in the userspace pt_regs.trap too much so if this is your preferred approach then I will document it in the ABI. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
On Thu, May 20, 2021 at 12:40:36PM +1000, Nicholas Piggin wrote: [...] > > Looks like struct pt_regs.trap already contains the information that could > > be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then > > it's scv. Is my reading of arch/powerpc/include/asm/ptrace.h correct? > > Hmm, I think it is. Certainly in the kernel regs struct it is, I had in > my mind that we put it to 0xc00 when populating the user struct for > compatibility, but it seems not. So I guess this would work. OK, can we state that (pt_regs.trap & ~0xf) == 0x3000 is a part of the scv ABI, so it's not going to change and could be relied upon by userspace? Could this be documented in Documentation/powerpc/syscall64-abi.rst, please? -- ldv
Re: Linux powerpc new system call instruction and ABI
On Thu, May 20, 2021 at 12:45:57PM +1000, Nicholas Piggin wrote: > Excerpts from Dmitry V. Levin's message of May 20, 2021 11:06 am: > > On Wed, May 19, 2021 at 07:48:47PM -0400, Rich Felker wrote: > >> On Wed, May 19, 2021 at 06:09:25PM +, Joakim Tjernlund wrote: > > [...] > >> > W.r.t breaking ABI, isn't that what PowerPC is trying to do with the new > >> > syscall I/F? > >> > >> No, it's a new independent interface. > > > > Unfortunately, being a new independent interface doesn't mean it isn't > > an ABI break. In fact, it was a severe ABI break, and this thread is > > an attempt to find a hotfix. > > It is an ABI break, that was known. The ptrace info stuff I fixed with > the patch earlier was obviously a bug in my initial implementation and > not intended (sorry my ptrace testing was not sufficient, and thanks for > reporting it, by the way). Could you check whether tools/testing/selftests/ptrace/get_syscall_info.c passes again with your fix, please? If yes, then PTRACE_GET_SYSCALL_INFO is fixed. By the way, kernel tracing and audit subsystems also use those functions from asm/syscall.h and asm/ptrace.h, so your ptrace fix is likely to fix these subsystems as well. -- ldv
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 20, 2021 11:06 am: > On Wed, May 19, 2021 at 07:48:47PM -0400, Rich Felker wrote: >> On Wed, May 19, 2021 at 06:09:25PM +, Joakim Tjernlund wrote: > [...] >> > W.r.t breaking ABI, isn't that what PowerPC is trying to do with the new >> > syscall I/F? >> >> No, it's a new independent interface. > > Unfortunately, being a new independent interface doesn't mean it isn't > an ABI break. In fact, it was a severe ABI break, and this thread is > an attempt to find a hotfix. It is an ABI break, that was known. The ptrace info stuff I fixed with the patch earlier was obviously a bug in my initial implementation and not intended (sorry my ptrace testing was not sufficient, and thanks for reporting it, by the way). But the register ABI was always a known break. The issue is that rfscv clobbers LR, so it can not support the old ABI. If the old ABI did not preserve LR, then we may have chosen to not change register ABI at all. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 20, 2021 9:27 am: > On Thu, May 20, 2021 at 08:51:53AM +1000, Nicholas Piggin wrote: >> Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm: >> > On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote: >> >> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm: >> >> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote: >> >> > [...] >> >> >> With this patch, I think the ptrace ABI should mostly be fixed. I >> >> >> think >> >> >> a problem remains with applications that look at system call return >> >> >> registers directly and have powerpc specific error cases. Those >> >> >> probably >> >> >> will just need to be updated unfortunately. Michael thought it might be >> >> >> possible to return an indication via ptrace somehow that the syscall is >> >> >> using a new ABI, so such apps can be updated to test for it. I don't >> >> >> know how that would be done. >> >> > >> >> > Is there any sane way for these applications to handle the scv case? >> >> > How can they tell that the scv semantics is being used for the given >> >> > syscall invocation? Can this information be obtained e.g. from struct >> >> > pt_regs? >> >> >> >> Not that I know of. Michael suggested there might be a way to add >> >> something. ptrace_syscall_info has some pad bytes, could >> >> we use one for flags bits and set a bit for "new system call ABI"? >> > >> > PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all >> > architecture-specific details behind struct ptrace_syscall_info which has >> > the same meaning on all architectures. ptrace_syscall_info.exit contains >> > both rval and is_error fields to support every architecture regardless of >> > its syscall ABI. >> > >> > ptrace_syscall_info.exit is extensible, but every architecture would have >> > to define a method of telling whether the system call follows the "new >> > system call ABI" conventions to export this bit of information. >> >> It's already architecture speicfic if you look at registers of syscall >> exit state so I don't see a problem with a flag that ppc can use for >> ABI. > > To be honest, I don't see anything architecture-specific in > PTRACE_GET_SYSCALL_INFO API. Yes, it's implementation uses various > functions defined in asm/syscall.h, but this doesn't make the interface > architecture-specific. No. But a field or flag it exports could be architecture dependent. It doesn't detract independence from the rest of the ABI. That said... > PTRACE_GET_SYSCALL_INFO saves its users from necessity to be aware of > tracee registers. That's why the only place where strace has to deal > with tracee registers nowadays is syscall tampering. The most reliable > solution is to introduce PTRACE_SET_SYSCALL_INFO, this would make the > whole syscall abi issue irrelevant for ptracers, maybe the time has come > to implement it. > > Unfortunately, extending ptrace API takes time, and it's not going to be > backported to older kernels anyway, but scv-enabled kernels are already > in the wild, so we need a quick powerpc-specific fix that would be > backported to all maintained scv-enabled kernels. > > [...] >> > I wonder why can't this information be just exported to the tracer via >> > struct pt_regs? >> >> It might be able to, I don't see why that would be superior though. >> >> Where could you put it... I guess it could go in the trap field in a >> high bit. But could that break things that just test for syscall >> trap number (and don't care about register ABI)? I'm not sure. > > Looks like struct pt_regs.trap already contains the information that could > be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then > it's scv. Is my reading of arch/powerpc/include/asm/ptrace.h correct? Hmm, I think it is. Certainly in the kernel regs struct it is, I had in my mind that we put it to 0xc00 when populating the user struct for compatibility, but it seems not. So I guess this would work. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 07:48:47PM -0400, Rich Felker wrote: > On Wed, May 19, 2021 at 06:09:25PM +, Joakim Tjernlund wrote: [...] > > W.r.t breaking ABI, isn't that what PowerPC is trying to do with the new > > syscall I/F? > > No, it's a new independent interface. Unfortunately, being a new independent interface doesn't mean it isn't an ABI break. In fact, it was a severe ABI break, and this thread is an attempt to find a hotfix. -- ldv
Re: [musl] Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 06:09:25PM +, Joakim Tjernlund wrote: > On Wed, 2021-05-19 at 10:22 -0500, Segher Boessenkool wrote: > > On Wed, May 19, 2021 at 03:06:49PM +, Joakim Tjernlund wrote: > > > On Wed, 2021-05-19 at 09:38 -0500, Segher Boessenkool wrote: > > > > On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > > > > > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > > > > > I always figured the ppc way was superior. It begs the question if > > > > > > not the other archs should > > > > > > change instead? > > > > > > > > > > It is superior in some ways, not enough to be worth being different. > > > > > > > > The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, > > > > you will have to do that whether you conflate the concepts of return > > > > code and error indicator or not! > > > > > > > > > Other archs are unlikely to change because it would be painful for > > > > > not much benefit. > > > > > > > > Other archs cannot easily change for much the same reason :-) > > > > > > Really? I figured you could just add extra error indication in kernel > > > syscall I/F. > > > Eventually user space could migrate to the new indication. > > > > You seem to assume all user space uses glibc, or *any* libc even? This > > is false. Some programs do system calls directly. Do not break the > > kernel ABI :-) > > Adding an extra indicator does not break ABI, does it ? It does, because the old ABI on most archs has no clobbers except the return value register. Some archs though have additional syscall-clobbered regs that could be repurposed as extra return values, but you could only rely on them being meaningful after probing for a kernel that speaks the new variant. This just makes things more complicated, not more useful. > W.r.t breaking ABI, isn't that what PowerPC is trying to do with the new > syscall I/F? No, it's a new independent interface. Rich
Re: Linux powerpc new system call instruction and ABI
On Thu, May 20, 2021 at 08:51:53AM +1000, Nicholas Piggin wrote: > Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm: > > On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote: > >> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm: > >> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote: > >> > [...] > >> >> With this patch, I think the ptrace ABI should mostly be fixed. I think > >> >> a problem remains with applications that look at system call return > >> >> registers directly and have powerpc specific error cases. Those probably > >> >> will just need to be updated unfortunately. Michael thought it might be > >> >> possible to return an indication via ptrace somehow that the syscall is > >> >> using a new ABI, so such apps can be updated to test for it. I don't > >> >> know how that would be done. > >> > > >> > Is there any sane way for these applications to handle the scv case? > >> > How can they tell that the scv semantics is being used for the given > >> > syscall invocation? Can this information be obtained e.g. from struct > >> > pt_regs? > >> > >> Not that I know of. Michael suggested there might be a way to add > >> something. ptrace_syscall_info has some pad bytes, could > >> we use one for flags bits and set a bit for "new system call ABI"? > > > > PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all > > architecture-specific details behind struct ptrace_syscall_info which has > > the same meaning on all architectures. ptrace_syscall_info.exit contains > > both rval and is_error fields to support every architecture regardless of > > its syscall ABI. > > > > ptrace_syscall_info.exit is extensible, but every architecture would have > > to define a method of telling whether the system call follows the "new > > system call ABI" conventions to export this bit of information. > > It's already architecture speicfic if you look at registers of syscall > exit state so I don't see a problem with a flag that ppc can use for > ABI. To be honest, I don't see anything architecture-specific in PTRACE_GET_SYSCALL_INFO API. Yes, it's implementation uses various functions defined in asm/syscall.h, but this doesn't make the interface architecture-specific. PTRACE_GET_SYSCALL_INFO saves its users from necessity to be aware of tracee registers. That's why the only place where strace has to deal with tracee registers nowadays is syscall tampering. The most reliable solution is to introduce PTRACE_SET_SYSCALL_INFO, this would make the whole syscall abi issue irrelevant for ptracers, maybe the time has come to implement it. Unfortunately, extending ptrace API takes time, and it's not going to be backported to older kernels anyway, but scv-enabled kernels are already in the wild, so we need a quick powerpc-specific fix that would be backported to all maintained scv-enabled kernels. [...] > > I wonder why can't this information be just exported to the tracer via > > struct pt_regs? > > It might be able to, I don't see why that would be superior though. > > Where could you put it... I guess it could go in the trap field in a > high bit. But could that break things that just test for syscall > trap number (and don't care about register ABI)? I'm not sure. Looks like struct pt_regs.trap already contains the information that could be used to tell 'sc' from 'scv': if (pt_regs.trap & ~0xf) == 0x3000, then it's scv. Is my reading of arch/powerpc/include/asm/ptrace.h correct? -- ldv
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 19, 2021 11:26 pm: > On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote: >> Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm: >> > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote: >> > [...] >> >> With this patch, I think the ptrace ABI should mostly be fixed. I think >> >> a problem remains with applications that look at system call return >> >> registers directly and have powerpc specific error cases. Those probably >> >> will just need to be updated unfortunately. Michael thought it might be >> >> possible to return an indication via ptrace somehow that the syscall is >> >> using a new ABI, so such apps can be updated to test for it. I don't >> >> know how that would be done. >> > >> > Is there any sane way for these applications to handle the scv case? >> > How can they tell that the scv semantics is being used for the given >> > syscall invocation? Can this information be obtained e.g. from struct >> > pt_regs? >> >> Not that I know of. Michael suggested there might be a way to add >> something. ptrace_syscall_info has some pad bytes, could >> we use one for flags bits and set a bit for "new system call ABI"? > > PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all > architecture-specific details behind struct ptrace_syscall_info which has > the same meaning on all architectures. ptrace_syscall_info.exit contains > both rval and is_error fields to support every architecture regardless of > its syscall ABI. > > ptrace_syscall_info.exit is extensible, but every architecture would have > to define a method of telling whether the system call follows the "new > system call ABI" conventions to export this bit of information. It's already architecture speicfic if you look at registers of syscall exit state so I don't see a problem with a flag that ppc can use for ABI. > > This essentially means implementing something like > static inline long syscall_get_error_abi(struct task_struct *task, struct > pt_regs *regs) > for every architecture, and using it along with syscall_get_error > in ptrace_get_syscall_info_exit to initialize the new field in > ptrace_syscall_info.exit structure. Yes this could work. Other architectures can just use a generic implementation if they don't define their own so that's easy. And in userspace they can continue to ignore the flag. > >> As a more hacky thing you could make a syscall with -1 and see how >> the error looks, and then assume all syscalls will be the same. > > This would be very unreliable because sc and scv are allowed to intermingle, > so every syscall invocation can follow any of these two error handling > conventions. > >> Or... is it possible at syscall entry to peek the address of >> the instruction which caused the call and see if that was a >> scv instruction? That would be about as reliable as possible >> without having that new flag bit. > > No other architecture requires peeking into tracee memory just to find out > the syscall ABI. This would make powerpc the most ugly architecture for > ptracing. > > I wonder why can't this information be just exported to the tracer via > struct pt_regs? It might be able to, I don't see why that would be superior though. Where could you put it... I guess it could go in the trap field in a high bit. But could that break things that just test for syscall trap number (and don't care about register ABI)? I'm not sure. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
On Wed, 2021-05-19 at 10:22 -0500, Segher Boessenkool wrote: > On Wed, May 19, 2021 at 03:06:49PM +, Joakim Tjernlund wrote: > > On Wed, 2021-05-19 at 09:38 -0500, Segher Boessenkool wrote: > > > On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > > > > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > > > > I always figured the ppc way was superior. It begs the question if > > > > > not the other archs should > > > > > change instead? > > > > > > > > It is superior in some ways, not enough to be worth being different. > > > > > > The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, > > > you will have to do that whether you conflate the concepts of return > > > code and error indicator or not! > > > > > > > Other archs are unlikely to change because it would be painful for > > > > not much benefit. > > > > > > Other archs cannot easily change for much the same reason :-) > > > > Really? I figured you could just add extra error indication in kernel > > syscall I/F. > > Eventually user space could migrate to the new indication. > > You seem to assume all user space uses glibc, or *any* libc even? This > is false. Some programs do system calls directly. Do not break the > kernel ABI :-) Adding an extra indicator does not break ABI, does it ? W.r.t breaking ABI, isn't that what PowerPC is trying to do with the new syscall I/F? Jocke
Re: [musl] Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 10:22:05AM -0500, Segher Boessenkool wrote: > On Wed, May 19, 2021 at 03:06:49PM +, Joakim Tjernlund wrote: > > On Wed, 2021-05-19 at 09:38 -0500, Segher Boessenkool wrote: > > > On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > > > > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > > > > I always figured the ppc way was superior. It begs the question if > > > > > not the other archs should > > > > > change instead? > > > > > > > > It is superior in some ways, not enough to be worth being different. > > > > > > The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, > > > you will have to do that whether you conflate the concepts of return > > > code and error indicator or not! > > > > > > > Other archs are unlikely to change because it would be painful for > > > > not much benefit. > > > > > > Other archs cannot easily change for much the same reason :-) > > > > Really? I figured you could just add extra error indication in kernel > > syscall I/F. > > Eventually user space could migrate to the new indication. > > You seem to assume all user space uses glibc, or *any* libc even? This > is false. Some programs do system calls directly. Do not break the > kernel ABI :-) Even if it were easy to change, the old ppc ABI with a separate error indicator is much worse to use. In musl we paper over archs that do this silliness by converting to a normal negated errno code. There are literally no syscalls that need the ability to return negative values in addition to error codes; historically there were one or two (I only recall one fcntl command) but there were ways to disambiguate and they're only fallbacks for ancient kernels nowadays, if used at all. Rich
Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 03:06:49PM +, Joakim Tjernlund wrote: > On Wed, 2021-05-19 at 09:38 -0500, Segher Boessenkool wrote: > > On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > > > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > > > I always figured the ppc way was superior. It begs the question if not > > > > the other archs should > > > > change instead? > > > > > > It is superior in some ways, not enough to be worth being different. > > > > The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, > > you will have to do that whether you conflate the concepts of return > > code and error indicator or not! > > > > > Other archs are unlikely to change because it would be painful for > > > not much benefit. > > > > Other archs cannot easily change for much the same reason :-) > > Really? I figured you could just add extra error indication in kernel syscall > I/F. > Eventually user space could migrate to the new indication. You seem to assume all user space uses glibc, or *any* libc even? This is false. Some programs do system calls directly. Do not break the kernel ABI :-) Segher
Re: Linux powerpc new system call instruction and ABI
On Wed, 2021-05-19 at 09:38 -0500, Segher Boessenkool wrote: > On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > > I always figured the ppc way was superior. It begs the question if not > > > the other archs should > > > change instead? > > > > It is superior in some ways, not enough to be worth being different. > > The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, > you will have to do that whether you conflate the concepts of return > code and error indicator or not! > > > Other archs are unlikely to change because it would be painful for > > not much benefit. > > Other archs cannot easily change for much the same reason :-) Really? I figured you could just add extra error indication in kernel syscall I/F. Eventually user space could migrate to the new indication. Jocke
Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 06:42:40PM +1000, Nicholas Piggin wrote: > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > > I always figured the ppc way was superior. It begs the question if not the > > other archs should > > change instead? > > It is superior in some ways, not enough to be worth being different. The PowerPC syscall ABI *requires* using cr0.3 for indicating errors, you will have to do that whether you conflate the concepts of return code and error indicator or not! > Other archs are unlikely to change because it would be painful for > not much benefit. Other archs cannot easily change for much the same reason :-) > New system calls just should be made to not return > error numbers. Which sometimes is a difficult / non-natural / clumsy thing to do. Segher
Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 08:59:05PM +1000, Nicholas Piggin wrote: > Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm: > > On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote: > > [...] > >> With this patch, I think the ptrace ABI should mostly be fixed. I think > >> a problem remains with applications that look at system call return > >> registers directly and have powerpc specific error cases. Those probably > >> will just need to be updated unfortunately. Michael thought it might be > >> possible to return an indication via ptrace somehow that the syscall is > >> using a new ABI, so such apps can be updated to test for it. I don't > >> know how that would be done. > > > > Is there any sane way for these applications to handle the scv case? > > How can they tell that the scv semantics is being used for the given > > syscall invocation? Can this information be obtained e.g. from struct > > pt_regs? > > Not that I know of. Michael suggested there might be a way to add > something. ptrace_syscall_info has some pad bytes, could > we use one for flags bits and set a bit for "new system call ABI"? PTRACE_GET_SYSCALL_INFO is an architecture-agnostic API, it hides all architecture-specific details behind struct ptrace_syscall_info which has the same meaning on all architectures. ptrace_syscall_info.exit contains both rval and is_error fields to support every architecture regardless of its syscall ABI. ptrace_syscall_info.exit is extensible, but every architecture would have to define a method of telling whether the system call follows the "new system call ABI" conventions to export this bit of information. This essentially means implementing something like static inline long syscall_get_error_abi(struct task_struct *task, struct pt_regs *regs) for every architecture, and using it along with syscall_get_error in ptrace_get_syscall_info_exit to initialize the new field in ptrace_syscall_info.exit structure. > As a more hacky thing you could make a syscall with -1 and see how > the error looks, and then assume all syscalls will be the same. This would be very unreliable because sc and scv are allowed to intermingle, so every syscall invocation can follow any of these two error handling conventions. > Or... is it possible at syscall entry to peek the address of > the instruction which caused the call and see if that was a > scv instruction? That would be about as reliable as possible > without having that new flag bit. No other architecture requires peeking into tracee memory just to find out the syscall ABI. This would make powerpc the most ugly architecture for ptracing. I wonder why can't this information be just exported to the tracer via struct pt_regs? -- ldv
Re: Linux powerpc new system call instruction and ABI
Nicholas Piggin via Libc-alpha writes: > As a more hacky thing you could make a syscall with -1 and see how > the error looks, and then assume all syscalls will be the same. I'm not sure this would work. Even in glibc, it's expected that early syscalls will use sc while scv is used later in the execution. -- Tulio Magno
Re: Linux powerpc new system call instruction and ABI
Excerpts from Nicholas Piggin's message of May 19, 2021 6:42 pm: > Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: >> On Wed, 2021-05-19 at 17:55 +1000, Nicholas Piggin wrote: >>> Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm: >>> > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote: >>> > > Hi, >>> > > >>> > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >>> > > [...] >>> > > > - Error handling: The consensus among kernel, glibc, and musl is to >>> > > > move to >>> > > > using negative return values in r3 rather than CR0[SO]=1 to >>> > > > indicate error, >>> > > > which matches most other architectures, and is closer to a function >>> > > > call. >>> > >>> > What about syscalls like times(2) which can return -1 without it being an >>> > error? >>> >>> They do become errors / indistinguishable and have to be dealt with by >>> libc or userspace. Which does follow what most architectures do (all >>> except ia64, mips, sparc, and powerpc actually). >>> >>> Interesting question though, it should have been noted. >>> >>> Thanks, >>> Nick >> >> I always figured the ppc way was superior. It begs the question if not the >> other archs should >> change instead? > > It is superior in some ways, not enough to be worth being different. > > Other archs are unlikely to change because it would be painful for > not much benefit. New system calls just should be made to not return > error numbers. If we ever had a big new version of syscall ABI in > Linux, we can always use another scv vector number for it. Or... is it possible at syscall entry to peek the address of the instruction which caused the call and see if that was a scv instruction? That would be about as reliable as possible without having that new flag bit. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 19, 2021 8:24 pm:
> On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
> [...]
>> With this patch, I think the ptrace ABI should mostly be fixed. I think
>> a problem remains with applications that look at system call return
>> registers directly and have powerpc specific error cases. Those probably
>> will just need to be updated unfortunately. Michael thought it might be
>> possible to return an indication via ptrace somehow that the syscall is
>> using a new ABI, so such apps can be updated to test for it. I don't
>> know how that would be done.
>
> Is there any sane way for these applications to handle the scv case?
> How can they tell that the scv semantics is being used for the given
> syscall invocation? Can this information be obtained e.g. from struct
> pt_regs?
Not that I know of. Michael suggested there might be a way to add
something. ptrace_syscall_info has some pad bytes, could
we use one for flags bits and set a bit for "new system call ABI"?
As a more hacky thing you could make a syscall with -1 and see how
the error looks, and then assume all syscalls will be the same.
Thanks,
Nick
>
> For example, in strace we have the following powerpc-specific code used
> for syscall tampering:
>
> $ cat src/linux/powerpc/set_error.c
> /*
> * Copyright (c) 2016-2021 The strace developers.
> * All rights reserved.
> *
> * SPDX-License-Identifier: LGPL-2.1-or-later
> */
>
> static int
> arch_set_r3_ccr(struct tcb *tcp, const unsigned long r3,
> const unsigned long ccr_set, const unsigned long ccr_clear)
> {
> if (ptrace_syscall_info_is_valid() &&
> upeek(tcp, sizeof(long) * PT_CCR, _regs.ccr))
> return -1;
> const unsigned long old_ccr = ppc_regs.ccr;
> ppc_regs.gpr[3] = r3;
> ppc_regs.ccr |= ccr_set;
> ppc_regs.ccr &= ~ccr_clear;
> if (ppc_regs.ccr != old_ccr &&
> upoke(tcp, sizeof(long) * PT_CCR, ppc_regs.ccr))
> return -1;
> return upoke(tcp, sizeof(long) * (PT_R0 + 3), ppc_regs.gpr[3]);
> }
>
> static int
> arch_set_error(struct tcb *tcp)
> {
> return arch_set_r3_ccr(tcp, tcp->u_error, 0x1000, 0);
> }
>
> static int
> arch_set_success(struct tcb *tcp)
> {
> return arch_set_r3_ccr(tcp, tcp->u_rval, 0, 0x1000);
> }
>
>
> --
> ldv
>
Re: Linux powerpc new system call instruction and ABI
On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
[...]
> With this patch, I think the ptrace ABI should mostly be fixed. I think
> a problem remains with applications that look at system call return
> registers directly and have powerpc specific error cases. Those probably
> will just need to be updated unfortunately. Michael thought it might be
> possible to return an indication via ptrace somehow that the syscall is
> using a new ABI, so such apps can be updated to test for it. I don't
> know how that would be done.
Is there any sane way for these applications to handle the scv case?
How can they tell that the scv semantics is being used for the given
syscall invocation? Can this information be obtained e.g. from struct
pt_regs?
For example, in strace we have the following powerpc-specific code used
for syscall tampering:
$ cat src/linux/powerpc/set_error.c
/*
* Copyright (c) 2016-2021 The strace developers.
* All rights reserved.
*
* SPDX-License-Identifier: LGPL-2.1-or-later
*/
static int
arch_set_r3_ccr(struct tcb *tcp, const unsigned long r3,
const unsigned long ccr_set, const unsigned long ccr_clear)
{
if (ptrace_syscall_info_is_valid() &&
upeek(tcp, sizeof(long) * PT_CCR, _regs.ccr))
return -1;
const unsigned long old_ccr = ppc_regs.ccr;
ppc_regs.gpr[3] = r3;
ppc_regs.ccr |= ccr_set;
ppc_regs.ccr &= ~ccr_clear;
if (ppc_regs.ccr != old_ccr &&
upoke(tcp, sizeof(long) * PT_CCR, ppc_regs.ccr))
return -1;
return upoke(tcp, sizeof(long) * (PT_R0 + 3), ppc_regs.gpr[3]);
}
static int
arch_set_error(struct tcb *tcp)
{
return arch_set_r3_ccr(tcp, tcp->u_error, 0x1000, 0);
}
static int
arch_set_success(struct tcb *tcp)
{
return arch_set_r3_ccr(tcp, tcp->u_rval, 0, 0x1000);
}
--
ldv
Re: Linux powerpc new system call instruction and ABI
Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm: > On Wed, 2021-05-19 at 17:55 +1000, Nicholas Piggin wrote: >> Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm: >> > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote: >> > > Hi, >> > > >> > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >> > > [...] >> > > > - Error handling: The consensus among kernel, glibc, and musl is to >> > > > move to >> > > > using negative return values in r3 rather than CR0[SO]=1 to indicate >> > > > error, >> > > > which matches most other architectures, and is closer to a function >> > > > call. >> > >> > What about syscalls like times(2) which can return -1 without it being an >> > error? >> >> They do become errors / indistinguishable and have to be dealt with by >> libc or userspace. Which does follow what most architectures do (all >> except ia64, mips, sparc, and powerpc actually). >> >> Interesting question though, it should have been noted. >> >> Thanks, >> Nick > > I always figured the ppc way was superior. It begs the question if not the > other archs should > change instead? It is superior in some ways, not enough to be worth being different. Other archs are unlikely to change because it would be painful for not much benefit. New system calls just should be made to not return error numbers. If we ever had a big new version of syscall ABI in Linux, we can always use another scv vector number for it. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
On Wed, 2021-05-19 at 17:55 +1000, Nicholas Piggin wrote: > Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm: > > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote: > > > Hi, > > > > > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: > > > [...] > > > > - Error handling: The consensus among kernel, glibc, and musl is to > > > > move to > > > > using negative return values in r3 rather than CR0[SO]=1 to indicate > > > > error, > > > > which matches most other architectures, and is closer to a function > > > > call. > > > > What about syscalls like times(2) which can return -1 without it being an > > error? > > They do become errors / indistinguishable and have to be dealt with by > libc or userspace. Which does follow what most architectures do (all > except ia64, mips, sparc, and powerpc actually). > > Interesting question though, it should have been noted. > > Thanks, > Nick I always figured the ppc way was superior. It begs the question if not the other archs should change instead? Jocke
Re: Linux powerpc new system call instruction and ABI
Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm: > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote: >> Hi, >> >> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >> [...] >> > - Error handling: The consensus among kernel, glibc, and musl is to move to >> > using negative return values in r3 rather than CR0[SO]=1 to indicate >> > error, >> > which matches most other architectures, and is closer to a function call. > > What about syscalls like times(2) which can return -1 without it being an > error? They do become errors / indistinguishable and have to be dealt with by libc or userspace. Which does follow what most architectures do (all except ia64, mips, sparc, and powerpc actually). Interesting question though, it should have been noted. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote: > Hi, > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: > [...] > > - Error handling: The consensus among kernel, glibc, and musl is to move to > > using negative return values in r3 rather than CR0[SO]=1 to indicate > > error, > > which matches most other architectures, and is closer to a function call. What about syscalls like times(2) which can return -1 without it being an error? Jocke
Re: Linux powerpc new system call instruction and ABI
Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm: > Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am: >> Hi, >> >> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >> [...] >>> - Error handling: The consensus among kernel, glibc, and musl is to move to >>> using negative return values in r3 rather than CR0[SO]=1 to indicate >>> error, >>> which matches most other architectures, and is closer to a function call. >> >> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was >> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and >> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used. >> This includes syscall_get_error() and all its users including >> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable >> when scv is used. >> >> See also https://bugzilla.redhat.com/1929836 > > I see, thanks. Using latest strace from github.com, the attached kernel > patch makes strace -k check results a lot greener. > > Some of the remaining failing tests look like this (I didn't look at all > of them yet): > > signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) > write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, > 0xfacefeeddeadbeef) = 0 (SIG_DFL) > ) = 50 > signal(SIGUSR1, SIG_IGN)= 0xfacefeeddeadbeef > write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown > errno 559038737) = 41 > write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737 > ) = 26 > exit_group(1) = ? > > I think the problem is glibc testing for -ve, but it should be comparing > against -4095 (+cc Matheus) > > #define RET_SCV \ > cmpdi r3,0; \ > bgelr+; \ > neg r3,r3; This glibc patch at least gets that signal test working. Haven't run the full suite yet because of trouble making it work with a local glibc install... Thanks, Nick --- diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h index c57bb1c05d..1ea4c3b917 100644 --- a/sysdeps/powerpc/powerpc64/sysdep.h +++ b/sysdeps/powerpc/powerpc64/sysdep.h @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ #endif #define RET_SCV \ -cmpdi r3,0; \ -bgelr+; \ +li r9,-4095; \ +cmpld r3,r9; \ +bltlr+; \ neg r3,r3; #define RET_SC \
Re: Linux powerpc new system call instruction and ABI
Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
> Hi,
>
> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
> [...]
>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>> using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>> which matches most other architectures, and is closer to a function call.
>
> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
> This includes syscall_get_error() and all its users including
> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
> when scv is used.
>
> See also https://bugzilla.redhat.com/1929836
I see, thanks. Using latest strace from github.com, the attached kernel
patch makes strace -k check results a lot greener.
Some of the remaining failing tests look like this (I didn't look at all
of them yet):
signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1,
0xfacefeeddeadbeef) = 0 (SIG_DFL)
) = 50
signal(SIGUSR1, SIG_IGN)= 0xfacefeeddeadbeef
write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown
errno 559038737) = 41
write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
) = 26
exit_group(1) = ?
I think the problem is glibc testing for -ve, but it should be comparing
against -4095 (+cc Matheus)
#define RET_SCV \
cmpdi r3,0; \
bgelr+; \
neg r3,r3;
With this patch, I think the ptrace ABI should mostly be fixed. I think
a problem remains with applications that look at system call return
registers directly and have powerpc specific error cases. Those probably
will just need to be updated unfortunately. Michael thought it might be
possible to return an indication via ptrace somehow that the syscall is
using a new ABI, so such apps can be updated to test for it. I don't
know how that would be done.
Thanks,
Nick
--
diff --git a/arch/powerpc/include/asm/ptrace.h
b/arch/powerpc/include/asm/ptrace.h
index 9c9ab2746168..b476a685f066 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -19,6 +19,7 @@
#ifndef _ASM_POWERPC_PTRACE_H
#define _ASM_POWERPC_PTRACE_H
+#include
#include
#include
@@ -152,25 +153,6 @@ extern unsigned long profile_pc(struct pt_regs *regs);
long do_syscall_trace_enter(struct pt_regs *regs);
void do_syscall_trace_leave(struct pt_regs *regs);
-#define kernel_stack_pointer(regs) ((regs)->gpr[1])
-static inline int is_syscall_success(struct pt_regs *regs)
-{
- return !(regs->ccr & 0x1000);
-}
-
-static inline long regs_return_value(struct pt_regs *regs)
-{
- if (is_syscall_success(regs))
- return regs->gpr[3];
- else
- return -regs->gpr[3];
-}
-
-static inline void regs_set_return_value(struct pt_regs *regs, unsigned long
rc)
-{
- regs->gpr[3] = rc;
-}
-
#ifdef __powerpc64__
#define user_mode(regs) regs)->msr) >> MSR_PR_LG) & 0x1)
#else
@@ -235,6 +217,31 @@ static __always_inline void set_trap_norestart(struct
pt_regs *regs)
regs->trap |= 0x1;
}
+#define kernel_stack_pointer(regs) ((regs)->gpr[1])
+static inline int is_syscall_success(struct pt_regs *regs)
+{
+ if (trap_is_scv(regs))
+ return !IS_ERR_VALUE((unsigned long)regs->gpr[3]);
+ else
+ return !(regs->ccr & 0x1000);
+}
+
+static inline long regs_return_value(struct pt_regs *regs)
+{
+ if (trap_is_scv(regs))
+ return regs->gpr[3];
+
+ if (is_syscall_success(regs))
+ return regs->gpr[3];
+ else
+ return -regs->gpr[3];
+}
+
+static inline void regs_set_return_value(struct pt_regs *regs, unsigned long
rc)
+{
+ regs->gpr[3] = rc;
+}
+
#define arch_has_single_step() (1)
#define arch_has_block_step() (true)
#define ARCH_HAS_USER_SINGLE_STEP_REPORT
diff --git a/arch/powerpc/include/asm/syscall.h
b/arch/powerpc/include/asm/syscall.h
index fd1b518eed17..e8b40149bf7e 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -41,11 +41,20 @@ static inline void syscall_rollback(struct task_struct
*task,
static inline long syscall_get_error(struct task_struct *task,
struct pt_regs *regs)
{
- /*
-* If the system call failed,
-* regs->gpr[3] contains a positive ERRORCODE.
-*/
- return (regs->ccr & 0x1000UL) ? -regs->gpr[3] : 0;
+ if (trap_is_scv(regs)) {
+ unsigned long error = regs->gpr[3];
+
+ if (task_is_32bit(task))
+ error = (long)(int)error;
+
+ return IS_ERR_VALUE(error) ? error : 0;
+ } else {
+ /*
+* If the system
Re: Linux powerpc new system call instruction and ABI
Hi, On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: [...] > - Error handling: The consensus among kernel, glibc, and musl is to move to > using negative return values in r3 rather than CR0[SO]=1 to indicate error, > which matches most other architectures, and is closer to a function call. Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used. This includes syscall_get_error() and all its users including PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable when scv is used. See also https://bugzilla.redhat.com/1929836 -- ldv
Re: Linux powerpc new system call instruction and ABI
Excerpts from Segher Boessenkool's message of June 12, 2020 7:02 am: > Hi! > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >> Calling convention >> -- >> The proposal is for scv 0 to provide the standard Linux system call ABI >> with the following differences from sc convention[1]: >> >> - lr is to be volatile across scv calls. This is necessary because the >> scv instruction clobbers lr. From previous discussion, this should be >> possible to deal with in GCC clobbers and CFI. >> >> - cr1 and cr5-cr7 are volatile. This matches the C ABI and would allow the >> kernel system call exit to avoid restoring the volatile cr registers >> (although we probably still would anyway to avoid information leaks). >> >> - Error handling: The consensus among kernel, glibc, and musl is to move to >> using negative return values in r3 rather than CR0[SO]=1 to indicate error, >> which matches most other architectures, and is closer to a function call. > > What about cr0 then? Will it be volatile as well (exactly like for > function calls)? Yes, same as for sc (except for SO bit). Which is a bit unclear in this section. >> Notes >> - >> - r0,r4-r8 are documented as volatile in the ABI, but the kernel patch as >> submitted currently preserves them. This is to leave room for deciding >> which way to go with these. > > The kernel has to set it to *something* that doesn't leak information ;-) For "sc" system calls these were defined as volatile (and used to just leak information), so now we just zero them. Thanks, Nick
Re: Linux powerpc new system call instruction and ABI
Hi! On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: > Calling convention > -- > The proposal is for scv 0 to provide the standard Linux system call ABI > with the following differences from sc convention[1]: > > - lr is to be volatile across scv calls. This is necessary because the > scv instruction clobbers lr. From previous discussion, this should be > possible to deal with in GCC clobbers and CFI. > > - cr1 and cr5-cr7 are volatile. This matches the C ABI and would allow the > kernel system call exit to avoid restoring the volatile cr registers > (although we probably still would anyway to avoid information leaks). > > - Error handling: The consensus among kernel, glibc, and musl is to move to > using negative return values in r3 rather than CR0[SO]=1 to indicate error, > which matches most other architectures, and is closer to a function call. What about cr0 then? Will it be volatile as well (exactly like for function calls)? > Notes > - > - r0,r4-r8 are documented as volatile in the ABI, but the kernel patch as > submitted currently preserves them. This is to leave room for deciding > which way to go with these. The kernel has to set it to *something* that doesn't leak information ;-) Segher
