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

Sending packets from sockets bound to 0.0.0.0 #3110

Open
stevenengler opened this issue Aug 17, 2023 · 0 comments
Open

Sending packets from sockets bound to 0.0.0.0 #3110

stevenengler opened this issue Aug 17, 2023 · 0 comments
Labels
Type: Bug Error or flaw producing unexpected results

Comments

@stevenengler
Copy link
Contributor

In Shadow when a socket has data ready to send, it calls host.notify_socket_has_packets(interface_ip, &socket) with some interface_ip value, which looks up a network interface object for the given IP address and calls interface.add_data_source(&socket). This adds the socket to the network interface's queue (either fifo or round-robin). Later when the network interface can send data it pops a socket from the queue, pops a packet from the socket, possibly re-adds the socket to the queue if it still has more packets to send, and then eventually sends the popped packet over the network.

This works fine when a socket only sends data over a single interface, but sockets bound to 0.0.0.0 may send packets over multiple interfaces (for example loopback and eth). With UDP sockets, the application can use sendto() to send messages to multiple addresses. For example a UDP socket can send a message to 127.0.0.1 and then immediately send another message to 8.8.8.8. With TCP sockets, a listening socket may reply to SYN packets from multiple IP addresses. For example after receiving SYN packets from both 127.0.0.1 and 8.8.8.8, the listening socket may send SYN responses back to both those IP addresses. But this leads to the question: When a socket is sending packets to multiple IP addresses on different network interfaces, which network interface should it call host.notify_socket_has_packets(interface_ip, &socket) for? Should it choose one interface, or should it call that method on all interfaces? Should it look at the next pending packet and call it on the interface for that packet?

Examples

In the following examples, we consider a socket that wants to send packets on both the loopback and the "internet" interfaces.

1. Calling host.notify_socket_has_packets(interface_ip, &socket) on the internet interface

We could always notify the internet interface for all packets, regardless of if they're destined for localhost or the network. The current relay code also has a check is_local = src.get_address() == *packet.dst_address().ip() to avoid decrementing the token bucket if the packet is destined for the local machine[1]. But this then leads to issues like #2677 where even though local packets don't count towards the token bucket, if the token bucket does run out due to non-local packets it will still prevent any future local packets from being send until the token bucket can refill.

[1] This code technically only checks if the packet's destination is the public IP of the host, but we could also modify it to check if the packet's destination is 127.0.0.0/8 and apply the same logic. This would not fix the issue described here though.

2. Calling host.notify_socket_has_packets(interface_ip, &socket) on the loopback interface

We could always notify the loopback interface for all packets, regardless of if they're destined for localhost or the network. But this would mean that these network-destined packets wouldn't count towards the relay's token buckets, and they probably wouldn't get routed properly anyway.

3. Calling host.notify_socket_has_packets(interface_ip, &socket) on the interface required by the next pending packet

Instead of notifying the same interface for every packet, the socket could peek the next packet and notify the network interface that the packet should be routed over (implementing a basic routing table within the socket). The problem here is that if the socket has a queue of packets to send, the socket could end up being added to multiple network interfaces at the same time. For example the socket sees that the next packet is to 127.0.0.1 so notifies the loopback interface. The loopback interface later pops the packet from the socket, and since the socket still has more pending sockets, keeps the socket in the interface's socket queue. Later the socket peeks the next packet sees that the next packet is to 8.8.8.8, so notifies the internet interface. Now the socket exists in the queues of both network interfaces and there is now a chance that when the network interface pops a packet from the socket, it will be for the wrong interface. The behaviour here is a little hard to reason about, but I don't think it works as intended.

Current behaviour

Currently TCP sockets take approach (3) and UDP sockets take approach (1).

Possible solutions

I'm not really sure. We need to redesign the network interface code when we move it to rust anyway. In general, I think it would be better to have a better routing table implemented in the host to redirect packets to the correct network interface. With better routing, we might be able to avoid needing special cases in the network interface for packets being sent on the wrong interface, which would make the code easier to reason about. The network interface also probably shouldn't re-add the socket to its queue after it pops a packet from the socket, and should instead have the socket figure out on its own if it should re-notify the network interface (or a different network interface).

@stevenengler stevenengler added the Type: Bug Error or flaw producing unexpected results label Aug 17, 2023
@stevenengler stevenengler changed the title Sending packets from sockets bound to 0.0.0.0 Notifying the network interface when sending packets from sockets bound to 0.0.0.0 Aug 17, 2023
@stevenengler stevenengler changed the title Notifying the network interface when sending packets from sockets bound to 0.0.0.0 Sending packets from sockets bound to 0.0.0.0 Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Error or flaw producing unexpected results
Projects
None yet
Development

No branches or pull requests

1 participant