Bug 827

this bug is invalid. The correct method to put a wifi device in monitor mode is to use YansWifiPhyHelper::EnablePcap(). See examples/wireless/wifi-wired-bridging.cc for an example. 

If you have further doubts about this issue, please write to the ns-3-users mailing list.

Re-opening this bug after the discussion started by Ruben, see this thread:
http://mailman.isi.edu/pipermail/ns-developers/2011-January/008559.html

Created attachment 1026 [details]
Ruben's revised patch

http://mailman.isi.edu/pipermail/ns-developers/2011-January/008570.html

In reply to this email:
http://mailman.isi.edu/pipermail/ns-developers/2011-January/008570.html

>> 
>> In principle the best approach would be to use 
>> NetDevice::SetPromiscReceiveCallback() as done by other devices,
>> rather than using a wifi-specific method. But in practice this
>> might gave problems, see below comments.
> 
> Well, that's not enough. Enabling NeDevice::SetPromiscReceiveCallback
> by calling Node::RegisterProtocolHandler with "bool promiscuous" set
> to true will not do anything with a WifiNetDevice. Indeed, in 
> MacLow::ReceiveOk, all packets with a destination MAC address other
> than the one of the receiver are ignored. So, if you want it to work
> fully, you need to modify MacLow::ReceiveOk as per the patch.

Here I was referring to the API only, not to the implementation. I agree
that, without the modifications in mac-low, the code won't work as you
need. Sorry for the confusion.


> Now, thanks to the modifications that went into ns-3.10, the patch is
>  now much cleaner. It only touches WifiNetDevice, RegularWifiMac and
> MacLow.
> 
[...]
>> The Wifi "MAC high" model underwent significant modifications
>> since ns-3.10, so that needs to be considered for this patch to be
>> accepted. In particular, if you need to add any new MAC high
>> functionality, the best approach is to do it in RegularWifiMac, if
>> possible, so that all the MAC high models (adhoc, ap, sta) inherit
>> this functionality.
> 
> That's what I've done
> 

Great, thanks. Your patch is now ok as far as MAC high is concerned.


>> I mean, with your proposed modifications, will the
>> wifi-wired-bridging example still work? Maybe yes with the example
>> program as-is, but what if you add a different wifi network with a
>> different SSID on the same channel? will the AP start forwarding
>> some packets it shouldn't?
> 
> I haven't looked into this example. I can try it out.

This would be very much appreciated!


> 
> With promiscuous mode, you will not capture packets with a different
> SSID.
> 

(Sorry I meant BSSID, not SSID)

There is basically no explicit bssid check performed for data packets in
the ns-3 wifi code, so my understanding is that your patch would break
the case of using wifi-wired-bridging where another network with a
different BSSID is added.

Adding an explicit BSSID check separated from the receiver address check
would probably make your patch work with wifi-wired-bridging. Still, if
there is an explicit BSSID check, then enabling promisc mode on an AP
has no effect, because in packets with ToDs=1 and FromDs=0 the BSSID is
in the Address1 field, and corresponds to the RA (IEEE Std. 802.11-2007,
Table 7.7). Hence only packets addressed to the MAC address of the
device will be forwarded to the upper layer (in spite of promisc mode
being active).


> 
> IMHO, monitor mode is not promiscuous mode. Frames received with
> monitor mode have a radiotap header (or some other header) attached.
> My solution just make NetDevice::SetPromiscRxCallback functional for
> Wifi devices.

I agree, but the two modes (monitor & promisc) are often mistaken. In
fact, there are several methods related to monitor mode that have
"promisc" in the name. I suggest to change them to "monitor" to avoid
ambiguity.

One minor comment. In MacLow::SetRxCallback :
 
  else if ((m_promisc) && (hdr.GetAddr1 () != m_self)) 

why the second test? 
Maybe we could do:

  else if (m_promisc) 
     {
       NS_ASSERT (hdr.GetAddr1 () != m_self);

(In reply to comment #4)

[snip]

> Great, thanks. Your patch is now ok as far as MAC high is concerned.

Do you need a tad more documentation? If yes, I'll update the patch.

> 
> >> I mean, with your proposed modifications, will the
> >> wifi-wired-bridging example still work? Maybe yes with the example
> >> program as-is, but what if you add a different wifi network with a
> >> different SSID on the same channel? will the AP start forwarding
> >> some packets it shouldn't?
> > 
> > I haven't looked into this example. I can try it out.
> 
> This would be very much appreciated!

Will do.
 
> 
> > 
> > With promiscuous mode, you will not capture packets with a different
> > SSID.
> > 
> 
> (Sorry I meant BSSID, not SSID)

Ah, so for BSSID, yes you will capture packets for a different BSSID. Indeed, this behavior should be checked.

> There is basically no explicit bssid check performed for data packets in
> the ns-3 wifi code, so my understanding is that your patch would break
> the case of using wifi-wired-bridging where another network with a
> different BSSID is added.

I haven't look into the example yet, so I don't know yet what is the scenario. But as I said above, I can check. Now, I'm not sure this would "break" the example. This would just be a "collateral" damage of misusing a feature.

> Adding an explicit BSSID check separated from the receiver address check
> would probably make your patch work with wifi-wired-bridging. Still, if
> there is an explicit BSSID check, then enabling promisc mode on an AP
> has no effect, because in packets with ToDs=1 and FromDs=0 the BSSID is
> in the Address1 field, and corresponds to the RA (IEEE Std. 802.11-2007,
> Table 7.7). Hence only packets addressed to the MAC address of the
> device will be forwarded to the upper layer (in spite of promisc mode
> being active).

Ok, will have a look to.

 
> > 
> > IMHO, monitor mode is not promiscuous mode. Frames received with
> > monitor mode have a radiotap header (or some other header) attached.
> > My solution just make NetDevice::SetPromiscRxCallback functional for
> > Wifi devices.
> 
> I agree, but the two modes (monitor & promisc) are often mistaken. In
> fact, there are several methods related to monitor mode that have
> "promisc" in the name. I suggest to change them to "monitor" to avoid
> ambiguity.

A quick grep in the code points to wifi-phy.cc and in particular m_phyPromiscSniffRxTrace and m_phyPromiscSniffTxTrace. I can investigate and produce a patch.

(In reply to comment #5)
> One minor comment. In MacLow::SetRxCallback :
> 
>   else if ((m_promisc) && (hdr.GetAddr1 () != m_self)) 
> 
> why the second test? 
> Maybe we could do:
> 
>   else if (m_promisc) 
>      {
>        NS_ASSERT (hdr.GetAddr1 () != m_self);

Yes, of course. It makes sense. I will produce a new patch once I have tested the bridge example.

(In reply to comment #6)

[snip]

> > >> I mean, with your proposed modifications, will the
> > >> wifi-wired-bridging example still work? Maybe yes with the example
> > >> program as-is, but what if you add a different wifi network with a
> > >> different SSID on the same channel? will the AP start forwarding
> > >> some packets it shouldn't?
> > > 
> > > I haven't looked into this example. I can try it out.
> > 
> > This would be very much appreciated!

I tried the wifi-wired-briding example (two versions, the vanilla one and a modified one with a second network). Everything works as expected in both cases.

The reason is the following: in ApWifiMac, the function Receive currently contains a check on the BSSID. Hence, even if there is a second network on the same channel, the frames from this second network are dropped. Note that this does not appear to be the case with AdhocWifiMac.

I also discovered that there is no check on the SSID (that's a bug in my opinion).

Summary: the patch behaves as expected with an AdHocWifiMac. With a ApWifiMac, my opinion is that frame with a different SSID should be dropped and frames with a different BSSID should go through.

> > 
> > > 
> > > With promiscuous mode, you will not capture packets with a different
> > > SSID.
> > > 
> > 
> > (Sorry I meant BSSID, not SSID)
> 
> Ah, so for BSSID, yes you will capture packets for a different BSSID. Indeed,
> this behavior should be checked.
> 
> > There is basically no explicit bssid check performed for data packets in
> > the ns-3 wifi code, so my understanding is that your patch would break
> > the case of using wifi-wired-bridging where another network with a
> > different BSSID is added.

Turns out there is on the BSSID. But not on the SSID.

> I haven't look into the example yet, so I don't know yet what is the scenario.
> But as I said above, I can check. Now, I'm not sure this would "break" the
> example. This would just be a "collateral" damage of misusing a feature.
> 
> > Adding an explicit BSSID check separated from the receiver address check
> > would probably make your patch work with wifi-wired-bridging. Still, if
> > there is an explicit BSSID check, then enabling promisc mode on an AP
> > has no effect, because in packets with ToDs=1 and FromDs=0 the BSSID is
> > in the Address1 field, and corresponds to the RA (IEEE Std. 802.11-2007,
> > Table 7.7). Hence only packets addressed to the MAC address of the
> > device will be forwarded to the upper layer (in spite of promisc mode
> > being active).

And that's exactly what happens. I'm not sure what to do there. Looks like the hardware I have in the lab (Atheros with madwifi) does not follow the above.
 
> Ok, will have a look to.
> 
> 
> > > 
> > > IMHO, monitor mode is not promiscuous mode. Frames received with
> > > monitor mode have a radiotap header (or some other header) attached.
> > > My solution just make NetDevice::SetPromiscRxCallback functional for
> > > Wifi devices.
> > 
> > I agree, but the two modes (monitor & promisc) are often mistaken. In
> > fact, there are several methods related to monitor mode that have
> > "promisc" in the name. I suggest to change them to "monitor" to avoid
> > ambiguity.
> 
> A quick grep in the code points to wifi-phy.cc and in particular
> m_phyPromiscSniffRxTrace and m_phyPromiscSniffTxTrace. I can investigate and
> produce a patch.

Any opinion on that?

Hi Ruben,

sorry for the very late reply... let's try to move this forward!

As for the wifi-wired-briging case, I agree with your interpretation. Also, thinking more about it, I think it is reasonable for us to assume that people enabling promisc mode know what they are doing. So, unless we get crazy and make promisc mode the default, it shouldn't be a problem.

To finalize the patch, I would kindly ask you to:
1) add doxygen for both MacLow::SetPromisc and RegularWifiMac::SetPromisc; the latter doxygen should also state that further filtering (such as the BSSID filtering done by ApWifiMac) might affect the actual behavior of promisc mode
2) rename the existing "PromiscSniff" to "MonitorMode", as agreed
3) change the "if (m_promisc)" piece, as agreed


> I also discovered that there is no check on the SSID (that's a bug in my
opinion).

I am not sure what you mean: the SSID is an IE that is included in management frames only, isn't it?

Created attachment 1140 [details]
Update promisc mode patch for wifi

Created attachment 1141 [details]
Rename promisc* to monitor* in WifiPhy

(In reply to comment #9)
> Hi Ruben,
> 
> sorry for the very late reply... let's try to move this forward!
> 
> As for the wifi-wired-briging case, I agree with your interpretation. Also,
> thinking more about it, I think it is reasonable for us to assume that people
> enabling promisc mode know what they are doing. So, unless we get crazy and
> make promisc mode the default, it shouldn't be a problem.
> 
> To finalize the patch, I would kindly ask you to:
> 1) add doxygen for both MacLow::SetPromisc and RegularWifiMac::SetPromisc; the
> latter doxygen should also state that further filtering (such as the BSSID
> filtering done by ApWifiMac) might affect the actual behavior of promisc mode
> 2) rename the existing "PromiscSniff" to "MonitorMode", as agreed
> 3) change the "if (m_promisc)" piece, as agreed
> 
> 
> > I also discovered that there is no check on the SSID (that's a bug in my
> opinion).
> 
> I am not sure what you mean: the SSID is an IE that is included in management
> frames only, isn't it?

Done, finally. Nicola, if you are fine with them, I can apply them when the freeze is over.

(In reply to comment #12)
> (In reply to comment #9)
> > Hi Ruben,
> > 
> > sorry for the very late reply... let's try to move this forward!
> > 
> > As for the wifi-wired-briging case, I agree with your interpretation. Also,
> > thinking more about it, I think it is reasonable for us to assume that people
> > enabling promisc mode know what they are doing. So, unless we get crazy and
> > make promisc mode the default, it shouldn't be a problem.
> > 
> > To finalize the patch, I would kindly ask you to:
> > 1) add doxygen for both MacLow::SetPromisc and RegularWifiMac::SetPromisc; the
> > latter doxygen should also state that further filtering (such as the BSSID
> > filtering done by ApWifiMac) might affect the actual behavior of promisc mode
> > 2) rename the existing "PromiscSniff" to "MonitorMode", as agreed
> > 3) change the "if (m_promisc)" piece, as agreed
> > 
> > 
> > > I also discovered that there is no check on the SSID (that's a bug in my
> > opinion).
> > 
> > I am not sure what you mean: the SSID is an IE that is included in management
> > frames only, isn't it?
> 
> Done, finally. Nicola, if you are fine with them, I can apply them when the
> freeze is over.

Nicola? Are you fine with it? If I don't hear from you until next Monday, I'll assume I can go ahead
Best,
Ruben

Sorry for the late reply. I've checked the patches and they are fine with me, so please commit.

(In reply to comment #14)
> Sorry for the late reply. I've checked the patches and they are fine with me,
> so please commit.

Thanks. Pushed.

Ruben, can you please just update RELEASE_NOTES and CHANGES.html before we mark this as closed?

(In reply to comment #16)
> Ruben, can you please just update RELEASE_NOTES and CHANGES.html before we mark
> this as closed?

done