Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check for SSE 4.2 and PCLMUL support to native zlib, add system properties for disabling natives #3718

Closed
wants to merge 4 commits into from

Conversation

lax1dude
Copy link
Contributor

@lax1dude lax1dude commented Aug 4, 2024

Fix for #3717

I implemented a function in native-compress.so to check the CPUID registers if the required features for the cloudflare zlib library are supported on the CPU or not, by checking for the SSE 4.2 and PCLMUL bits. My implementation is only compatible with GCC as far as I know. This should probably be tested on real hardware, I haven't verified if the function actually returns false on a CPU that doesn't support SSE 4.2 or PCLMUL. I have only verified that the Java exception is handled correctly when I change the function to return false manually.

I also tested the binary on both Debian and Alpine Linux to make sure there are no C library portability issues like we had with the SSL library.

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 4, 2024

Added the following system properties for manually disabling natives:

  • net.md_5.bungee.jni.native-compress.enable (true/false)
  • net.md_5.bungee.jni.native-cipher.enable (true/false)

Example usage:

java -Dnet.md_5.bungee.jni.native-compress.enable=false -jar BungeeCord.jar

@lax1dude lax1dude changed the title Add check for SSE 4.2 and PCLMUL support to native zlib Add check for SSE 4.2 and PCLMUL support to native zlib, add system properties for disabling natives Aug 4, 2024
@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 6, 2024

Alright I tested this on a pentium-based laptop from 2006 running debian and a pretty old kernel, it does correctly detect the unsupported CPU and print the message to the console without crashing, and uses the Java library instead. However, it doesn't appear to detect the unsupported CPU until I actually connect to the server. The log messages printed when BungeeCord start up initially say that its using the native compression library.

@md-5
Copy link
Member

md-5 commented Aug 6, 2024

I guess you need to move the checking code from newInstance to load to fix that

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 7, 2024

Fixed, here's the output on the pentium-based laptop from 2006:

21:26:15 [INFO] Using mbed TLS based native cipher.
21:26:15 [INFO] Native library native-compress is incompatible: This CPU does not support the required
SSE 4.2 and/or PCLMUL extensions!
21:26:15 [INFO] Using standard Java compressor.
21:26:15 [INFO] Enabled BungeeCord version git:BungeeCord-Bootstrap:1.21-R0.1-SNAPSHOT:14767de:unknown
21:26:15 [INFO] Not on Windows, attempting to use enhanced EpollEventLoop
21:26:15 [INFO] Epoll is working, utilising it!
21:26:15 [INFO] Couldn't detect bungee version. Custom build?
21:26:16 [WARNING] Forced host server pvp is not defined
21:26:16 [INFO] Listening on /0.0.0.0:25577

@md-5
Copy link
Member

md-5 commented Aug 7, 2024

How come it says
21:26:15 [INFO] Using mbed TLS based native cipher.

and then

21:26:15 [INFO] Native library native-compress is incompatible: This CPU does not support the required
SSE 4.2 and/or PCLMUL extensions!

Eyeballing the code, it should only be the second message

@Janmm14
Copy link
Contributor

Janmm14 commented Aug 7, 2024

How come it says 21:26:15 [INFO] Using mbed TLS based native cipher.

and then

21:26:15 [INFO] Native library native-compress is incompatible: This CPU does not support the required SSE 4.2 and/or PCLMUL extensions!

Eyeballing the code, it should only be the second message

cipher and compress are two different things. for cipher/mbedtls maybe a different check is needed, if at all? right now this is just about zlib.

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 7, 2024

I only implemented the SSE 4.2 check for the compression natives, the cipher doesn't use SSE, so there's no need to disable it. Only the compression library uses features we can't expect every CPU made in the past 2 decades to support.

@md-5
Copy link
Member

md-5 commented Aug 7, 2024

Whoops, my mistake. Sorry

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 8, 2024

It looks like mbedtls already has all the CPUID checks baked directly into it, and selects a supported implementation at runtime

@md-5
Copy link
Member

md-5 commented Aug 8, 2024

Are we sure both of these are required?

It's failing on Jenkins which was historically fine:

Native library native-compress is incompatible: This CPU does not support the required SSE 4.2 and/or PCLMUL extensions!
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.027 sec <<< FAILURE!
net.md_5.bungee.NativeZlibTest.testException()  Time elapsed: 0.025 sec  <<< FAILURE!
org.opentest4j.AssertionFailedError: Native code failed to load! ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at net.md_5.bungee.NativeZlibTest.testException(NativeZlibTest.java:36)

Are you sure pclmul is required? cpuinfo for the QEMU vCPU has sse4_2 but no pclmulqdq

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 8, 2024

I looked through the code a bit and used by the CRC32 algorithm. Apparently the CRC32 function is only used for gzip, not deflate (this is news to me), so I guess if we can verify that the compression natives will only ever need to handle deflate and not gzip we could disable gzip support and compile the library for deflate/inflate only. I'm not sure if zlib's inflate function automatically detects gzip or not or if the gzip handling code is never even called in the first place and optimized away.

https://github.com/cloudflare/zlib/blob/gcc.amd64/crc32_simd.c

@md-5
Copy link
Member

md-5 commented Aug 8, 2024

disable gzip support and compile the library for deflate/inflate only.

Is there a way to do this?

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 8, 2024

Is there a way to do this?

I'm 90% sure there's a simple preprocessor directive we can set to compile zlib without gzip support. I'll try it out when I get home from work, I get off in about 2.5hrs.

@md-5
Copy link
Member

md-5 commented Aug 8, 2024

No rush, thanks

@lax1dude
Copy link
Contributor Author

lax1dude commented Aug 9, 2024

#3722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants