JDK-8318119: Invalid narrow Klass base on aarch64 post 8312018

[tstuefe]

When reserving space for class space or the combined CDS+class space, provide a fallback on aarch64 when all prior attempts to reserve space failed. Now the logic goes like this:

1. (only if not CDS) attempt to reserve in low-address regions
2. attempt to reserve at a 32G-aligned address at randomized attach points within the whole of the available address space
3. (new) attempt to reserve 4G-aligned by using os::reserve_aligned.

(3) is similar to (2), but (2) may fail if we are very unlucky and the address space is very populated. The difference between (2) and (3) is that with (3), the kernel chooses the placement of the VMA, and we rely on ASLR to provide entropy.

The patch also fixes os::reserve_memory_aligned() to remove the assertion that size needs to be aligned. There is no need for that, and for large alignments this will blow up the required over-allocation size even further.

The patch also fixes up CompressedKlassPointers::is_valid_base(). The whole purpose of this function is to filter out possibly invalid base addresses when someone manually specifies SharedBaseAddress. The problem with this function is that the encoding base alone is not sufficient to determine its validity, since for that one needs to know the shift value too, and for that the boundaries of the future encoding range. I changed this function to give the best answer possible at that point, see comment.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8318119

Issue

• JDK-8318119: Invalid narrow Klass base on aarch64 post JDK-8312018 (Bug - P2) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16215/head:pull/16215
$ git checkout pull/16215

Update a local copy of the PR:
$ git checkout pull/16215
$ git pull https://git.openjdk.org/jdk.git pull/16215/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16215

View PR using the GUI difftool:
$ git pr show -t 16215

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16215.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@tstuefe The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[theRealAph]

What's the point of preferring a shift? On AArch64, better simply to use a movk.

[tstuefe]

The way movk mode is currently implemented is to apply the right-shifted base to the narrow Klass ID, then to left-shift the thing to get Klass*

jdk/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

Lines 4723 to 4731 in d0ea2a5

	const uint64_t shifted_base =
	(uint64_t)CompressedKlassPointers::base() >> CompressedKlassPointers::shift();
	if (dst != src) movw(dst, src);
	movk(dst, shifted_base >> 32, 32);
	if (CompressedKlassPointers::shift() != 0) {
	lsl(dst, dst, LogKlassAlignmentInBytes);
	}

which requires the right-shifted base to be movk-able. So it must have the lower 32+shift bits zero:

jdk/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

Lines 4656 to 4659 in d0ea2a5

	const uint64_t shifted_base =
	(uint64_t)CompressedKlassPointers::base() >> CompressedKlassPointers::shift();
	guarantee((shifted_base & 0xffff0000ffffffff) == 0,
	"compressed class base bad alignment");

hence the 32g alignment.

movw dst, src
movk dst, right-shifted-base to q3
shl dst, dst, 3

Note that this patch is still draft. I wanted to add another movk decoding variant, would work with a 4GB aligned base, just to increase our options.

It would apply the unshifted base to the left-shifted narrow Klass ID:

shl dst, src, 3
movk dst, unshifted-base

and that would even need one less operation.

Only problem is that it limits the value range of the narrow Klass ID, since it must not spill over into the third quadrant, so its upper (shift) bits must be zero. For shift=3, it means narrowKlass must be <= 2^(32-3). I think this is fine though, since we still can address 4 GB of class space. In other words, we should have never seen such high narrow Klass IDs anyway.

[theRealAph]

The way movk mode is currently implemented is to apply the right-shifted base to the narrow Klass ID, then to left-shift the thing to get Klass*

There's no need to left shift the Klass if we're using movk mode. Shift should be zero. We should always use a zero shift, 4G aligned, class space. That's how it was intended to work when I wrote it.

[mlbridge]

Webrevs

• 02: Full - Incremental (892bdc1c)
• 01: Full - Incremental (0f5a8c14)
• 00: Full (87cb6284)

[tstuefe]

Anyone?

[theRealAph]

Anyone?

I find those comments utterly incomprehensible.

[theRealAph]

Does this mean that the class metadata might be late than 4 Gbyte?

[tstuefe]

Sorry, I don't understand this question.

[theRealAph]

Argh, typo. Does this mean that the class metadata might be more than 4G in size? Is that the risk?

[tstuefe]

Currently not. Class space is limited to 3Gb, CDS to 1Gb, they are placed adjacent to each other to not surpass 4G.

[theRealAph]

So what does the word "probably" refer to?

[tstuefe]

This function attempts to answer "is this address a valid encoding base". Strictly speaking, that question cannot be answered in isolation. The correct question "given a specific Klass range, can a valid encoding scheme be found with this base and any valid shift?".

"Probably" in this context means "This base would work, assuming a future Klass range that starts at the given encoding base and its length does not surpass the maximum encoding range".

This function is used just for one valid use case: verifying the user input for -XX:SharedBaseAddress. That happens at a point where we have not allocated anything yet, so we don't know the future encoding range.

I am really considering removing this function. It is difficult to explain, of not much use, and also slightly wrong since it ignores the base==valid EOR immediate case.

Side note about non-zero immediate + non-zero shift:

The Klass range does not have to start at the encoding base. For encoding base == zero it obviously does not. But also for non-zero encoding bases this could in theory make sense, at least on aarch64 that has no fallback add-base mode.

For example, given a Klass range starting just below 8 GB due to ASLR, this normally would lead to VM exit since such an address is unusable with MOVK, unless it fits EOR mode. But we could use 4GB as encoding base. Since the future Klass range would extend beyond 4GB + 4GB, we would need a non-zero shift that enlarges the encoding range. But then it would work.

We have not done this so far, and there are things to consider (e.g. it would make sense to reserve and protect a single page at encoding base, to get NULL-reference crashes), but it is possible. That is why I am hesitant to hard-code 4GB as encoding range limit.

[theRealAph]

This function is used just for one valid use case: verifying the
user input for -XX:SharedBaseAddress. That happens at a point where
we have not allocated anything yet, so we don't know the future
encoding range.

So this function is simply asking a question that cannot be answered
without actually trying it.

I am really considering removing this function. It is difficult to
explain, of not much use, and also slightly wrong since it ignores
the base==valid EOR immediate case.

Please do! If there is to be an answer about whether a base is valid,
let it happen just after a failed attempted allocation.

Side note about non-zero immediate + non-zero shift:

The Klass range does not have to start at the encoding base. For
encoding base == zero it obviously does not. But also for non-zero
encoding bases this could in theory make sense, at least on aarch64
that has no fallback add-base mode.

For example, given a Klass range starting just below 8 GB due to
ASLR, this normally would lead to VM exit since such an address is
unusable with MOVK, unless it fits EOR mode.

Surely we'd scan upwards until a successful allocation. Why does it
matter where the Klass range starts? Or is the problem that we're
trying hard to honour a -XX: user request, for some reason? What would
be the purpose?

[tstuefe]

This function is used just for one valid use case: verifying the
user input for -XX:SharedBaseAddress. That happens at a point where
we have not allocated anything yet, so we don't know the future
encoding range.

So this function is simply asking a question that cannot be answered without actually trying it.

I am really considering removing this function. It is difficult to
explain, of not much use, and also slightly wrong since it ignores
the base==valid EOR immediate case.

Please do! If there is to be an answer about whether a base is valid, let it happen just after a failed attempted allocation.

Side note about non-zero immediate + non-zero shift:
The Klass range does not have to start at the encoding base. For
encoding base == zero it obviously does not. But also for non-zero
encoding bases this could in theory make sense, at least on aarch64
that has no fallback add-base mode.
For example, given a Klass range starting just below 8 GB due to
ASLR, this normally would lead to VM exit since such an address is
unusable with MOVK, unless it fits EOR mode.

Surely we'd scan upwards until a successful allocation. Why does it matter where the Klass range starts? Or is the problem that we're trying hard to honour a -XX: user request, for some reason? What would be the purpose?

We cannot scan upward because Oracle does not want us to do that for security reasons. We are to provide some form of ASLR. But we do scan the available address range (32-48 bits) for a suitable mapping point in a random fashion, see:

jdk/src/hotspot/share/memory/metaspace.cpp

Lines 620 to 622 in b3126b6

	log_debug(metaspace, map)("Trying between " UINT64_FORMAT_X " and " UINT64_FORMAT_X
	" with " SIZE_FORMAT_X " alignment", min, max, alignment);
	result = os::attempt_reserve_memory_between((char*)min, (char*)max, size, alignment, randomize);

With this patch, as a fallback, we now let the system reserve anywhere but with overalignment, and then cut-align the memory ourselves. That way, we rely on the entropy of the system's ASLR at the cost of temporary over-allocation of address space. In theory this should work most of the time unless someone limits the vsize of the process.

All of that may fail once in a blue moon. The address space may be heavily populated. The user space may be limited from below with vm.mmap_min_adr or by SELinux. The kernel itself may further limit at what addresses the user is allowed to map, though I did not find such a restriction for Arm64.

Note that the error motivating this PR came from someone running the VM on an OrangePi SoC. I compared the orangepi linux sources with stock but could not see anything immediately obvious, but it does have a large delta to stock.

Anyway, this patch was supposed to be a small low-cost attempt to improve matters.

[theRealAph]

Oh, wow. I never heard about "Oracle does not want us to do that for security reasons. We are to provide some form of ASLR." Sorry, this is all news to me.

[tstuefe]

Anyone?

I find those comments utterly incomprehensible.

Which ones exactly?

[tstuefe]

@theRealAph I remove the questionable CompressedKlassPointers::is_valid_base() which in hindsight confused more than it helped. I removed all asserts using that function, since on the one platform where it matters we will run against other asserts early if encoding does not work.

For the one legitimate use of that function, validating -XX:SharedBaseAddress at CDS dump time, I just added an aarch64-specific line and a clear comment. I feel this is much clearer to the code reader. Sometimes ifdef is the best way to go.

I also added some comments to the reservation function. Please check if this agrees with you.

[tstuefe]

I am withdrawing this in favour of a clearer, broader solution.