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.

Sign up for GitHub

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

Jump to bottom

Fix RawSocket buffer size #380

Closed
wants to merge 1 commit into from
Closed

Fix RawSocket buffer size #380

wants to merge 1 commit into from

Conversation

vlad9486
Copy link

@vlad9486 vlad9486 commented Oct 9, 2020

The buffer of RawSocket should account Ethernet header.

whitequark
whitequark requested changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. Could you please explain the exact rationale for this change?

@vlad9486
Copy link
Author

vlad9486 commented Oct 12, 2020 •
edited
Loading

I don't think this is right. Could you please explain the exact rationale for this change?

sys::RawSocketDesc::recv gives the packet which might be up to mtu bytes long plus the Ethernet header (or some other hardware header), if the packet length is not fit, it just cutoff the last bytes of payload. I actually saw it in the debugging logs.

@Dirbaio
Copy link
Member

Dirbaio commented Dec 25, 2020

The root cause of the bug is that Linux's MTU is the IP mtu, while smoltcp's MTU is the Ethernet MTU (ie, including the Ethernet header). The fix is to update RawSocketDesc to return the correct MTU, done in #393.

I've also opened #392 to track improving the docs. I totally see how this can be very confusing the way it is now.

@Dirbaio Dirbaio closed this Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitequark whitequark whitequark requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vlad9486 @Dirbaio @whitequark