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

added io_uring support #3646

Closed
wants to merge 3 commits into from
Closed

Conversation

Outfluencer
Copy link
Collaborator

@Outfluencer Outfluencer commented Apr 8, 2024

io_uring is still experimental, but its now possible to activate it with the -Dbungee.io_uring=true argument

Maybe someone would like to test io_uring for performace or any issues

proxy/pom.xml Show resolved Hide resolved
@Janmm14
Copy link
Contributor

Janmm14 commented Apr 8, 2024

In other PRs, done to PandaSpigot and Paper, it was mentioned that compression wouldn't work together with this.
It might be a good idea to test this out, maybe also with different combinations of compression thresholds on bungee & server (like default, 1, and -1 or so as thresholds).

Hopefully netty works in a way our native decompress doesn't attempt to do it in the place where io_uring is doing its stuff

Related to security concerns in those PRs, the security problem is apparantly related to unsafe code using io_uring. Whether bungee or paper are using io_uring, we do not contribute unsafe code, so I do not think that security is a reason to not accept this PR.

@md-5
Copy link
Member

md-5 commented Apr 8, 2024

You'd think it'd be immediately noticeable if compression didn't work...

@Outfluencer
Copy link
Collaborator Author

You'd think it'd be immediately noticeable if compression didn't work...

yes should be noticeable lmao

@andreasdc
Copy link

In these PRs it's mentioned that compressed packets can't be decoded. I would also check if it's safe to use io_uring in terms of exploits etc.

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Apr 8, 2024

In other PRs, done to PandaSpigot and Paper, it was mentioned that compression wouldn't work together with this. It might be a good idea to test this out, maybe also with different combinations of compression thresholds on bungee & server (like default, 1, and -1 or so as thresholds).

Hopefully netty works in a way our native decompress doesn't attempt to do it in the place where io_uring is doing its stuff

Related to security concerns in those PRs, the security problem is apparantly related to unsafe code using io_uring. Whether bungee or paper are using io_uring, we do not contribute unsafe code, so I do not think that security is a reason to not accept this PR.

i tested all cases and got no issues or exception everything was stable for me in any case

@Outfluencer
Copy link
Collaborator Author

In these PRs it's mentioned that compressed packets can't be decoded. I would also check if it's safe to use io_uring in terms of exploits etc.

what you mean with exploits?

@md-5
Copy link
Member

md-5 commented Apr 8, 2024

In these PRs it's mentioned that compressed packets can't be decoded.

Chunk packets are compressed, so if this was the case (and assuming Outfluencer tested joining a normal Spigot server with default config) this PR just wouldn't work. Sounds like an issue with those other PRs, which is irrelevant

@Outfluencer
Copy link
Collaborator Author

In these PRs it's mentioned that compressed packets can't be decoded.

Chunk packets are compressed, so if this was the case (and assuming Outfluencer tested joining a normal Spigot server with default config) this PR just wouldn't work. Sounds like an issue with those other PRs, which is irrelevant

I did

@andreasdc
Copy link

andreasdc commented Apr 8, 2024

In these PRs it's mentioned that compressed packets can't be decoded. I would also check if it's safe to use io_uring in terms of exploits etc.

what you mean with exploits?

I just looked some topics up, so I'm curious if it's even relevant in our scenario. If there are no problems with packets etc. I volunteer to test it of course. :)
https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html
(android only?) https://i.blackhat.com/BH-US-23/Presentations/US-23-Lin-bad_io_uring.pdf?ref=blog.isosceles.com

md-5 pushed a commit that referenced this pull request Apr 9, 2024
@md-5 md-5 closed this Apr 9, 2024
@andreasdc
Copy link

When using io_uring on bukkit side too, I'm getting disconnected with: Exception Connecting: DecoderException : net.md_5.bungee.protocol.BadPacketException: Uncompressed size 2 is less than threshold 512 @ io.netty.handler.codec.MessageToMessageDecoder:98

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Apr 10, 2024

When using io_uring on bukkit side too, I'm getting disconnected with: Exception Connecting: DecoderException : net.md_5.bungee.protocol.BadPacketException: Uncompressed size 2 is less than threshold 512 @ io.netty.handler.codec.MessageToMessageDecoder:98

irrelevant for us, thats a problem of the vanilla impl and not a problem of bungeecord

Do you create a custom spigot/bukkit server to test it there?

@andreasdc
Copy link

andreasdc commented Apr 10, 2024

That's right, I used this implementation: hpfxd/PandaSpigot#129 or PaperMC/Paper#9141
BTW I'm testing this io_uring patch on bungee, but I can't test it along with bukkit's implementation.

@Outfluencer
Copy link
Collaborator Author

That's right, I used this implementation: hpfxd/PandaSpigot#129 or PaperMC/Paper#9141 BTW I'm testing this io_uring patch on bungee, but I can't test it along with bukkit's implementation.

yes because the panda and paper spigot impls are broken, that has nothing to do with bungeecord at all, please tell us if you find any issues with this enabled and normal software like (vanilla/spigot/paper)

@andreasdc
Copy link

I don't see any issues, but I don't use pure BungeeCord. Some benchmarks would be awesome, I'm not sure if it improves performance.

thxrben pushed a commit to thxrben/BungeeCord that referenced this pull request Jun 16, 2024
@andreasdc
Copy link

andreasdc commented Aug 23, 2024

I think using this option makes the server vulnerable to some kind of attacks, probably some packet spam and the attacked netty thread is being lagged out.
I see NativeIoException : io_uring read(..) failed: Connection reset by peer in logs which is not typical.

@Janmm14
Copy link
Contributor

Janmm14 commented Aug 23, 2024

Just seeing that in the log doesn't make a server vulnerable.

@andreasdc
Copy link

andreasdc commented Aug 23, 2024

Just seeing that in the log doesn't make a server vulnerable.

Dropping all players from this netty thread (I believe) I think it means that the server is vulnerable. 😕

@Outfluencer
Copy link
Collaborator Author

Just seeing that in the log doesn't make a server vulnerable.

Dropping all players from this netty thread (I believe) I think it means that the server is vulnerable. 😕

You can also spam the netty thread with nio or epoll

@andreasdc
Copy link

andreasdc commented Aug 23, 2024

Just seeing that in the log doesn't make a server vulnerable.

Dropping all players from this netty thread (I believe) I think it means that the server is vulnerable. 😕

You can also spam the netty thread with nio or epoll

I think it's not so vulnerable, I didn't see that with nio/epoll. Spamming the thread is only my guess. My guess is the netty thread is being lagged out earlier than it would be in nio/epoll.
I saw also that the outgoing packets (sent from the server to the players) were coming normally to the players, but incoming packets (from the players to the server) were problematic and lagged out.

@andreasdc
Copy link

Packet limiter didn't fix the issue.

@Outfluencer Outfluencer deleted the io_uring-impl branch September 1, 2024 15:39
@andreasdc
Copy link

Is it unfixable?

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.

4 participants