| Message ID | 153258768962.6738.11319866502689416568.stgit@dhcp-9-109-246-16 |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user: ppc64: don't use volatile register during safe_syscall | expand |
Le 26/07/2018 à 08:48, Shivaprasad G Bhat a écrit : > r11 is a volatile register on PPC as per calling conventions. > The safe_syscall code uses it to check if the signal_pending > is set during the safe_syscall. When a syscall is interrupted > on return from signal handling, the r11 might be corrupted > before we retry the syscall leading to a crash. The registers > r0-r13 are not to be used here as they have > volatile/designated/reserved usages. Change the code to use > r14 which is non-volatile and is appropriate for local use in > safe_syscall. > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> > --- > Steps to reproduce: > On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` > Attempt Ctrl-C, the issue is reproduced. > > Reference: > https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG I think these comments should be included in the commit message. Using the example of qemu-x86_64 on ppc64 would be less ambiguous. I've tested on ppc64: qemu-x86_64 /usr/bin/cc -E - Tested-by: Laurent Vivier <laurent@vivier.eu> > linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Laurent Vivier <laurent@vivier.eu> Thanks, Laurent
On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: > r11 is a volatile register on PPC as per calling conventions. > The safe_syscall code uses it to check if the signal_pending > is set during the safe_syscall. When a syscall is interrupted > on return from signal handling, the r11 might be corrupted > before we retry the syscall leading to a crash. The registers > r0-r13 are not to be used here as they have > volatile/designated/reserved usages. Change the code to use > r14 which is non-volatile and is appropriate for local use in > safe_syscall. > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> > --- > Steps to reproduce: > On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` > Attempt Ctrl-C, the issue is reproduced. > > Reference: > https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG > > linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S > index d30050a67c..b0cbbe6a69 100644 > --- a/linux-user/host/ppc64/safe-syscall.inc.S > +++ b/linux-user/host/ppc64/safe-syscall.inc.S > @@ -49,7 +49,7 @@ safe_syscall_base: > * and returns the result in r3 > * Shuffle everything around appropriately. > */ > - mr 11, 3 /* signal_pending */ > + mr 14, 3 /* signal_pending */ I do see that I was incorrect in assuming that r11 would be unmodified. But you can't simply write to a call-saved register -- you must preserve its value for the caller. Saving the value requires that you find some space on, or create, a stack frame. Note that there are two different conventions for _CALL_AIX and _CALL_ELF==2. r~
On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: > Reference: > https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG This document is for _CALL_ELF < 2. For ppc64le, the document is at https://openpowerfoundation.org/wp-content/uploads/2016/03/ABI64BitOpenPOWERv1.1_16July2015_pub4.pdf In both cases, it appears that we can (ab)use SP+16 to save the value of r14 across the syscall. This slot would normally be used for saving our own return address (LR), but we have no need to save that value because it *is* preserved across the syscall. r~
Le 26/07/2018 à 19:15, Richard Henderson a écrit : > On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: >> r11 is a volatile register on PPC as per calling conventions. >> The safe_syscall code uses it to check if the signal_pending >> is set during the safe_syscall. When a syscall is interrupted >> on return from signal handling, the r11 might be corrupted >> before we retry the syscall leading to a crash. The registers >> r0-r13 are not to be used here as they have >> volatile/designated/reserved usages. Change the code to use >> r14 which is non-volatile and is appropriate for local use in >> safe_syscall. >> >> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> >> --- >> Steps to reproduce: >> On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` >> Attempt Ctrl-C, the issue is reproduced. >> >> Reference: >> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG >> >> linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S >> index d30050a67c..b0cbbe6a69 100644 >> --- a/linux-user/host/ppc64/safe-syscall.inc.S >> +++ b/linux-user/host/ppc64/safe-syscall.inc.S >> @@ -49,7 +49,7 @@ safe_syscall_base: >> * and returns the result in r3 >> * Shuffle everything around appropriately. >> */ >> - mr 11, 3 /* signal_pending */ >> + mr 14, 3 /* signal_pending */ > > I do see that I was incorrect in assuming that r11 would be unmodified. But > you can't simply write to a call-saved register -- you must preserve its value > for the caller. > > Saving the value requires that you find some space on, or create, a stack > frame. Note that there are two different conventions for _CALL_AIX and > _CALL_ELF==2. Can we guess the syscall ('sc') will not modify neither r11 nor r14, but the function caller expects that r11 is not modified because it's the environment pointer, and saves r14 because it's one of its local variable it knows it has to preserve? In this case, I think Shivaprasad's fix is correct. Thanks, Laurent
On 07/26/2018 10:39 AM, Laurent Vivier wrote: > Le 26/07/2018 à 19:15, Richard Henderson a écrit : >> On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: >>> r11 is a volatile register on PPC as per calling conventions. >>> The safe_syscall code uses it to check if the signal_pending >>> is set during the safe_syscall. When a syscall is interrupted >>> on return from signal handling, the r11 might be corrupted >>> before we retry the syscall leading to a crash. The registers >>> r0-r13 are not to be used here as they have >>> volatile/designated/reserved usages. Change the code to use >>> r14 which is non-volatile and is appropriate for local use in >>> safe_syscall. >>> >>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> >>> --- >>> Steps to reproduce: >>> On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` >>> Attempt Ctrl-C, the issue is reproduced. >>> >>> Reference: >>> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG >>> >>> linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S >>> index d30050a67c..b0cbbe6a69 100644 >>> --- a/linux-user/host/ppc64/safe-syscall.inc.S >>> +++ b/linux-user/host/ppc64/safe-syscall.inc.S >>> @@ -49,7 +49,7 @@ safe_syscall_base: >>> * and returns the result in r3 >>> * Shuffle everything around appropriately. >>> */ >>> - mr 11, 3 /* signal_pending */ >>> + mr 14, 3 /* signal_pending */ >> >> I do see that I was incorrect in assuming that r11 would be unmodified. But >> you can't simply write to a call-saved register -- you must preserve its value >> for the caller. >> >> Saving the value requires that you find some space on, or create, a stack >> frame. Note that there are two different conventions for _CALL_AIX and >> _CALL_ELF==2. > > Can we guess the syscall ('sc') will not modify neither r11 nor r14... But sc does modify r11. li r11,0 std r11,GPR9(r1) std r11,GPR10(r1) std r11,GPR11(r1) (Incidentally, unless I'm misreading, the kernel could have saved r11 with exactly as much effort as it took to clobber it to zero. That's just rude.) > but the function caller expects that r11 is not modified because it's the > environment pointer Huh? What do you think an "environment pointer" is in this context? In the ppc64 abi, r11 is one of the ones that are clobbered by plt entries; it is not special in any way except as a scratch. > and saves r14 because it's one of its local > variable it knows it has to preserve? Huh? The caller of safe_syscall does not save r14, and does not expect it clobbered. > In this case, I think Shivaprasad's fix is correct. Definitely not. r~
On 07/26/2018 10:56 PM, Richard Henderson wrote: > On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: >> Reference: >> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG > This document is for _CALL_ELF < 2. For ppc64le, the document is at > > https://openpowerfoundation.org/wp-content/uploads/2016/03/ABI64BitOpenPOWERv1.1_16July2015_pub4.pdf > > In both cases, it appears that we can (ab)use SP+16 to save > the value of r14 across the syscall. This slot would normally > be used for saving our own return address (LR), but we have no > need to save that value because it *is* preserved across the syscall. I will send updated patch as suggested. Thanks, Shivaprasad > > > r~ >
Le 27/07/2018 à 06:47, Richard Henderson a écrit : > On 07/26/2018 10:39 AM, Laurent Vivier wrote: >> Le 26/07/2018 à 19:15, Richard Henderson a écrit : >>> On 07/25/2018 11:48 PM, Shivaprasad G Bhat wrote: >>>> r11 is a volatile register on PPC as per calling conventions. >>>> The safe_syscall code uses it to check if the signal_pending >>>> is set during the safe_syscall. When a syscall is interrupted >>>> on return from signal handling, the r11 might be corrupted >>>> before we retry the syscall leading to a crash. The registers >>>> r0-r13 are not to be used here as they have >>>> volatile/designated/reserved usages. Change the code to use >>>> r14 which is non-volatile and is appropriate for local use in >>>> safe_syscall. >>>> >>>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> >>>> --- >>>> Steps to reproduce: >>>> On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` >>>> Attempt Ctrl-C, the issue is reproduced. >>>> >>>> Reference: >>>> https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG >>>> >>>> linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S >>>> index d30050a67c..b0cbbe6a69 100644 >>>> --- a/linux-user/host/ppc64/safe-syscall.inc.S >>>> +++ b/linux-user/host/ppc64/safe-syscall.inc.S >>>> @@ -49,7 +49,7 @@ safe_syscall_base: >>>> * and returns the result in r3 >>>> * Shuffle everything around appropriately. >>>> */ >>>> - mr 11, 3 /* signal_pending */ >>>> + mr 14, 3 /* signal_pending */ >>> >>> I do see that I was incorrect in assuming that r11 would be unmodified. But >>> you can't simply write to a call-saved register -- you must preserve its value >>> for the caller. >>> >>> Saving the value requires that you find some space on, or create, a stack >>> frame. Note that there are two different conventions for _CALL_AIX and >>> _CALL_ELF==2. >> >> Can we guess the syscall ('sc') will not modify neither r11 nor r14... > > But sc does modify r11. > > li r11,0 > std r11,GPR9(r1) > std r11,GPR10(r1) > std r11,GPR11(r1) > > (Incidentally, unless I'm misreading, the kernel could have saved r11 with > exactly as much effort as it took to clobber it to zero. That's just rude.) > >> but the function caller expects that r11 is not modified because it's the >> environment pointer > > Huh? What do you think an "environment pointer" is in this context? It's the comment for r11 in the section "2.2.1.1 Register Roles" of the document. > In the ppc64 abi, r11 is one of the ones that are clobbered by plt entries; it > is not special in any way except as a scratch. > >> and saves r14 because it's one of its local >> variable it knows it has to preserve? > > Huh? The caller of safe_syscall does not save r14, and does not expect it > clobbered. Yes, you're right. The callee has to save nonvolatile registers, and r14 is a nonvolatile register. And r11 is volatile, it's why it can be modified by sc. I should read the doc more carefully. Thanks, Laurent
diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S index d30050a67c..b0cbbe6a69 100644 --- a/linux-user/host/ppc64/safe-syscall.inc.S +++ b/linux-user/host/ppc64/safe-syscall.inc.S @@ -49,7 +49,7 @@ safe_syscall_base: * and returns the result in r3 * Shuffle everything around appropriately. */ - mr 11, 3 /* signal_pending */ + mr 14, 3 /* signal_pending */ mr 0, 4 /* syscall number */ mr 3, 5 /* syscall arguments */ mr 4, 6 @@ -67,7 +67,7 @@ safe_syscall_base: */ safe_syscall_start: /* if signal_pending is non-zero, don't do the call */ - lwz 12, 0(11) + lwz 12, 0(14) cmpwi 0, 12, 0 bne- 0f sc
r11 is a volatile register on PPC as per calling conventions. The safe_syscall code uses it to check if the signal_pending is set during the safe_syscall. When a syscall is interrupted on return from signal handling, the r11 might be corrupted before we retry the syscall leading to a crash. The registers r0-r13 are not to be used here as they have volatile/designated/reserved usages. Change the code to use r14 which is non-volatile and is appropriate for local use in safe_syscall. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- Steps to reproduce: On PPC host, issue `qemu-ppc64le /usr/bin/cc -E -` Attempt Ctrl-C, the issue is reproduced. Reference: https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#REG linux-user/host/ppc64/safe-syscall.inc.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)