Bugzilla – Bug 2124
UdpSocketImpl::ShutdownRecv doesn't stop the Ipv[4,6]EndPointDemux
Last modified: 2015-07-02 12:26:41 EDT
Created attachment 2046 [details] This is the function where one adds the send and listen socket for an interface on a node for AODV Ipv6 implementation. I have commented the difference between the source of error and makearound I am working on AODV ipv6 implementation. Overview: Sending sockets are bound to interface by their corresponding Ipv6 address (Link-local) and UDP port (AODV_PORT = 654) in the NotifyInterfaceUp and NotifyAddAddress functions. On the other hand, we have a single receive socket bound to any Ipv6 address (Ipv6Address::GetAny) and UDP port (AODV_PORT = 654). Technically, the listen socket must be able to detect any Ipv6 packet coming on the corresponding UDP port and activate the corresponding callback function bound to the receive socket. But, the UDP socket which is responsible for listening, is not making the callback for unicast packets. Steps to Reproduce: 1. create send and receive sockets exactly as described in the RIPng protocol in the NotifyInterfaceUp function of AODV Ipv6 implementation (which I have partly shared with Tommasso). Without this implementation it is hard to reproduce. 2.You will notice that the routing protocol works well for sending and listening to broadcast packets, but when the first unicast packet is sent, the packet arrives at the receiver, moves all the way from Phy to UDP, but at UDP, the corresponding callback is not available. Actual Result: Unicast packet is dropped at UDP layer and not processed Expected Result: The listen socket should take control of the received unicast packet and make the necessary callback to transfer control to handle a unicast packet. Proposed makearound: Add a receive callback function on the sender socket as well. Thus when the packet arrives on the UDP port, and UDP handles this packet, the tuple of ipv6endpointdemux has a match to the received address and the corresponding callback is initiated.
Some clarification is due. Shashidhar is developing AODV for IPv6 and he got a practical issue. However, this issue is present in IPv4 and IPv6. Suppose you have two sockets, one bound to Any and one to one of your addresses. Moreover, you want to shut down the receiving on the address-bound socket. Upon receiving a packet, the Ip Demux will search for a socket, and will prefer the sockets bound to the address the packet is sent to over the ones bound to Any. As a consequence, the packet is sent to the socket with receiving shut down and it is discarded. Note that this doesn't happen with TCP. Unless, of course, you open a socket in accept and shut its receiving. Quite... uncommon. There are several ways to overcome this issue: 1) Avoid to have this situation (not that simple on the long run). 2) Have a "confirmation" that the socket has processed the packet, more or less like routing. 3) Why a socket that can't receive packets is in the Endpoint Demux ? Option 3 is simply a matter of calling DeallocateEndPoint (); in UdpSocketImpl::ShutdownRecv. Opinions and tests please.
Another user being hit by this bug: https://groups.google.com/forum/#!topic/ns-3-users/DXeRiE90Yr0
> > Option 3 is simply a matter of calling DeallocateEndPoint (); in > UdpSocketImpl::ShutdownRecv. > Why not test this simple patch? Is there an issue with it?
(In reply to Tom Henderson from comment #3) > > > > Option 3 is simply a matter of calling DeallocateEndPoint (); in > > UdpSocketImpl::ShutdownRecv. > > > > Why not test this simple patch? Is there an issue with it? I forgot to mention... All the tests passes and it seems to fix the user's problems. Cheers
(In reply to Tom Henderson from comment #3) > > > > Option 3 is simply a matter of calling DeallocateEndPoint (); in > > UdpSocketImpl::ShutdownRecv. > > > > Why not test this simple patch? Is there an issue with it? The "issue" is that, after deallocating the endpoint, there's no way to build another one. Basically it means that you can't "undo" a ShutdownRecv. This is not a big issue, as there's no public method to revert a ShutdownRecv. Still... I have too many doubts recently.
(In reply to Tommaso Pecorella from comment #5) > (In reply to Tom Henderson from comment #3) > > > > > > Option 3 is simply a matter of calling DeallocateEndPoint (); in > > > UdpSocketImpl::ShutdownRecv. > > > > > > > Why not test this simple patch? Is there an issue with it? > > The "issue" is that, after deallocating the endpoint, there's no way to > build another one. Basically it means that you can't "undo" a ShutdownRecv. > This is not a big issue, as there's no public method to revert a > ShutdownRecv. > Still... I have too many doubts recently. Scratch the patch. I have to learn to trust my guts. The endpoint is holding the sending port as well. If we delete it, we'll: 1) risk to have the port re-assigned, and 2) the Send function will call a disposable object. Definitely not a good idea. The patch is more complex.
Created attachment 2072 [details] real patch This is more intrusive, but it seems to work. What's new. Ipv[4,6]EndPoint has 2 more public functions and one private member variable. The two public functions are "DisableRx ()" and "IsRxEnabled ()". Guess how they're used... Ipv[4,6]EndPointDemux will skip any endpoint not Rx enabled in the Lookup (...) function. Note that LookupLocal and LookupPortLocal are unaffected, and they must be unaffected. UdpSocketImpl doesn't deallocate the endpoints when the rx is shut down. It simply changes the flag in its EndPoints. TCP is untouched, and I don't think it should be touched thanks to how socket are used by TCP.
(In reply to Tommaso Pecorella from comment #7) > TCP is untouched, and I don't think it should be touched thanks to how > socket are used by TCP. If I understand, this point: > Suppose you have two sockets, one bound to Any and one to one of your > addresses. > Moreover, you want to shut down the receiving on the address-bound socket. Is not applicable to TCP, since receiver should be always on (the ACKs..). So the bug cannot be reproducible on TCP; however, in your opinion, why not adding also DisableTx and IsTxEnabled ? And, there can be the possibility to "give life again" to a shutted-down socket, for example with EnableRx ?
(In reply to Tommaso Pecorella from comment #7) > Created attachment 2072 [details] > real patch > > This is more intrusive, but it seems to work. > I'd suggest to change DisableRx (void) to SetRxEnabled (bool enabled) since it is just a flag and who knows what use case might come up in the future to enable the endpoint again? Otherwise, it looks fine to me.
changeset: 11470:0b3d6135a28b About the idea of having a IsTxEnabled / DisableTx, I didn't add it because EndPoints are used for DeMuxing. I.e., they are not used when sending packets. (In reply to natale.patriciello from comment #8) > (In reply to Tommaso Pecorella from comment #7) > > TCP is untouched, and I don't think it should be touched thanks to how > > socket are used by TCP. > > > If I understand, this point: > > > Suppose you have two sockets, one bound to Any and one to one of your > > addresses. > > Moreover, you want to shut down the receiving on the address-bound socket. > > Is not applicable to TCP, since receiver should be always on (the ACKs..). > So the bug cannot be reproducible on TCP; however, in your opinion, why not > adding also DisableTx and IsTxEnabled ? And, there can be the possibility to > "give life again" to a shutted-down socket, for example with EnableRx ?