-
Notifications
You must be signed in to change notification settings - Fork 429
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
Fix mismatching src ip address in TCP socket transmissions #970
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what the motivation is, could you elaborate on that please?
Ahh, sorry. I assumed the linked issue was enough context. Sorry about that. In my case, I am running a single smoltcp stack, but changing between two PPP based interfaces in a super crude way (because smoltcp does not support actual multi-interface routing). This means that the src address on any open socket, will no longer match the address when the socket was opened, but the socket stays established and still transmits packets with the old source IP address. This PR is an attempt at making sure a host never transmits packets with a source IP that's not its own, and at the same time closes the socket, allowing socket consumers to immediately notice and reconnect. |
This feels like a fairly niche use case that I'm not sure we should support in the core library. In particular the lack of ability to notify the upper levels of connectivity change feels like a design issue that isn't necessarily something that should have a workaround in smoltcp. |
While I agree, my own use case is niche, this PR doesn't really address my use case in any particular way. This could just as well happen from plugging an ethernet cable into a different network, which feels like something that should absolutely be handled in smoltcp, as the alternative is to emit garbage into the network? The discussion on whether this is the right approach to handling this, or not, is the primary reason I opened this PR still with a "FIXME" comment in the code. |
Thanks for elaborating on the context! |
yeah I'm seeing the bug in a standard ethernet+dhcp network, when you plug into a different network, or when the DHCP IP changes. That's not niche 🙃 |
How do we drive this any further? |
I'm travelling this week but could take another look after. |
…/mismatching-src-ip
Any progress on this? |
…/mismatching-src-ip
This PR is still in my queue. I'll look at it after I quit my current job at the end of Nov. |
This is an attempt to close TCP sockets if the src address changes, while the socket is open.
I have been testing this on my application, and it seems to do the trick. I have also tried with both
socket.close()
andsocket.abort()
in place of thesocket.reset()
call currently in here, but neither of them quite seems to do the trick, as the tuple is not set tonone
?I am not a fan of making
reset
public here, so this is mostly an attempt at getting some feedback from you guys?Fixes #466