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

creation of "connected socket" returns unsupported #395

Open
codemonkeylabs-de opened this issue Mar 12, 2024 · 1 comment
Open

creation of "connected socket" returns unsupported #395

codemonkeylabs-de opened this issue Mar 12, 2024 · 1 comment

Comments

@codemonkeylabs-de
Copy link

Hi, while troubleshooting performance issues regarding the integration of the boringtun library into a project I am working on, I ran into the following like of code:

socket2::Socket::new(Domain::for_address(addr), Type::STREAM, Some(Protocol::UDP))?;

Now, creating a UDP socket of type STREAM (as opposed to DGRAM) is quite likely an error since it doesn't make much sense. This call returns an IoError(Os { code: 93, kind: Uncategorized, message: "Protocol not supported" } and the wireguard peer lookup happens on every packet received in the anonymous UDP handler thus reducing throughput to a crawl.

The fix is ofc easy enough (i.e. changing the socket type to DGRAM), I tried this myself and performance is as expected greatly improved to at least the same order of magnitude as the cloudflare warp app.. but this is what bugs me: beyond the actual problem, it strikes me as quite unlikely this is the code that cloudflare uses in production; I mean, a bug like this suggests perhaps I am not using the correct branch? Comments welcome. Thanks!

TimFroidcoeur pushed a commit to TimFroidcoeur/boringtun that referenced this issue Apr 3, 2024
cloudflare#395

connected sock needs to be DGRAM

Signed-off-by: Tim Froidcoeur <tim.froidcoeur@gmail.com>
TimFroidcoeur pushed a commit to Tessares/boringtun that referenced this issue Apr 3, 2024
cloudflare#395

connected sock needs to be DGRAM

Signed-off-by: Tim Froidcoeur <tim.froidcoeur@gmail.com>
TimFroidcoeur pushed a commit to Tessares/boringtun that referenced this issue Apr 3, 2024
connected sock needs to be DGRAM

see cloudflare#395

this has substantial performance impact on linux

Signed-off-by: Tim Froidcoeur <tim.froidcoeur@gmail.com>
@givascu
Copy link

givascu commented May 27, 2024

I've recently run into this too and can confirm that this typo (most likely Type::STREAM wasn't intended there) dramatically reduces the throughput when running boringtun with the "connected UDP socket" config ON (the default). Fortunately the fix is trivial enough such that it should be addressed sooner rather than later.

P.S. A workaround for those unable to patch the library themselves would be to set use_connected_socket to false in the device config or to pass the --disable-connected-udp option when running boringtun-cli (for the executable users).

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

No branches or pull requests

2 participants