Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
> This PR proposes the introduction of **guarding** of the use of > `DirectBuffer::address` within the JDK. With the introduction of the Foreign > Function and Memory access, it is possible to derive Buffer instances that > are backed by native memory that, in turn, can be closed asynchronously by > the user (and not only via a `Cleaner` when there is no other reference to > the `Buffer` object). If another thread is invoking `MemorySession::close` > while a thread is doing work using raw addresses, the outcome is undefined. > This means the JVM might crash or even worse, silent modification of > unrelated memory might occur. > > Design choices in this PR: > > There is already a method `MemorySession::whileAlive` that takes a runnable > and that will perform the provided action while acquiring the underlying` > MemorySession` (if any). However, using this method entailed relatively large > changes while converting larger/nested code segments into lambdas. This would > also risk introducing lambda capturing. So, instead, a try-with-resources > friendly access method was added. This made is more easy to add guarding and > did not risk lambda capturing. Also, introducing lambdas in certain > fundamental JDK classes might incur bootstrap problems. > > The aforementioned TwR is using a "session acquisition" that is not used > explicitly in the try block itself. This raises a warning that is suppressed > using `@SuppressWarnings("try")`. In the future, we might be able to remove > these suppressions by using the reserved variable name `_`. Another > alternative was evaluated where a non-autocloseable resource was used. > However, it became relatively complicated to guarantee that the session was > always released and, at the same time, the existing 'final` block was always > executed properly (as they both might throw exceptions). In the end, the > proposed solution was much simpler and robust despite requiring a > non-referenced TwR variable. > > In some cases, where is is guaranteed that the backing memory session is > non-closeable, we do not have to guard the use of `DirectBuffer::address`. > ~~These cases have been documented in the code.~~ > > On some occasions, a plurality of acquisitions are made. This would never > lead to deadlocks as acquisitions are fundamentally concurrent counters and > not resources that only one thread can "own". > > I have added comments (and not javadocs) before the declaration of the > non-public-api `DirectBuffer::address` method, that the use of the returned > address needs to be guarded. It can be discussed if this is preferable or not. > > This PR spawns several areas of responsibility and so, I expect more than one > reviewer before promoting the PR. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Rework Acquisition - Changes: - all: https://git.openjdk.org/jdk/pull/11260/files - new: https://git.openjdk.org/jdk/pull/11260/files/88ed3aa2..0526e3dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=07-08 Stats: 249 lines in 19 files changed: 66 ins; 91 del; 92 mod Patch: https://git.openjdk.org/jdk/pull/11260.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260 PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote: >> This PR proposes the introduction of **guarding** of the use of >> `DirectBuffer::address` within the JDK. With the introduction of the Foreign >> Function and Memory access, it is possible to derive Buffer instances that >> are backed by native memory that, in turn, can be closed asynchronously by >> the user (and not only via a `Cleaner` when there is no other reference to >> the `Buffer` object). If another thread is invoking `MemorySession::close` >> while a thread is doing work using raw addresses, the outcome is undefined. >> This means the JVM might crash or even worse, silent modification of >> unrelated memory might occur. >> >> Design choices in this PR: >> >> There is already a method `MemorySession::whileAlive` that takes a runnable >> and that will perform the provided action while acquiring the underlying` >> MemorySession` (if any). However, using this method entailed relatively >> large changes while converting larger/nested code segments into lambdas. >> This would also risk introducing lambda capturing. So, instead, a >> try-with-resources friendly access method was added. This made is more easy >> to add guarding and did not risk lambda capturing. Also, introducing lambdas >> in certain fundamental JDK classes might incur bootstrap problems. >> >> The aforementioned TwR is using a "session acquisition" that is not used >> explicitly in the try block itself. This raises a warning that is suppressed >> using `@SuppressWarnings("try")`. In the future, we might be able to remove >> these suppressions by using the reserved variable name `_`. Another >> alternative was evaluated where a non-autocloseable resource was used. >> However, it became relatively complicated to guarantee that the session was >> always released and, at the same time, the existing 'final` block was always >> executed properly (as they both might throw exceptions). In the end, the >> proposed solution was much simpler and robust despite requiring a >> non-referenced TwR variable. >> >> In some cases, where is is guaranteed that the backing memory session is >> non-closeable, we do not have to guard the use of `DirectBuffer::address`. >> ~~These cases have been documented in the code.~~ >> >> On some occasions, a plurality of acquisitions are made. This would never >> lead to deadlocks as acquisitions are fundamentally concurrent counters and >> not resources that only one thread can "own". >> >> I have added comments (and not javadocs) before the declaration of the >> non-public-api `DirectBuffer::address` method, that the use of the returned >> address needs to be guarded. It can be discussed if this is preferable or >> not. >> >> This PR spawns several areas of responsibility and so, I expect more than >> one reviewer before promoting the PR. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Rework Acquisition > because this PR is touching several low level and performance critical code > paths Indeed. Have we verified that performance doesn't regress with these changes? - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote: >> This PR proposes the introduction of **guarding** of the use of >> `DirectBuffer::address` within the JDK. With the introduction of the Foreign >> Function and Memory access, it is possible to derive Buffer instances that >> are backed by native memory that, in turn, can be closed asynchronously by >> the user (and not only via a `Cleaner` when there is no other reference to >> the `Buffer` object). If another thread is invoking `MemorySession::close` >> while a thread is doing work using raw addresses, the outcome is undefined. >> This means the JVM might crash or even worse, silent modification of >> unrelated memory might occur. >> >> Design choices in this PR: >> >> There is already a method `MemorySession::whileAlive` that takes a runnable >> and that will perform the provided action while acquiring the underlying` >> MemorySession` (if any). However, using this method entailed relatively >> large changes while converting larger/nested code segments into lambdas. >> This would also risk introducing lambda capturing. So, instead, a >> try-with-resources friendly access method was added. This made is more easy >> to add guarding and did not risk lambda capturing. Also, introducing lambdas >> in certain fundamental JDK classes might incur bootstrap problems. >> >> The aforementioned TwR is using a "session acquisition" that is not used >> explicitly in the try block itself. This raises a warning that is suppressed >> using `@SuppressWarnings("try")`. In the future, we might be able to remove >> these suppressions by using the reserved variable name `_`. Another >> alternative was evaluated where a non-autocloseable resource was used. >> However, it became relatively complicated to guarantee that the session was >> always released and, at the same time, the existing 'final` block was always >> executed properly (as they both might throw exceptions). In the end, the >> proposed solution was much simpler and robust despite requiring a >> non-referenced TwR variable. >> >> In some cases, where is is guaranteed that the backing memory session is >> non-closeable, we do not have to guard the use of `DirectBuffer::address`. >> ~~These cases have been documented in the code.~~ >> >> On some occasions, a plurality of acquisitions are made. This would never >> lead to deadlocks as acquisitions are fundamentally concurrent counters and >> not resources that only one thread can "own". >> >> I have added comments (and not javadocs) before the declaration of the >> non-public-api `DirectBuffer::address` method, that the use of the returned >> address needs to be guarded. It can be discussed if this is preferable or >> not. >> >> This PR spawns several areas of responsibility and so, I expect more than >> one reviewer before promoting the PR. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Rework Acquisition src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 914: > 912: * If so, make a copy to put the dst data in. > 913: */ > 914: @SuppressWarnings("try") After looking at the implementation some more, I'm not sure this need fixing? E.g. this method is just using the address to compute some overlap - and return a buffer sliced accordingly. There's no access to the buffer data (except for the last part which does a `put`). The access will fail if the session is closed from underneath. I don't think this can crash the VM (in fact this code did not have a reachability fence to begin with). src/java.base/share/classes/java/nio/Buffer.java line 827: > 825: > 826: @Override > 827: public Runnable acquireSessionOrNull(Buffer buffer, > boolean async) { If allocation/performance is an issue, a relatively straightforward way to adjust the code would be to let go of the TWR entirely. E.g. we have code that does this: Buffer b = ... try { // use buffer.address(); } finally { Reference.reachabilityFence(b); } We could transform to this: Buffer b = ... acquire(b); // calls MemorySessionImpl.acquire0 if session is not null try { // use buffer.address(); } finally { release(b); // calls MemorySessionImpl.release0 if session is not null (and maybe adds a reachability fence, just in case) } This leads to a simpler patch that is probably easier to validate. The remaining IOUtil code will be using a different idiom, and perhaps we can, at some point, go back and make that consistent too. src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590: > 588: int pos) > 589: throws IOException { > 590: try (var guard = NIO_ACCESS.acquireScope(bb)) { Why was the old code not using reachability fences? Bug or feature? - PR: https://git.openjdk.org/jdk/pull/112
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:32:32 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rework Acquisition > > src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590: > >> 588: int pos) >> 589: throws IOException { >> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) { > > Why was the old code not using reachability fences? Bug or feature? I see that there's a subsequent buffer call if `n > 0`, so that's probably why the fence was skipped? (I also assume that the code calling this method will access the buffer before/after, so reachability is never truly an issue - but for session-backed buffers this needs fixing). Also, stepping back, I note how, if `receive0` was a native call using Linker, perhaps we wouldn't need all this manual address computation - we'd just get a memory segment slice from the buffer and pass that to the handle (which will perform the correct liveness check). E.g. maybe a better long term solution would be to panama-ize this code? - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote: >> This PR proposes the introduction of **guarding** of the use of >> `DirectBuffer::address` within the JDK. With the introduction of the Foreign >> Function and Memory access, it is possible to derive Buffer instances that >> are backed by native memory that, in turn, can be closed asynchronously by >> the user (and not only via a `Cleaner` when there is no other reference to >> the `Buffer` object). If another thread is invoking `MemorySession::close` >> while a thread is doing work using raw addresses, the outcome is undefined. >> This means the JVM might crash or even worse, silent modification of >> unrelated memory might occur. >> >> Design choices in this PR: >> >> There is already a method `MemorySession::whileAlive` that takes a runnable >> and that will perform the provided action while acquiring the underlying` >> MemorySession` (if any). However, using this method entailed relatively >> large changes while converting larger/nested code segments into lambdas. >> This would also risk introducing lambda capturing. So, instead, a >> try-with-resources friendly access method was added. This made is more easy >> to add guarding and did not risk lambda capturing. Also, introducing lambdas >> in certain fundamental JDK classes might incur bootstrap problems. >> >> The aforementioned TwR is using a "session acquisition" that is not used >> explicitly in the try block itself. This raises a warning that is suppressed >> using `@SuppressWarnings("try")`. In the future, we might be able to remove >> these suppressions by using the reserved variable name `_`. Another >> alternative was evaluated where a non-autocloseable resource was used. >> However, it became relatively complicated to guarantee that the session was >> always released and, at the same time, the existing 'final` block was always >> executed properly (as they both might throw exceptions). In the end, the >> proposed solution was much simpler and robust despite requiring a >> non-referenced TwR variable. >> >> In some cases, where is is guaranteed that the backing memory session is >> non-closeable, we do not have to guard the use of `DirectBuffer::address`. >> ~~These cases have been documented in the code.~~ >> >> On some occasions, a plurality of acquisitions are made. This would never >> lead to deadlocks as acquisitions are fundamentally concurrent counters and >> not resources that only one thread can "own". >> >> I have added comments (and not javadocs) before the declaration of the >> non-public-api `DirectBuffer::address` method, that the use of the returned >> address needs to be guarded. It can be discussed if this is preferable or >> not. >> >> This PR spawns several areas of responsibility and so, I expect more than >> one reviewer before promoting the PR. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Rework Acquisition src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 43: > 41: // try (var guard = NIO_ACCESS.acquireSession(bb)) { > 42: // performOperation(bb.address()); > 43: // } Again, updating this comment was missed: Suggestion: // try (var guard = NIO_ACCESS.acquireScope(bb)) { // performOperation(guard.address()); // } - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:38:35 GMT, Maurizio Cimadamore wrote: >> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590: >> >>> 588: int pos) >>> 589: throws IOException { >>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) { >> >> Why was the old code not using reachability fences? Bug or feature? > > I see that there's a subsequent buffer call if `n > 0`, so that's probably > why the fence was skipped? (I also assume that the code calling this method > will access the buffer before/after, so reachability is never truly an issue > - but for session-backed buffers this needs fixing). > > Also, stepping back, I note how, if `receive0` was a native call using > Linker, perhaps we wouldn't need all this manual address computation - we'd > just get a memory segment slice from the buffer and pass that to the handle > (which will perform the correct liveness check). E.g. maybe a better long > term solution would be to panama-ize this code? Yes, once the memory/linker APIs are permanent then the SCTP implementation would be a good candidate to redo. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:29:14 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rework Acquisition > > src/java.base/share/classes/java/nio/Buffer.java line 827: > >> 825: >> 826: @Override >> 827: public Runnable acquireSessionOrNull(Buffer buffer, >> boolean async) { > > If allocation/performance is an issue, a relatively straightforward way to > adjust the code would be to let go of the TWR entirely. E.g. we have code > that does this: > > > Buffer b = ... > try { > // use buffer.address(); > } finally { > Reference.reachabilityFence(b); > } > > > We could transform to this: > > > Buffer b = ... > acquire(b); // calls MemorySessionImpl.acquire0 if session is not null > try { > // use buffer.address(); > } finally { > release(b); // calls MemorySessionImpl.release0 if session is not null > (and maybe adds a reachability fence, just in case) > } > > This leads to a simpler patch that is probably easier to validate. The > remaining IOUtil code will be using a different idiom, and perhaps we can, at > some point, go back and make that consistent too. The AutoCloseable experiment was interesting to try but I think you are right, acquire try { .. } finally release would be simpler and also remove the concerns about the performance critical code paths. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:23:40 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rework Acquisition > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 914: > >> 912: * If so, make a copy to put the dst data in. >> 913: */ >> 914: @SuppressWarnings("try") > > After looking at the implementation some more, I'm not sure this need fixing? > E.g. this method is just using the address to compute some overlap - and > return a buffer sliced accordingly. There's no access to the buffer data > (except for the last part which does a `put`). The access will fail if the > session is closed from underneath. I don't think this can crash the VM (in > fact this code did not have a reachability fence to begin with). Well spotted. I will remove the guarding here. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 13:49:45 GMT, Per Minborg wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 914: >> >>> 912: * If so, make a copy to put the dst data in. >>> 913: */ >>> 914: @SuppressWarnings("try") >> >> After looking at the implementation some more, I'm not sure this need >> fixing? E.g. this method is just using the address to compute some overlap - >> and return a buffer sliced accordingly. There's no access to the buffer data >> (except for the last part which does a `put`). The access will fail if the >> session is closed from underneath. I don't think this can crash the VM (in >> fact this code did not have a reachability fence to begin with). > > Well spotted. I will remove the guarding here. This `@SuppressWarnings` annotation is no longer needed: Suggestion: - PR: https://git.openjdk.org/jdk/pull/11260