RFR: 8296507: GCM using more memory than necessary with in-place operations

[Mark Powers]

On Sun, 13 Nov 2022 02:54:10 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

> I would like a review of an update to the GCM code.  A recent report showed that GCM memory usage for TLS was very large.  This was a result of in-place buffers, which TLS uses, and how the code handled the combined intrinsic method during decryption.  A temporary buffer was used because the combined intrinsic does gctr before ghash which results in a bad tag.  The fix is to not use the combined intrinsic during in-place decryption and depend on the individual GHASH and CounterMode intrinsics.  Direct ByteBuffers are not affected as they are not used by the intrinsics directly.
> 
> The reduction in the memory usage boosted performance back to where it was before despite using slower intrinsics (gctr & ghash individually).  The extra memory allocation for the temporary buffer out-weighted the faster intrinsic.
> 
> 
>     JDK 17:   122913.554 ops/sec
>     JDK 19:    94885.008 ops/sec
>     Post fix: 122735.804 ops/sec 
> 
> There is no regression test because this is a memory change and test coverage already existing.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 580:

> 578:      * an upper limit on the number of blocks encrypted in the intrinsic.
> 579:      *
> 580:      * For decrypting in-place byte[], calling methods must ct must set to null

end of sentence mangled

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 592:

> 590: 
> 591:         int len = 0;
> 592:         // Loop if input length is greater than the SPLIT_LEN

comment doesn't add anything not already obvious from the code

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 596:

> 594:             int partlen;
> 595:             while (inLen >= SPLIT_LEN) {
> 596:                 partlen = implGCMCrypt0(in, inOfs + len, SPLIT_LEN, ct,

why not `int partlen` and get rid of line 594

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 603:

> 601:         }
> 602: 
> 603:         // Finish any remaining data

comment doesn't add anything special

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 694:

> 692:         int originalOutOfs = 0;
> 693: 
> 694:         // True if op is in-place array decryption with the input & output

// Setting `inPlaceArray` to true turns off combined intrinsic processing.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 764:

> 762:                 byte[] array;
> 763:                 if (encryption) {
> 764:                     array = dst.array();

You could factor out lines 764 and 770 by changing line 762 to
`byte[] array = encryption ? dst.array() : src.array();`

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1644:

> 1642:                 // Clear output data
> 1643:                 dst.reset();
> 1644:                 // If this is an in-place array, don't zero the src

The comment doesn't jive with the line of code on the next line. It is the inverse of the comment.

-------------

PR: https://git.openjdk.org/jdk/pull/11121