search

battleblow / openjdk-jdk11u

BSD port of OpenJDK 11
GNU General Public License v2.0
9 stars 8 forks source link

Kernel stack guard pages #83

Closed battleblow closed 5 years ago

battleblow commented 5 years ago

Two changes

  1. Disable the kernel stack guard pages on recent versions of FreeBSD. Currently this is only FreeBSD-CURRENT as of 09/04
  2. Workaround the kernel stack guard pages on other versions of FreeBSD. This attempts to do something reasonable knowing that it can't resolve all of the issues.
battleblow commented 5 years ago

kib@ has MFC'ed the changes to 12 and 11. So they are in -STABLE for both branches at the moment and will eventually be in 12.1 (out in November) and 11.4 (not sure when that is likely to be).

I've made some additional changes which are along the lines of your first suggestion there. Namely, we define the needed constants if they aren't defined and always try to disable the kernel stack guard pages. Then, the code checks if the kernel stack guard pages procctl is disabled in the signal handler and only does the workaround if it is not. The net effect being that everything happens at runtime whether support was present at compile time or not.

I think that the sysctl changing values while the process is running, and hence affecting what pages may or may not be in place for a given thread, is more effort than it is worth. This seems like it is a rare enough case that it's an edge case and I don't think the extra code to deal with it is worth pursuing. People running on current versions of FreeBSD already have a complete workaround if they choose to use it, which is to set the value of the sysctl to 0. With that, everything works correctly. For those who choose not to do so, this is the level I think it's reasonable to go to.

battleblow commented 5 years ago

In terms of testing, I've tested these changes on -CURRENT with kib@'s changes. There the procctl disables as expected and StackOverflowError's are correctly thrown and use of the Reserved pages also works correctly. On a system without the changes, setting the sysctl to zero has the same effect. For a system without the changes and the sysctl set to a non-zero value (I tried 1 and 2) StackOverflow occurs correctly, but Reserved pages fail, as expected.

bsdkurt commented 5 years ago

I'm ok with this as is. As you pointed out it doesn't cover the Reserved pages test except when the procctl succeeds. I think there is a way forward that works for when the procctl fails that has minimal TOCTOU issues.

I looked over libthr in 11/12/current and the thread caching behavior is different then what I anticipated in a good way. It only reuses a thread stack if both the stack size and the guard size exactly match the saved stack. This means that setting the pthread guard size to match the security.bsd.stack_guard_page size has a very small window for a change to cause https://github.com/battleblow/openjdk-jdk11u/issues/51#issuecomment-521480373 to fail; e.g. the time between the syscall to get the value and the pthread create. Since I didn't check the libthr behavior previously I way overstated the TOCTOU window in my comments there.

The one benefit of the work-around in https://github.com/battleblow/openjdk-jdk11u/issues/51#issuecomment-521480373 is Reserved pages test will succeed even if the procctl fails (so long as security.bsd.stack_guard_page doesn't get increased in the sub-second window between getting the value and creating the thread). The part that is missing in 51 is querying the sysctl and setting the guard size to match it (in two places). With that added it should give you a working JVM regardless of the sysctl value or if the new procctl is available. Oh, __attribute__ ((optnone)) is clang only so that would need to be adjusted to work for both gcc and clang with something like this:

#if defined(__clang__)
#define NOOPT __attribute__ ((optnone))
#else
#define NOOPT __attribute__((optimize(0)))
#endif
NOOPT bool os::pd_create_stack_guard_pages(char* addr, size_t size)

I leave it up to you do decide between the current work-around or the one in 51.

https://github.com/freebsd/freebsd/blob/master/lib/libthr/thread/thr_stack.c#L237 https://github.com/freebsd/freebsd/blob/stable/11/lib/libthr/thread/thr_stack.c#L235 https://github.com/freebsd/freebsd/blob/stable/12/lib/libthr/thread/thr_stack.c#L238

battleblow commented 5 years ago

Thanks Kurt!

I made some changes this morning to take up the suggestion from the comments you had, but I'm not seeing the expected benefits. I think I'll merge this and then look more deeply at those changes to see if I missed something. I'll also push them onto this branch in case you have time to look at them too.

battleblow commented 5 years ago

Actually, I think that is because I missed part of the changes. Let me add them in and try again.

I must admit comments like "Danger" and "If we hit a kernel placed guard page this will crash the jvm" do make me a little nervous.

bsdkurt commented 5 years ago

Sorry about that. That comment should be softened a bit. I now believe that can only happen if security.bsd.stack_guard_page is increased in the short window of time between getting the value and creating the thread (edit: and procctl has failed).

battleblow commented 5 years ago

Oops, I meant to squash before merging. I'm loathe to do it post merge and rewrite history on the main branch though, so will leave it.

I'll start another review with the other approach. For the moment only for transparency's sake since I'm still seeing issues with it.