Bug 299

Summary: promiscuous code breaks raw packet sockets
Product: ns-3 Reporter: Mathieu Lacage <mathieu.lacage>
Component: networkAssignee: ns-bugs <ns-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: gjcarneiro
Priority: P1    
Version: pre-release   
Hardware: All   
OS: All   

Description Mathieu Lacage 2008-08-28 18:23:22 UTC
The following changeset introduced a check in PacketSocket::ForwardUp for PACKET_HOST and that breaks packet sockets because they can't erceive anymore packets from the device unless it has been put in promiscuous mode

changeset:   3448:0bd851bb1225
user:        Gustavo J. A. M. Carneiro  <gjc@inescporto.pt>
date:        Mon Jul 07 12:18:05 2008 +0100
summary:     Simplify promiscuous mode API: NetDevices always operate in promiscuous mode, normal receive callbacks receive extra destination address and packet type.
Comment 1 Mathieu Lacage 2008-08-28 18:39:19 UTC
reverting the above-mentioned changeset fixes the problem:


diff -r 92ef80f0352e src/node/packet-socket.cc
--- a/src/node/packet-socket.cc Thu Aug 28 15:06:49 2008 -0700
+++ b/src/node/packet-socket.cc Thu Aug 28 15:39:02 2008 -0700
@@ -353,10 +353,6 @@ PacketSocket::ForwardUp (Ptr<NetDevice> 
     {
       return;
     }
-  if (packetType != NetDevice::PACKET_HOST)
-    {
-      return;
-    }
 
 
   PacketSocketAddress address;
Comment 2 Mathieu Lacage 2008-08-28 18:57:05 UTC
looking at this some more, it's pretty clear that the underlying problem here come from the Node::RegisterProtocolHandler API support for promiscuous mode. From the documentation:
   * \param packetType type of packet received
   *                   (broadcast/multicast/unicast/otherhost); Note:
   *                   this value is only valid for promiscuous mode
   *                   protocol handlers.

but, if you look around, you can see that the protocol handlers are never told when they are called in "promiscuous" mode so, the field is invalid but you have no way of knowing whether it is valid or not. 

Options:
1) add an extra argument to protocol handlers to convey promiscuous state
2) add a value to enum NetDevice::PacketType to document invalidality
3) get rid of both receive callbacks and make the devices themselves report the information so PacketType is valid all the time.

2) is the least intrusive.
Comment 3 Gustavo J. A. M. Carneiro 2008-08-28 19:45:10 UTC
(In reply to comment #1)
> reverting the above-mentioned changeset fixes the problem:
> 
> 
> diff -r 92ef80f0352e src/node/packet-socket.cc
> --- a/src/node/packet-socket.cc Thu Aug 28 15:06:49 2008 -0700
> +++ b/src/node/packet-socket.cc Thu Aug 28 15:39:02 2008 -0700
> @@ -353,10 +353,6 @@ PacketSocket::ForwardUp (Ptr<NetDevice> 
>      {
>        return;
>      }
> -  if (packetType != NetDevice::PACKET_HOST)
> -    {
> -      return;
> -    }
> 
> 
>    PacketSocketAddress address;
> 

This is actually the correct fix.  This code is a result of back and forth changes:

 1- First you insisted on a single "upcall path" for both promiscuous and non-promiscous packets.  I implemented that and had to put the PacketType filtering code in several places to prevent the stack from collapsing as soon as promiscuous mode was activated;

 2- Later Craig sent an email saying different paths would be better/safer, but wanted only a single protocol handler callback signature.  I changed to do so and non-promiscuous protocol handlers no longer needed the filtering.  Thus I removed the filtering code again from several places, but must have forgotten this particular place :-/

Note that there is no "promiscuous mode" as such.  There are protocol handlers registered promiscuously (notice the last parameter in Node::RegisterProtocolHandler) which receive a valid PacketType parameter, and there are protocol handlers registered in normally, which receive a dummy PacketType parameter but should not read it.

There is no need to pass a parameter to a protocol handler saying if it receives a promiscuous packet or not.  The programmer is the one that registers promiscuously or not, and so it is the one that designs the callback to act accordingly to the promiscuous flag.
Comment 4 Craig Dowell 2008-08-29 17:35:30 UTC
Need to make it so that packet socket regression test fails in this case, then fix it.
Comment 5 Mathieu Lacage 2008-08-29 23:40:37 UTC
changeset 01bebf28addd