RFR(S): 8220794: PPC64: Fix signal handler for SIGSEGV on branch to illegal address
Hi, Please, could I get reviews for the following change: bug : https://bugs.openjdk.java.net/browse/JDK-8220794 webrev: http://cr.openjdk.java.net/~gromero/8220794/v1/ It fixes the JVM signal handler on Linux / PPC64 when a SIGSEGV generated by a branch to an illegal/invalid address (not mapped address, address with no executable flags, etc) is caught by the JVM signal handler. Currently the signal handler does not handle that case correctly and the JVM crashes silently. That issue was reported by Goetz (SAP). Thanks for reporting the issue, Goetz. Thank you. Best regards, Gustavo
Hi Gustavo, Just a question, is the other case where we replace si_addr with address addr = ((NativeInstruction*)pc)->get_stack_bang_address(uc); really needed? Since we use si_addr in a number of places and I always assumed that just works like on every other posix system. E.g. we use it for assertion poison page handling or handling of secondary crashes during error reporting, see handle_assert_poison_fault(). Cheers, Thomas On Fri, Mar 22, 2019 at 4:29 PM Gustavo Romero <gromero@linux.vnet.ibm.com> wrote:
Hi,
Please, could I get reviews for the following change:
bug : https://bugs.openjdk.java.net/browse/JDK-8220794 webrev: http://cr.openjdk.java.net/~gromero/8220794/v1/
It fixes the JVM signal handler on Linux / PPC64 when a SIGSEGV generated by a branch to an illegal/invalid address (not mapped address, address with no executable flags, etc) is caught by the JVM signal handler. Currently the signal handler does not handle that case correctly and the JVM crashes silently.
That issue was reported by Goetz (SAP). Thanks for reporting the issue, Goetz.
Thank you.
Best regards, Gustavo
Hi Thomas, On 03/22/2019 02:18 PM, Thomas Stüfe wrote:
Hi Gustavo,
Just a question, is the other case where we replace si_addr with
address addr = ((NativeInstruction*)pc)->get_stack_bang_address(uc);
really needed? Since we use si_addr in a number of places and I always assumed that just works like on every other posix system.
This a good point, Thomas. If si_addr had always worked on Linux I understand that get_stack_band_address() would have never existed and the signal handler would not have to discern between data and instruction interruptions at all. By the comments and logs it looks like si_addr was broken before kernel 2.6.6 (quite old) which afaics is not currently supported by any distros anymore. So get_stack_bang_address() in the past was necessary, but not anymore, since long time, thus relying on si_addr in other places you pointed out worked well so far and will continues to work. We could switch the code to trust si_addr only, since I believe there is no real impact nowadays. But in theory it can be a regression... ? Thomas and Goetz: which approach do you suggest here? Please, advise. Thank you. Best regards, Gustavo
E.g. we use it for assertion poison page handling or handling of secondary crashes during error reporting, see handle_assert_poison_fault().
Cheers, Thomas
On Fri, Mar 22, 2019 at 4:29 PM Gustavo Romero <gromero@linux.vnet.ibm.com <mailto:gromero@linux.vnet.ibm.com>> wrote:
Hi,
Please, could I get reviews for the following change:
bug : https://bugs.openjdk.java.net/browse/JDK-8220794 webrev: http://cr.openjdk.java.net/~gromero/8220794/v1/ <http://cr.openjdk.java.net/%7Egromero/8220794/v1/>
It fixes the JVM signal handler on Linux / PPC64 when a SIGSEGV generated by a branch to an illegal/invalid address (not mapped address, address with no executable flags, etc) is caught by the JVM signal handler. Currently the signal handler does not handle that case correctly and the JVM crashes silently.
That issue was reported by Goetz (SAP). Thanks for reporting the issue, Goetz.
Thank you.
Best regards, Gustavo
Hi, the change looks good, and passed our testing together with the test fix for be. I would remove get_stack_band_address() in a follow up. I would appreciate if this one gets downported to jdk11u. We run jdk11u on quite old linux versions, so we better leave this in place for 11. For 13++ it makes sense to remove the old stuff. Best regards, Goetz.
-----Original Message----- From: hotspot-runtime-dev <hotspot-runtime-dev- bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Sonntag, 24. März 2019 20:12 To: Thomas Stüfe <thomas.stuefe@gmail.com>; goetz.lindenmeir@sap.com Cc: ppc-aix-port-dev@openjdk.java.net; hotspot-runtime- dev@openjdk.java.net Subject: Re: RFR(S): 8220794: PPC64: Fix signal handler for SIGSEGV on branch to illegal address
Hi Thomas,
On 03/22/2019 02:18 PM, Thomas Stüfe wrote:
Hi Gustavo,
Just a question, is the other case where we replace si_addr with
address addr = ((NativeInstruction*)pc)->get_stack_bang_address(uc);
really needed? Since we use si_addr in a number of places and I always assumed that just works like on every other posix system.
This a good point, Thomas. If si_addr had always worked on Linux I understand that get_stack_band_address() would have never existed and the signal handler would not have to discern between data and instruction interruptions at all.
By the comments and logs it looks like si_addr was broken before kernel 2.6.6 (quite old) which afaics is not currently supported by any distros anymore. So get_stack_bang_address() in the past was necessary, but not anymore, since long time, thus relying on si_addr in other places you pointed out worked well so far and will continues to work.
We could switch the code to trust si_addr only, since I believe there is no real impact nowadays. But in theory it can be a regression... ?
Thomas and Goetz: which approach do you suggest here? Please, advise.
Thank you.
Best regards, Gustavo
E.g. we use it for assertion poison page handling or handling of secondary crashes during error reporting, see handle_assert_poison_fault().
Cheers, Thomas
On Fri, Mar 22, 2019 at 4:29 PM Gustavo Romero <gromero@linux.vnet.ibm.com <mailto:gromero@linux.vnet.ibm.com>> wrote:
Hi,
Please, could I get reviews for the following change:
bug : https://bugs.openjdk.java.net/browse/JDK-8220794 webrev: http://cr.openjdk.java.net/~gromero/8220794/v1/ <http://cr.openjdk.java.net/%7Egromero/8220794/v1/>
It fixes the JVM signal handler on Linux / PPC64 when a SIGSEGV generated by a branch to an illegal/invalid address (not mapped address, address with no executable flags, etc) is caught by the JVM signal handler. Currently the signal handler does not handle that case correctly and the JVM crashes silently.
That issue was reported by Goetz (SAP). Thanks for reporting the issue, Goetz.
Thank you.
Best regards, Gustavo
On Mon, Mar 25, 2019 at 10:02 AM Lindenmaier, Goetz < goetz.lindenmaier@sap.com> wrote:
Hi,
the change looks good, and passed our testing together with the test fix for be.
I would remove get_stack_band_address() in a follow up. I would appreciate if this one gets downported to jdk11u. We run jdk11u on quite old linux versions, so we better leave this in place for 11. For 13++ it makes sense to remove the old stuff.
Okay. We should create a follow up issue. Thanks, Thomas
Best regards, Goetz.
-----Original Message----- From: hotspot-runtime-dev <hotspot-runtime-dev- bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Sonntag, 24. März 2019 20:12 To: Thomas Stüfe <thomas.stuefe@gmail.com>; goetz.lindenmeir@sap.com Cc: ppc-aix-port-dev@openjdk.java.net; hotspot-runtime- dev@openjdk.java.net Subject: Re: RFR(S): 8220794: PPC64: Fix signal handler for SIGSEGV on branch to illegal address
Hi Thomas,
On 03/22/2019 02:18 PM, Thomas Stüfe wrote:
Hi Gustavo,
Just a question, is the other case where we replace si_addr with
address addr = ((NativeInstruction*)pc)->get_stack_bang_address(uc);
really needed? Since we use si_addr in a number of places and I always assumed that just works like on every other posix system.
This a good point, Thomas. If si_addr had always worked on Linux I understand that get_stack_band_address() would have never existed and the signal handler would not have to discern between data and instruction interruptions at all.
By the comments and logs it looks like si_addr was broken before kernel 2.6.6 (quite old) which afaics is not currently supported by any distros anymore. So get_stack_bang_address() in the past was necessary, but not anymore, since long time, thus relying on si_addr in other places you pointed out worked well so far and will continues to work.
We could switch the code to trust si_addr only, since I believe there is no real impact nowadays. But in theory it can be a regression... ?
Thomas and Goetz: which approach do you suggest here? Please, advise.
Thank you.
Best regards, Gustavo
E.g. we use it for assertion poison page handling or handling of secondary crashes during error reporting, see handle_assert_poison_fault().
Cheers, Thomas
On Fri, Mar 22, 2019 at 4:29 PM Gustavo Romero <gromero@linux.vnet.ibm.com <mailto:gromero@linux.vnet.ibm.com>> wrote:
Hi,
Please, could I get reviews for the following change:
bug : https://bugs.openjdk.java.net/browse/JDK-8220794 webrev: http://cr.openjdk.java.net/~gromero/8220794/v1/ <http://cr.openjdk.java.net/%7Egromero/8220794/v1/>
It fixes the JVM signal handler on Linux / PPC64 when a SIGSEGV generated by a branch to an illegal/invalid address (not mapped address, address with no executable flags, etc) is caught by the JVM signal handler. Currently the signal handler does not handle that case correctly and the JVM crashes silently.
That issue was reported by Goetz (SAP). Thanks for reporting the issue, Goetz.
Thank you.
Best regards, Gustavo
Hi Thomas and Goetz, On 03/25/2019 09:06 AM, Thomas Stüfe wrote:
On Mon, Mar 25, 2019 at 10:02 AM Lindenmaier, Goetz <goetz.lindenmaier@sap.com <mailto:goetz.lindenmaier@sap.com>> wrote:
Hi,
the change looks good, and passed our testing together with the test fix for be.
I would remove get_stack_band_address() in a follow up. I would appreciate if this one gets downported to jdk11u. We run jdk11u on quite old linux versions, so we better leave this in place for 11. For 13++ it makes sense to remove the old stuff.
Okay. We should create a follow up issue.
I've opened the following bug to track it: PPC64: Remove get_stack_bang_address workaround https://bugs.openjdk.java.net/browse/JDK-8221410 Best regards, Gustavo
On 03/25/2019 06:02 AM, Lindenmaier, Goetz wrote:
Hi,
the change looks good, and passed our testing together with the test fix for be.
Thanks Thomas and Goetz. Pushed: http://hg.openjdk.java.net/jdk/jdk/rev/e61065c08924 Cheers, Gustavo
participants (3)
-
Gustavo Romero
-
Lindenmaier, Goetz
-
Thomas Stüfe