2008-01-18 12:52:47

by Bruno Randolf

[permalink] [raw]
Subject: [PATCH] mac80211: enable IBSS merging

enable IBSS cell merging. if an IBSS beacon with the same ESSID and a TSF
higher than the local TSF (mactime) is received, we have to join its BSSID.

* move the relevant code section (previously only containing debug code) down
to the end of the function, so we can reuse the bss structure.

* we have to compare the mactime (TSF at the time of packet receive) rather
than the current TSF.

* in IBSS mode we want to allow beacons to override probe response info so we
can correctly do merges.

* we don't only configure beacons based on scan results, so change that
message.

* to enable all this we have to let all beacons thru in IBSS mode, even if they
have a different BSSID.

Signed-off-by: Bruno Randolf <[email protected]>
---

net/mac80211/ieee80211_sta.c | 75 ++++++++++++++++++++++++++----------------
net/mac80211/rx.c | 5 ++-
2 files changed, 51 insertions(+), 29 deletions(-)


diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index e7da1cd..f6b76a3 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_device *dev,
struct ieee80211_sta_bss *bss);
static int ieee80211_sta_find_ibss(struct net_device *dev,
struct ieee80211_if_sta *ifsta);
+static int ieee80211_sta_join_ibss(struct net_device *dev,
+ struct ieee80211_if_sta *ifsta,
+ struct ieee80211_sta_bss *bss);
static int ieee80211_sta_wep_configured(struct net_device *dev);
static int ieee80211_sta_start_scan(struct net_device *dev,
u8 *ssid, size_t ssid_len);
@@ -1825,7 +1828,7 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
struct ieee80211_sta_bss *bss;
struct sta_info *sta;
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- u64 timestamp;
+ u64 timestamp, mactime;
DECLARE_MAC_BUF(mac);
DECLARE_MAC_BUF(mac2);

@@ -1843,30 +1846,6 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
return;

timestamp = le64_to_cpu(mgmt->u.beacon.timestamp);
-
- if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
- memcmp(mgmt->bssid, sdata->u.sta.bssid, ETH_ALEN) == 0) {
-#ifdef CONFIG_MAC80211_IBSS_DEBUG
- static unsigned long last_tsf_debug = 0;
- u64 tsf;
- if (local->ops->get_tsf)
- tsf = local->ops->get_tsf(local_to_hw(local));
- else
- tsf = -1LLU;
- if (time_after(jiffies, last_tsf_debug + 5 * HZ)) {
- printk(KERN_DEBUG "RX beacon SA=%s BSSID="
- "%s TSF=0x%llx BCN=0x%llx diff=%lld "
- "@%lu\n",
- print_mac(mac, mgmt->sa), print_mac(mac2, mgmt->bssid),
- (unsigned long long)tsf,
- (unsigned long long)timestamp,
- (unsigned long long)(tsf - timestamp),
- jiffies);
- last_tsf_debug = jiffies;
- }
-#endif /* CONFIG_MAC80211_IBSS_DEBUG */
- }
-
ieee802_11_parse_elems(mgmt->u.beacon.variable, len - baselen, &elems);

if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && elems.supp_rates &&
@@ -1951,7 +1930,8 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
#endif
}

- if (bss->probe_resp && beacon) {
+ if (sdata->vif.type != IEEE80211_IF_TYPE_IBSS &&
+ bss->probe_resp && beacon) {
/* Do not allow beacon to override data from Probe Response. */
ieee80211_rx_bss_put(dev, bss);
return;
@@ -2070,6 +2050,45 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
bss->noise = rx_status->noise;
if (!beacon)
bss->probe_resp++;
+
+ /* check if we need to merge IBSS */
+ if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
+ !local->sta_sw_scanning && !local->sta_hw_scanning &&
+ mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
+ memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0) {
+#ifdef CONFIG_MAC80211_IBSS_DEBUG
+ static unsigned long last_tsf_debug;
+#endif
+ if (rx_status->flag & RX_FLAG_TSFT)
+ mactime = rx_status->mactime;
+ else {
+ mactime = -1LLU;
+ printk(KERN_WARNING "%s: IBSS mode needs mactime for "
+ "beacons\n", dev->name);
+ }
+#ifdef CONFIG_MAC80211_IBSS_DEBUG
+ if (time_after(jiffies, last_tsf_debug + 5 * HZ)) {
+ printk(KERN_DEBUG "RX beacon SA=%s BSSID="
+ "%s TSF=0x%llx BCN=0x%llx diff=%lld @%lu\n",
+ print_mac(mac, mgmt->sa),
+ print_mac(mac2, mgmt->bssid),
+ (unsigned long long)mactime,
+ (unsigned long long)timestamp,
+ (unsigned long long)(mactime - timestamp),
+ jiffies);
+ last_tsf_debug = jiffies;
+ }
+#endif /* CONFIG_MAC80211_IBSS_DEBUG */
+ if (mactime <= timestamp) {
+ printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
+ " -> IBSS merge with BSSID %s\n",
+ dev->name, print_mac(mac, mgmt->bssid));
+ ieee80211_sta_join_ibss(dev, &sdata->u.sta, bss);
+ ieee80211_ibss_add_sta(dev, NULL,
+ mgmt->bssid, mgmt->sa);
+ }
+ }
+
ieee80211_rx_bss_put(dev, bss);
}

@@ -2724,7 +2743,7 @@ static int ieee80211_sta_join_ibss(struct net_device *dev,
return -1;
}

- /* Set beacon template based on scan results */
+ /* Set beacon template */
skb = dev_alloc_skb(local->hw.extra_tx_headroom + 400);
do {
if (!skb)
@@ -2810,7 +2829,7 @@ static int ieee80211_sta_join_ibss(struct net_device *dev,
local->ops->beacon_update(local_to_hw(local),
skb, &control) == 0) {
printk(KERN_DEBUG "%s: Configured IBSS beacon "
- "template based on scan results\n", dev->name);
+ "template\n", dev->name);
skb = NULL;
}

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 6cc1e7e..5617e17 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1609,7 +1609,10 @@ static int prepare_for_handlers(struct ieee80211_sub_if_data *sdata,
case IEEE80211_IF_TYPE_IBSS:
if (!bssid)
return 0;
- if (!ieee80211_bssid_match(bssid, sdata->u.sta.bssid)) {
+ if ((rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_MGMT &&
+ (rx->fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_BEACON)
+ return 1;
+ else if (!ieee80211_bssid_match(bssid, sdata->u.sta.bssid)) {
if (!(rx->flags & IEEE80211_TXRXD_RXIN_SCAN))
return 0;
rx->flags &= ~IEEE80211_TXRXD_RXRA_MATCH;



2008-01-22 19:47:50

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Jan 21, 2008 11:05 AM, Ivo van Doorn <[email protected]> wrote:
> Hi,
>
> > > Then there is a problem for rt2x00. Since the mactime isn't known.
> > > rt2400pci is the _only_ device which has a RX_END_TIME field in the
> > > RX descriptor.
> >
> > one workaround could be to simply use the current TSF at the time in the
> > tasklet or interrupt handler (to be more close to the actual rx time). this
> > should be sufficient to catch most cases where an IBSS merge is necessary -
> > usually the beacon's TSF will be much higher than the local TSF.
>
> Should the driver to this, or should mac80211 handle that?

The driver should if it has access to some the mactime of the received
packet otherwise yes -- I think mac80211 can handle this using the
supplied get_tsf().

> Personally I think it is something for the mac80211 layer since the driver will
> give what it can, and can be sure that it is what mac80211 expects instead of
> drivers interpreting what mac80211 might want as replacement.
> If mac80211 needs the TSF value when no mac time is given, it could just
> use the get_tsf() callback function to the driver to get the substitute. When the
> get_tsf() callback is not provided, then mac80211 can complain about missing
> information.

Agreed, how about something like this modified to Bruno's patch:

+ if (rx_status->flag & RX_FLAG_TSFT)
+ mactime = rx_status->mactime;
+ else {
+ if (!local->ops->get_tsf) {
+ WARN_ON(1);
+ mactime = -1LLU;
+ if (net_ratelimit())
+ printk(KERN_WARNING "%s: cannot determine if "
+ "IBSS merge is required", dev->name);
+ }
+ else {
+ if (net_ratelimit())
+ printk("Could not get detailed timestamp
of beacon "
+ during reception, using the
driver's TSF timer for mactime\n")
+ mactime = local->ops->get_tsf(local_to_hw(local));
+ }
+ }

We could just not support IBSS for driver's without a get_tsf(). Thoughts?

Luis

2008-01-25 06:16:13

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Thursday 24 January 2008 23:30:02 Johannes Berg wrote:
> > > I think that needs a check "&& ssid_len" or something. Someone's bound
> > > to try making an IBSS with a hidden SSID and we really don't want that
> > > to work.
> >
> > why? i know, hidden SSIDs are pretty useless but if somedody wants
> > them... why would you want to make extra effort so it does not work?
>
> Uh, I think you should rethink your question. Consider two different
> hidden-SSID IBSSes.

that's the same case as two different IBSS with the same non-zero ESSID.

> > > > + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > > > + " -> IBSS merge with BSSID %s\n",
> > > > + dev->name, print_mac(mac, mgmt->bssid));
> > >
> > > No way. At the very least you need to ratelimit this.
> >
> > well, it should not happen very often, and for debugging i would like to
> > see this case. would
>
> Well, "should" is pretty soft. It can also be trivially triggered as a
> DOS attack by sending beacons with fake timestamps.

ok, i agree that a ratelimit is necessary.

> > if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
> > printk(KERN_DEBUG "%s: beacon TSF higher than "
> >
> > be ok with you?
>
> Maybe && would be better?

well, i would like to see every merge when debugging IBSS, hence the OR.
if you want i can remove that message when IBSS debugging is disabled.

bruno




2008-01-22 20:32:07

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Jan 22, 2008 2:54 PM, Ivo van Doorn <[email protected]> wrote:
>
> Hi,
>
> > > > > Then there is a problem for rt2x00. Since the mactime isn't known.
> > > > > rt2400pci is the _only_ device which has a RX_END_TIME field in the
> > > > > RX descriptor.
> > > >
> > > > one workaround could be to simply use the current TSF at the time in the
> > > > tasklet or interrupt handler (to be more close to the actual rx time). this
> > > > should be sufficient to catch most cases where an IBSS merge is necessary -
> > > > usually the beacon's TSF will be much higher than the local TSF.
> > >
> > > Should the driver to this, or should mac80211 handle that?
> >
> > The driver should if it has access to some the mactime of the received
> > packet otherwise yes -- I think mac80211 can handle this using the
> > supplied get_tsf().
> >
> > > Personally I think it is something for the mac80211 layer since the driver will
> > > give what it can, and can be sure that it is what mac80211 expects instead of
> > > drivers interpreting what mac80211 might want as replacement.
> > > If mac80211 needs the TSF value when no mac time is given, it could just
> > > use the get_tsf() callback function to the driver to get the substitute. When the
> > > get_tsf() callback is not provided, then mac80211 can complain about missing
> > > information.
> >
> > Agreed, how about something like this modified to Bruno's patch:
> >
> > + if (rx_status->flag & RX_FLAG_TSFT)
> > + mactime = rx_status->mactime;
> > + else {
> > + if (!local->ops->get_tsf) {
> > + WARN_ON(1);
> > + mactime = -1LLU;
> > + if (net_ratelimit())
> > + printk(KERN_WARNING "%s: cannot determine if "
> > + "IBSS merge is required", dev->name);
> > + }
> > + else {
> > + if (net_ratelimit())
> > + printk("Could not get detailed timestamp
> > of beacon "
> > + during reception, using the
> > driver's TSF timer for mactime\n")
> > + mactime = local->ops->get_tsf(local_to_hw(local));
> > + }
> > + }
> >
> > We could just not support IBSS for driver's without a get_tsf(). Thoughts?
>
> I am not sure if we should print error warning messages when the mactime is
> not provided. But other then that I agree with the above change, as it would
> allow rt2x00 to use the IBSS syncing. :)

Ah yes, ignore my printks and instead replace them with better comments :)

I still think we should inform the user if the user switches to IBSS
at some point, perhaps better during interface addition if their
driver's IBSS mode is going to have some issues. How about we add to
the enum ieee80211_hw_flags a "IEEE80211_HW_RX_MACTIME". Then we can
warn accordingly during ieee80211_if_add() if the interface type is
IBSS"

* If driver supports IEEE80211_HW_RX_MACTIME we don't warn anything
* If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is
implemented inform user IBSS merge may not behave accurately
* If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is not
implemented warn IBSS merge will not work

We could add:

static inline u64 __approx_mactime(struct ieee80211_local *local) {
BUG_ON(!local || !local->ops);
return (local->ops->get_tsf) ?
local->ops->get_tsf(local_to_hw(local)) : -1LLU;
}

Then in ieee80211_rx_bss_info() we can do something like:

+ if (local->hw.flags & IEEE80211_HW_RX_MACTIME) {
+ if (rx_status->flag & RX_FLAG_TSFT)
+ mactime = rx_status->mactime;
+ else {
+ WARN_ON(1);
+ mactime = __approx_mactime(local);
+ }
+ else {
+ mactime = __approx_mactime(local);
+ }

Luis

2008-01-24 14:58:16

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging



> > Do you care to explain what's so bad about function declarations?
>
> I don't know about Johannes, but I just see them as unnecessary
> burden. If I change some parameters in a function, I'll get a compiler
> error and only after that I realise to change the declaration as well.
>
> With public (ie. non-static) functions it's a different issue, because
> I know that there's a declaration in the header file. But with static
> functions this is not the case.

Seconded. I also see it as totally unnecessary if you structure the code
properly in the first place.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-01-22 20:51:30

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Hi,

<snip>

> > I am not sure if we should print error warning messages when the mactime is
> > not provided. But other then that I agree with the above change, as it would
> > allow rt2x00 to use the IBSS syncing. :)
>
> Ah yes, ignore my printks and instead replace them with better comments :)
>
> I still think we should inform the user if the user switches to IBSS
> at some point, perhaps better during interface addition if their
> driver's IBSS mode is going to have some issues. How about we add to
> the enum ieee80211_hw_flags a "IEEE80211_HW_RX_MACTIME". Then we can
> warn accordingly during ieee80211_if_add() if the interface type is
> IBSS"
>
> * If driver supports IEEE80211_HW_RX_MACTIME we don't warn anything
> * If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is
> implemented inform user IBSS merge may not behave accurately
> * If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is not
> implemented warn IBSS merge will not work
>
> We could add:
>
> static inline u64 __approx_mactime(struct ieee80211_local *local) {
> BUG_ON(!local || !local->ops);
> return (local->ops->get_tsf) ?
> local->ops->get_tsf(local_to_hw(local)) : -1LLU;
> }
>
> Then in ieee80211_rx_bss_info() we can do something like:
>
> + if (local->hw.flags & IEEE80211_HW_RX_MACTIME) {
> + if (rx_status->flag & RX_FLAG_TSFT)
> + mactime = rx_status->mactime;
> + else {
> + WARN_ON(1);
> + mactime = __approx_mactime(local);
> + }
> + else {
> + mactime = __approx_mactime(local);
> + }

Sounds good to me. :)

Ivo

2008-01-24 16:55:20

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> i can see now how you could interpret this as only applying to timers, but i
> think that does just not make sense - why would you want to sync timers and
> adapt beacon intervals if you don't share the same BSSID?

Hmm. Well I thought that was pretty much just for optimising performance
but now that I think about it again...

> i agree that the standard is quite ambigious when it comes to IBSS, but
> practically speaking, it is necessary to merge IBSS cells most of the time if
> you want to have IBSS working as expected. therefore i tend to interpret the
> ambigious parts of the standard in a way that will improve functionality.

I think "working as expected" is stretching it a bit ;) How do you
expect IBSS to work? I'd expect only the simplest use case from it:
start IBSS on one station and have another one in the vicinity find it
while scanning and join it. That works regardless of merging or not.

> please see my last post where i suggest to use:
>
> + if (rx_status->flag & RX_FLAG_TSFT)
> + /* in order for correct IBSS merging we need mactime*/
> + mactime = rx_status->mactime;
> + else if (local && local->ops && local->ops->get_tsf)
> + /* second best option: get current TSF */
> + mactime = local->ops->get_tsf(local_to_hw(local));
> + else
> + /* can't merge without knowing the TSF */
> + mactime = -1LLU;
>
> instead.

Not sure. That's a bit on a fine line, if you have a slow device
get_tsf() might be well off and this could trigger a BSSID change in
both stations.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-01-21 16:05:33

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Hi,

> > Then there is a problem for rt2x00. Since the mactime isn't known.
> > rt2400pci is the _only_ device which has a RX_END_TIME field in the
> > RX descriptor.
>
> one workaround could be to simply use the current TSF at the time in the
> tasklet or interrupt handler (to be more close to the actual rx time). this
> should be sufficient to catch most cases where an IBSS merge is necessary -
> usually the beacon's TSF will be much higher than the local TSF.

Should the driver to this, or should mac80211 handle that?
Personally I think it is something for the mac80211 layer since the driver will
give what it can, and can be sure that it is what mac80211 expects instead of
drivers interpreting what mac80211 might want as replacement.
If mac80211 needs the TSF value when no mac time is given, it could just
use the get_tsf() callback function to the driver to get the substitute. When the
get_tsf() callback is not provided, then mac80211 can complain about missing
information.

Ivo

2008-01-23 17:11:22

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> Then there is a problem for rt2x00. Since the mactime isn't known.
> rt2400pci is the _only_ device which has a RX_END_TIME field in the
> RX descriptor.

I don't think it's required for IBSS to function, only for this from my
POV dubious "merge" functionality.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-01-24 14:58:33

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> > I think that needs a check "&& ssid_len" or something. Someone's bound
> > to try making an IBSS with a hidden SSID and we really don't want that
> > to work.
>
> why? i know, hidden SSIDs are pretty useless but if somedody wants them... why
> would you want to make extra effort so it does not work?

Uh, I think you should rethink your question. Consider two different
hidden-SSID IBSSes.

> > > + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > > + " -> IBSS merge with BSSID %s\n",
> > > + dev->name, print_mac(mac, mgmt->bssid));
> >
> > No way. At the very least you need to ratelimit this.
>
> well, it should not happen very often, and for debugging i would like to see
> this case. would

Well, "should" is pretty soft. It can also be trivially triggered as a
DOS attack by sending beacons with fake timestamps.

>
> if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
> printk(KERN_DEBUG "%s: beacon TSF higher than "
>
> be ok with you?

Maybe && would be better?

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-01-24 08:52:32

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

bruno randolf <[email protected]> writes:

>> > --- a/net/mac80211/ieee80211_sta.c
>> > +++ b/net/mac80211/ieee80211_sta.c
>> > @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_device
>> > *dev, struct ieee80211_sta_bss *bss); static int
>> > ieee80211_sta_find_ibss(struct net_device *dev,
>> > struct ieee80211_if_sta *ifsta);
>> > +static int ieee80211_sta_join_ibss(struct net_device *dev,
>> > + struct ieee80211_if_sta *ifsta,
>> > + struct ieee80211_sta_bss *bss);
>>
>> No way, order the code properly, this mess needs to be cleaned up not
>> added to.
>
> Do you care to explain what's so bad about function declarations?

I don't know about Johannes, but I just see them as unnecessary
burden. If I change some parameters in a function, I'll get a compiler
error and only after that I realise to change the declaration as well.

With public (ie. non-static) functions it's a different issue, because
I know that there's a declaration in the header file. But with static
functions this is not the case.

--
Kalle Valo

2008-01-23 17:10:20

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> enable IBSS cell merging. if an IBSS beacon with the same ESSID and a TSF
> higher than the local TSF (mactime) is received, we have to join its BSSID.

Can you back that up with standard references? I see no such requirement
in the standard text. I can see that under some circumstances this may
be desirable, but maybe not.

In fact, this seems to *break* standard behaviour, as per 11.1.3.1:
(emphasis mine)

When a STA starts a BSS, that STA shall determine the BSSID of
the BSS. If the BSSType indicates an infrastructure BSS, then
the STA shall start an infrastructure BSS and the BSSID shall be
equal to the STA’s dot11StationID. ***The value of the BSSID
shall remain unchanged***, even if the value of dot11StationID
is changed after the completion of the MLME-START.request. If
the BSSType indicates an IBSS, the STA shall start an IBSS, and
the BSSID shall be an individual locally administered IEEE MAC
address as defined in 5.2 of IEEE Std 802-1990. The remaining 46
bits of that MAC address shall be a number selected in a manner
that minimizes the probability of STAs generating the same
number, even when those STAs are subjected to the same initial
conditions.

Remember that BSSIDs, *not* SSIDs are used to identify BSSes uniquely.

Of course you could argue that "merging" is simply tearing down the own
and then joining the other IBSS.

Comments on the code now.

> --- a/net/mac80211/ieee80211_sta.c
> +++ b/net/mac80211/ieee80211_sta.c
> @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_device *dev,
> struct ieee80211_sta_bss *bss);
> static int ieee80211_sta_find_ibss(struct net_device *dev,
> struct ieee80211_if_sta *ifsta);
> +static int ieee80211_sta_join_ibss(struct net_device *dev,
> + struct ieee80211_if_sta *ifsta,
> + struct ieee80211_sta_bss *bss);

No way, order the code properly, this mess needs to be cleaned up not
added to.

> - if (bss->probe_resp && beacon) {
> + if (sdata->vif.type != IEEE80211_IF_TYPE_IBSS &&
> + bss->probe_resp && beacon) {
> /* Do not allow beacon to override data from Probe Response. */

Ahem. Comment update required.

> + /* check if we need to merge IBSS */

Could use a plural, but whatever.

> + if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
> + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0) {

I think that needs a check "&& ssid_len" or something. Someone's bound
to try making an IBSS with a hidden SSID and we really don't want that
to work.

> +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> + static unsigned long last_tsf_debug;
> +#endif

Let's just get rid of that, it's not usable with multiple devices.

> + if (rx_status->flag & RX_FLAG_TSFT)
> + mactime = rx_status->mactime;
> + else {
> + mactime = -1LLU;
> + printk(KERN_WARNING "%s: IBSS mode needs mactime for "
> + "beacons\n", dev->name);

Very bad, you'll be flooded by this when others send beacons. Also, I
doubt its truthfulness.

> + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> + " -> IBSS merge with BSSID %s\n",
> + dev->name, print_mac(mac, mgmt->bssid));

No way. At the very least you need to ratelimit this.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-01-24 03:26:37

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wednesday 23 January 2008 23:48:49 Johannes Berg wrote:
> > enable IBSS cell merging. if an IBSS beacon with the same ESSID and=
a TSF
> > higher than the local TSF (mactime) is received, we have to join it=
s
> > BSSID.
>
> Can you back that up with standard references? I see no such requirem=
ent
> in the standard text. I can see that under some circumstances this ma=
y
> be desirable, but maybe not.

sure. the relevant sections of the standard are 11.1.2.3 "Beacon recept=
ion":

"STAs in an IBSS shall use other information in any received Beacon fra=
me for=20
which the IBSS subfield of the Capability field is set to 1 and the con=
tent=20
of the SSID element is equal to the SSID of the IBSS. Use of this infor=
mation=20
is specified in 11.1.4."

and in 11.1.4 "Adjusting STA timers":
(emphasis and brackets mine):

"In an IBSS, a STA shall always adopt the information in the contents o=
f a=20
Beacon or Probe Response frame when that frame contains a matching SSID=
and=20
the value of the time stamp is later than the STA=E2=80=99s TSF timer. =
[In response=20
to an MLME-Join.request, a STA shall initialize its TSF timer to 0 and =
shall=20
not transmit a beacon or probe response until it hears a beacon or prob=
e=20
response from a member of the IBSS with a matching SSID.]"

"All Beacon and Probe Response frames carry a Timestamp field. A STA re=
ceiving=20
such a frame from another STA in an IBSS with the same SSID shall compa=
re the=20
Timestamp field with its own TSF time. If the Timestamp field of the re=
ceived=20
frame is later than its own TSF time, the STA shall adopt *all* paramet=
ers=20
contained in the Beacon frame."

i can see now how you could interpret this as only applying to timers, =
but i=20
think that does just not make sense - why would you want to sync timers=
and=20
adapt beacon intervals if you don't share the same BSSID?

i agree that the standard is quite ambigious when it comes to IBSS, but=
=20
practically speaking, it is necessary to merge IBSS cells most of the t=
ime if=20
you want to have IBSS working as expected. therefore i tend to interpre=
t the=20
ambigious parts of the standard in a way that will improve functionalit=
y.

just think about two IBSS nodes which are configured with the same ESSI=
D and=20
which start up at nearly the same time. they would both scan, not find =
an=20
existing BSS and start their own. current mac80211 implementation would=
after=20
a while detect that they are alone on their BSS and start another scan,=
which=20
would finally cause one of them to reconfigure its BSSID, but this is n=
ot=20
sufficient: if you happen to have 2 nodes in each BSSID a merge would n=
ever=20
happen.
there are similar situations like this in real life, the most obvious e=
xample=20
beeing a city wide IBSS where you can easily have two separate groups o=
f=20
nodes starting up with a different BSSID. suddenly an obstacle is remov=
ed or=20
a new node comes up in the middle of these two groups (with connections=
to=20
both) - which BSSID is it supposed to join? the one with the higher TSF=
, but=20
that would leave us with 2 distinct BSSIDs. would all involved nodes re=
start,=20
they would all share the same BSSID now. i think the only sensible answ=
er to=20
this problem is that they all should join the BSSID with the higher TSF=
=2E=20

this is also how all drivers and (also firmware based) cards i know han=
dle=20
IBSS - they will merge in a similar fashion based on the ESSID. (i know=
=2E..=20
that might not be a good argument, since most drivers are broken in one=
way=20
or the other, especially when it comes to IBSS mode.)

> In fact, this seems to *break* standard behaviour, as per 11.1.3.1:
> (emphasis mine)
>
> When a STA starts a BSS, that STA shall determine the BSSID o=
f
> the BSS. If the BSSType indicates an infrastructure BSS, then
> the STA shall start an infrastructure BSS and the BSSID shall=
be
> equal to the STA=E2=80=99s dot11StationID. ***The value of th=
e BSSID
> shall remain unchanged***, even if the value of dot11StationI=
D
> is changed after the completion of the MLME-START.request.

i understand this to apply to infrastructure STAs only.

> If=20
> the BSSType indicates an IBSS, the STA shall start an IBSS, a=
nd
> the BSSID shall be an individual locally administered IEEE MA=
C
> address as defined in 5.2 of IEEE Std 802-1990. The remaining=
46
> bits of that MAC address shall be a number selected in a mann=
er
> that minimizes the probability of STAs generating the same
> number, even when those STAs are subjected to the same initia=
l
> conditions.
>
> Comments on the code now.
>
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_devic=
e
> > *dev, struct ieee80211_sta_bss *bss); static int
> > ieee80211_sta_find_ibss(struct net_device *dev,
> > struct ieee80211_if_sta *ifsta);
> > +static int ieee80211_sta_join_ibss(struct net_device *dev,
> > + struct ieee80211_if_sta *ifsta,
> > + struct ieee80211_sta_bss *bss);
>
> No way, order the code properly, this mess needs to be cleaned up not
> added to.

ok. i just didn't want to make my patch too unclear by moving stuff aro=
und in=20
addition to adding functionality. i'll make 2 separate patches, once we=
=20
agreed that this whole merge behaviour is wanted in mac80211 at all.

> > - if (bss->probe_resp && beacon) {
> > + if (sdata->vif.type !=3D IEEE80211_IF_TYPE_IBSS &&
> > + bss->probe_resp && beacon) {
> > /* Do not allow beacon to override data from Probe Response. */
>
> Ahem. Comment update required.

ok.

> > + if (sdata->vif.type =3D=3D IEEE80211_IF_TYPE_IBSS && beacon &&
> > + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> > + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) =
=3D=3D 0)
> > {
>
> I think that needs a check "&& ssid_len" or something. Someone's boun=
d
> to try making an IBSS with a hidden SSID and we really don't want tha=
t
> to work.

yep.

> > +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> > + static unsigned long last_tsf_debug;
> > +#endif
>
> Let's just get rid of that, it's not usable with multiple devices.

i'm happy to do that, i just wanted to preserve as much as possible of =
the=20
original code since i wasn't sure if anybody needed it.

> > + if (rx_status->flag & RX_FLAG_TSFT)
> > + mactime =3D rx_status->mactime;
> > + else {
> > + mactime =3D -1LLU;
> > + printk(KERN_WARNING "%s: IBSS mode needs mactime for "
> > + "beacons\n", dev->name);
>
> Very bad, you'll be flooded by this when others send beacons. Also, I
> doubt its truthfulness.

please see my last post where i suggest to use:

+ if (rx_status->flag & RX_FLAG_TSFT)
+ /* in order for correct IBSS merging we need ma=
ctime*/
+ mactime =3D rx_status->mactime;
+ else if (local && local->ops && local->ops->get_tsf)
+ /* second best option: get current TSF */
+ mactime =3D local->ops->get_tsf(local_to_hw(loc=
al));
+ else
+ /* can't merge without knowing the TSF */
+ mactime =3D -1LLU;

instead.

> > + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > + " -> IBSS merge with BSSID %s\n",
> > + dev->name, print_mac(mac, mgmt->bssid));
>
> No way. At the very least you need to ratelimit this.

ok, i'll add a ratelimit.

bruno

2008-01-22 19:54:36

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Hi,

> > > > Then there is a problem for rt2x00. Since the mactime isn't known.
> > > > rt2400pci is the _only_ device which has a RX_END_TIME field in the
> > > > RX descriptor.
> > >
> > > one workaround could be to simply use the current TSF at the time in the
> > > tasklet or interrupt handler (to be more close to the actual rx time). this
> > > should be sufficient to catch most cases where an IBSS merge is necessary -
> > > usually the beacon's TSF will be much higher than the local TSF.
> >
> > Should the driver to this, or should mac80211 handle that?
>
> The driver should if it has access to some the mactime of the received
> packet otherwise yes -- I think mac80211 can handle this using the
> supplied get_tsf().
>
> > Personally I think it is something for the mac80211 layer since the driver will
> > give what it can, and can be sure that it is what mac80211 expects instead of
> > drivers interpreting what mac80211 might want as replacement.
> > If mac80211 needs the TSF value when no mac time is given, it could just
> > use the get_tsf() callback function to the driver to get the substitute. When the
> > get_tsf() callback is not provided, then mac80211 can complain about missing
> > information.
>
> Agreed, how about something like this modified to Bruno's patch:
>
> + if (rx_status->flag & RX_FLAG_TSFT)
> + mactime = rx_status->mactime;
> + else {
> + if (!local->ops->get_tsf) {
> + WARN_ON(1);
> + mactime = -1LLU;
> + if (net_ratelimit())
> + printk(KERN_WARNING "%s: cannot determine if "
> + "IBSS merge is required", dev->name);
> + }
> + else {
> + if (net_ratelimit())
> + printk("Could not get detailed timestamp
> of beacon "
> + during reception, using the
> driver's TSF timer for mactime\n")
> + mactime = local->ops->get_tsf(local_to_hw(local));
> + }
> + }
>
> We could just not support IBSS for driver's without a get_tsf(). Thoughts?

I am not sure if we should print error warning messages when the mactime is
not provided. But other then that I agree with the above change, as it would
allow rt2x00 to use the IBSS syncing. :)

Ivo

2008-01-21 01:53:09

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sunday 20 January 2008 19:43:09 Ivo van Doorn wrote:
> Hi,
>
> > Adding Johill and Michael as they are maintainers of mac80211 and they
> > need to review this. Please add them on further mac80211 patches.
> >
> > Note to driver authors: zd1211rw, rt2x00 drivers and adm8211 will need
> > to set mactime if they want IBSS merge to work as this functionality
> > would be added with this. I believe all other drivers set it, but
> > didn't do a thorough grep.
>
> Then there is a problem for rt2x00. Since the mactime isn't known.
> rt2400pci is the _only_ device which has a RX_END_TIME field in the
> RX descriptor.

one workaround could be to simply use the current TSF at the time in the
tasklet or interrupt handler (to be more close to the actual rx time). this
should be sufficient to catch most cases where an IBSS merge is necessary -
usually the beacon's TSF will be much higher than the local TSF.

bruno

2008-01-25 08:01:58

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Friday 25 January 2008 01:55:04 Johannes Berg wrote:
> > i can see now how you could interpret this as only applying to timers,
> > but i think that does just not make sense - why would you want to sync
> > timers and adapt beacon intervals if you don't share the same BSSID?
>
> Hmm. Well I thought that was pretty much just for optimising performance
> but now that I think about it again...
>
> > i agree that the standard is quite ambigious when it comes to IBSS, but
> > practically speaking, it is necessary to merge IBSS cells most of the
> > time if you want to have IBSS working as expected. therefore i tend to
> > interpret the ambigious parts of the standard in a way that will improve
> > functionality.
>
> I think "working as expected" is stretching it a bit ;) How do you
> expect IBSS to work?

i think it's a reasonable expectation from a users point of view that two or
more IBSS nodes using the same ESSID on the same channel can communicate with
each other, regardless of when and where they were configured.

> I'd expect only the simplest use case from it:
> start IBSS on one station and have another one in the vicinity find it
> while scanning and join it. That works regardless of merging or not.

this is probably a valid, but a most conservative interpretation of the
standard, which makes IBSS practically unusable in many (or most) occasions.

i guess you have never actually used IBSS much. otherwise you would know from
experience that your simplest use case practically rarely happens. usually
you have situations where you miss the others BSS while scanning and end up
with 2 different BSSIDs. or what happens if there is a third station within
station 2's reach but out of range for station 1? sta2 will adopt the BSSID
of sta3, but sta1 will not and start a new BSSID. so not even your simplest
use case of two newly started stations will just work (without ibss merge).
examples and scenarios are endless, do you want me to continue? ;)

also i am not inventing this "non-standard merge". it is *not* non-standard,
it's just not directly obvious from reading the standard.

IBSS merges are done like this by various cards and drivers: prism54 does the
same (in firmware), hostap driver did it like this (probably does but i
haven't used it in quite a while), the broadcom driver in openwrt acts like
this, madwifi merges like this (ups, maybe i shouldn't have said that ;)) and
so on...

aren't there any other serious IBSS users out there who can support this case
and confirm that we need IBSS merge functionality?

> > please see my last post where i suggest to use:
> >
> > + if (rx_status->flag & RX_FLAG_TSFT)
> > + /* in order for correct IBSS merging we need
> > mactime*/ + mactime = rx_status->mactime;
> > + else if (local && local->ops && local->ops->get_tsf)
> > + /* second best option: get current TSF */
> > + mactime =
> > local->ops->get_tsf(local_to_hw(local)); + else
> > + /* can't merge without knowing the TSF */
> > + mactime = -1LLU;
> >
> > instead.
>
> Not sure. That's a bit on a fine line, if you have a slow device
> get_tsf() might be well off and this could trigger a BSSID change in
> both stations.

i don't see that problem: if get_tsf() is called very late on a slow device,
it would be higher than the beacons TSF, and nothing would happen.
also usually the beacons TSF is quite old, maybe from an IBSS running for
several hours or even days or weeks and therefore we would catch most
situations where IBSS merge is necessary even if get_tsf() is a bit off the
beacon reception time. but in order to be able to merge more correctly it
would be better if these devices could get the TSF as early as possible.

regards,
bruno

2008-01-23 17:28:04

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wed, 2008-01-23 at 15:48 +0100, Johannes Berg wrote:
> > enable IBSS cell merging. if an IBSS beacon with the same ESSID and=
a TSF
> > higher than the local TSF (mactime) is received, we have to join it=
s BSSID.
>=20
> Can you back that up with standard references? I see no such requirem=
ent
> in the standard text. I can see that under some circumstances this ma=
y
> be desirable, but maybe not.
>=20
> In fact, this seems to *break* standard behaviour, as per 11.1.3.1:
> (emphasis mine)

The way I understood Ad-Hoc to work was that if you weren't using the
SSID + BSSID as somebody else, you weren't part of that adhoc network.
Ad-Hoc + roaming isn't supported and is even less defined, which is wha=
t
this patch would enable IIUC. When joining an adhoc network, your
card/driver must scan around to find the SSID + (optionally) BSSID of
that adhoc network. If it finds one that matches criteria, it joins
that adhoc network, which means using the same BSSID. If it doesn't, i=
t
creates a new adhoc network with a generated BSSID and the specified
SSID.

If this patch were applied, then you'd pretty quickly get split adhoc
networks where some people were automatically moved to a different BSSI=
D
and the rest are left on the old BSSID (maybe missed a beacon, or
whatever), and therefore can't talk to each other. If only because
mac80211 would jump adhoc BSSIDs, while all other drivers (including
fullmac parts) would not.

A cell is identified uniquely by the combination of BSSID + SSID,
nothing less. You don't combine infrastructure networks of the same
SSID and a different BSSID, nor does this happen for adhoc.

Dan

> When a STA starts a BSS, that STA shall determine the BSSID o=
f
> the BSS. If the BSSType indicates an infrastructure BSS, then
> the STA shall start an infrastructure BSS and the BSSID shall=
be
> equal to the STA=E2=80=99s dot11StationID. ***The value of th=
e BSSID
> shall remain unchanged***, even if the value of dot11StationI=
D
> is changed after the completion of the MLME-START.request. If
> the BSSType indicates an IBSS, the STA shall start an IBSS, a=
nd
> the BSSID shall be an individual locally administered IEEE MA=
C
> address as defined in 5.2 of IEEE Std 802-1990. The remaining=
46
> bits of that MAC address shall be a number selected in a mann=
er
> that minimizes the probability of STAs generating the same
> number, even when those STAs are subjected to the same initia=
l
> conditions.
>=20
> Remember that BSSIDs, *not* SSIDs are used to identify BSSes uniquely=
=2E
>=20
> Of course you could argue that "merging" is simply tearing down the o=
wn
> and then joining the other IBSS.
>=20
> Comments on the code now.
>=20
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_devic=
e *dev,
> > struct ieee80211_sta_bss *bss);
> > static int ieee80211_sta_find_ibss(struct net_device *dev,
> > struct ieee80211_if_sta *ifsta);
> > +static int ieee80211_sta_join_ibss(struct net_device *dev,
> > + struct ieee80211_if_sta *ifsta,
> > + struct ieee80211_sta_bss *bss);
>=20
> No way, order the code properly, this mess needs to be cleaned up not
> added to.
> =20
> > - if (bss->probe_resp && beacon) {
> > + if (sdata->vif.type !=3D IEEE80211_IF_TYPE_IBSS &&
> > + bss->probe_resp && beacon) {
> > /* Do not allow beacon to override data from Probe Response. */
>=20
> Ahem. Comment update required.
>=20
> > + /* check if we need to merge IBSS */
>=20
> Could use a plural, but whatever.
>=20
> > + if (sdata->vif.type =3D=3D IEEE80211_IF_TYPE_IBSS && beacon &&
> > + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> > + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) =
=3D=3D 0) {
>=20
> I think that needs a check "&& ssid_len" or something. Someone's boun=
d
> to try making an IBSS with a hidden SSID and we really don't want tha=
t
> to work.
>=20
> > +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> > + static unsigned long last_tsf_debug;
> > +#endif
>=20
> Let's just get rid of that, it's not usable with multiple devices.
>=20
> > + if (rx_status->flag & RX_FLAG_TSFT)
> > + mactime =3D rx_status->mactime;
> > + else {
> > + mactime =3D -1LLU;
> > + printk(KERN_WARNING "%s: IBSS mode needs mactime for "
> > + "beacons\n", dev->name);
>=20
> Very bad, you'll be flooded by this when others send beacons. Also, I
> doubt its truthfulness.
>=20
> > + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > + " -> IBSS merge with BSSID %s\n",
> > + dev->name, print_mac(mac, mgmt->bssid));
>=20
> No way. At the very least you need to ratelimit this.
>=20
> johannes

2008-01-20 10:17:06

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Adding Johill and Michael as they are maintainers of mac80211 and they
need to review this. Please add them on further mac80211 patches.

Note to driver authors: zd1211rw, rt2x00 drivers and adm8211 will need
to set mactime if they want IBSS merge to work as this functionality
would be added with this. I believe all other drivers set it, but
didn't do a thorough grep.

On Jan 18, 2008 7:52 AM, Bruno Randolf <[email protected]> wrote:

> +
> + /* check if we need to merge IBSS */
> + if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
> + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0) {
> +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> + static unsigned long last_tsf_debug;
> +#endif
> + if (rx_status->flag & RX_FLAG_TSFT)
> + mactime = rx_status->mactime;
> + else {
> + mactime = -1LLU;
> + printk(KERN_WARNING "%s: IBSS mode needs mactime for "
> + "beacons\n", dev->name);

Does this merit a WARN_ON() ? This essentially will prevent IBSS
merges for drivers that haven't set mactime yet.

> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 6cc1e7e..5617e17 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1609,7 +1609,10 @@ static int prepare_for_handlers(struct ieee80211_sub_if_data *sdata,
> case IEEE80211_IF_TYPE_IBSS:
> if (!bssid)
> return 0;
> - if (!ieee80211_bssid_match(bssid, sdata->u.sta.bssid)) {
> + if ((rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_MGMT &&
> + (rx->fc & IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_BEACON)

Since we are letting all beacons through how about adding a check for
ssid on ieee80211_rx_bss_info()?

(memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0)

Just not sure exactly we'd check for it yet.

> + return 1;
> + else if (!ieee80211_bssid_match(bssid, sdata->u.sta.bssid)) {
> if (!(rx->flags & IEEE80211_TXRXD_RXIN_SCAN))
> return 0;
> rx->flags &= ~IEEE80211_TXRXD_RXRA_MATCH;
>
>

Luis

2008-01-24 05:43:49

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

hello johannes!

i appreciate your feedback, but on a second thought, let me question this:

On Wednesday 23 January 2008 23:48:49 Johannes Berg wrote:
> Comments on the code now.
>
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_device
> > *dev, struct ieee80211_sta_bss *bss); static int
> > ieee80211_sta_find_ibss(struct net_device *dev,
> > struct ieee80211_if_sta *ifsta);
> > +static int ieee80211_sta_join_ibss(struct net_device *dev,
> > + struct ieee80211_if_sta *ifsta,
> > + struct ieee80211_sta_bss *bss);
>
> No way, order the code properly, this mess needs to be cleaned up not
> added to.

do you care to explain what's so bad about function declarations?

> > + if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
> > + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> > + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0)
> > {
>
> I think that needs a check "&& ssid_len" or something. Someone's bound
> to try making an IBSS with a hidden SSID and we really don't want that
> to work.

why? i know, hidden SSIDs are pretty useless but if somedody wants them... why
would you want to make extra effort so it does not work?

> > + printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > + " -> IBSS merge with BSSID %s\n",
> > + dev->name, print_mac(mac, mgmt->bssid));
>
> No way. At the very least you need to ratelimit this.

well, it should not happen very often, and for debugging i would like to see
this case. would

if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
printk(KERN_DEBUG "%s: beacon TSF higher than "

be ok with you?

regards,
bruno

2008-01-24 03:49:12

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Thursday 24 January 2008 02:22:26 Dan Williams wrote:
> On Wed, 2008-01-23 at 15:48 +0100, Johannes Berg wrote:
> > > enable IBSS cell merging. if an IBSS beacon with the same ESSID and a
> > > TSF higher than the local TSF (mactime) is received, we have to join
> > > its BSSID.
> >
> > Can you back that up with standard references? I see no such requirement
> > in the standard text. I can see that under some circumstances this may
> > be desirable, but maybe not.
> >
> > In fact, this seems to *break* standard behaviour, as per 11.1.3.1:
> > (emphasis mine)
>
> The way I understood Ad-Hoc to work was that if you weren't using the
> SSID + BSSID as somebody else, you weren't part of that adhoc network.
> Ad-Hoc + roaming isn't supported and is even less defined, which is what
> this patch would enable IIUC.

i agree that you could call what my patch enables is roaming. but i think in
IBSS mode this "roaming" has to happen in order to get IBSS mode working as
the user would expect. if you configure 2 laptops in distance, say two
different rooms and then move one of them into the other room you would
expect to be able to communicate with the other laptop wouldn't you?

> When joining an adhoc network, your
> card/driver must scan around to find the SSID + (optionally) BSSID of
> that adhoc network. If it finds one that matches criteria, it joins
> that adhoc network, which means using the same BSSID. If it doesn't, it
> creates a new adhoc network with a generated BSSID and the specified
> SSID.
>
> If this patch were applied, then you'd pretty quickly get split adhoc
> networks where some people were automatically moved to a different BSSID
> and the rest are left on the old BSSID (maybe missed a beacon, or
> whatever), and therefore can't talk to each other.

actually i would argue that *without* this patch you will get split adhoc
networks all the time (which is the reason i created the patch in the first
place). please see my previous mail for some additional scenarios where this
would happen. also please understand that merging in a city wide IBSS might
seem like a crazy, irrelevant idea to you, but that is how IBSS mode is used
in practice (too) and secondly it's just an example of similar odd cases that
can also happen when you start an IBSS in a conference room with just a few
participants.

> If only because
> mac80211 would jump adhoc BSSIDs, while all other drivers (including
> fullmac parts) would not.

that's not true. most other drivers, including fullmac drivers will join
BSSIDs when the ESSID matches and the TSF is higher.

> A cell is identified uniquely by the combination of BSSID + SSID,
> nothing less. You don't combine infrastructure networks of the same
> SSID and a different BSSID, nor does this happen for adhoc.

sure you do that in infrastructure mode: any bigger infrastructure network
will consist of several APs connected by ethernet and using different BSSIDs,
with the same ESSID, allowing users to roam between them.

as far as adhoc is concerned: yes, you shouldn't do that, and this is exactly
what my patch is trying to achieve - only one BSSID for a given ESSID and
channel (the one with the oldest TSF value).

bruno

2008-01-23 02:00:15

by Bruno Randolf

[permalink] [raw]
Subject: Re: [ath5k-devel] [PATCH] mac80211: enable IBSS merging

On Wednesday 23 January 2008 05:32:05 Luis R. Rodriguez wrote:
> On Jan 22, 2008 2:54 PM, Ivo van Doorn <[email protected]> wrote:
> > Hi,
> >
> > > > > > Then there is a problem for rt2x00. Since the mactime isn't
> > > > > > known. rt2400pci is the _only_ device which has a RX_END_TIME
> > > > > > field in the RX descriptor.
> > > > >
> > > > > one workaround could be to simply use the current TSF at the time
> > > > > in the tasklet or interrupt handler (to be more close to the actual
> > > > > rx time). this should be sufficient to catch most cases where an
> > > > > IBSS merge is necessary - usually the beacon's TSF will be much
> > > > > higher than the local TSF.
> > > >
> > > > Should the driver to this, or should mac80211 handle that?
> > >
> > > The driver should if it has access to some the mactime of the received
> > > packet otherwise yes -- I think mac80211 can handle this using the
> > > supplied get_tsf().
> > >
> > > > Personally I think it is something for the mac80211 layer since the
> > > > driver will give what it can, and can be sure that it is what
> > > > mac80211 expects instead of drivers interpreting what mac80211 might
> > > > want as replacement. If mac80211 needs the TSF value when no mac time
> > > > is given, it could just use the get_tsf() callback function to the
> > > > driver to get the substitute. When the get_tsf() callback is not
> > > > provided, then mac80211 can complain about missing information.

well we can do that, but the closer you record the TSF of a packet after
reception, the better. this is why i suggested the interrupt handler or the
drivers rx tasklet. i don't know enough about when mac80211 rx handlers will
run, but it seems they could be delayed quite a bit (work queue?).
especially for IBSS merges we need to basically compare the TSF of the beaon
and the local TSF on a usec level. for most cases i guess getting the TSF in
ieee80211_rx_bss_info() should be sufficient, but it will not merge 100%
correctly if the time difference between IBSS nodes is small.

> I still think we should inform the user if the user switches to IBSS
> at some point, perhaps better during interface addition if their
> driver's IBSS mode is going to have some issues. How about we add to
> the enum ieee80211_hw_flags a "IEEE80211_HW_RX_MACTIME". Then we can
> warn accordingly during ieee80211_if_add() if the interface type is
> IBSS"
>
> * If driver supports IEEE80211_HW_RX_MACTIME we don't warn anything
> * If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is
> implemented inform user IBSS merge may not behave accurately
> * If IEEE80211_HW_RX_MACTIME is not supported and get_tsf() is not
> implemented warn IBSS merge will not work

jep, i think that would be best. can we add that in a separate patch?

> We could add:
>
> static inline u64 __approx_mactime(struct ieee80211_local *local) {
> BUG_ON(!local || !local->ops);
> return (local->ops->get_tsf) ?
> local->ops->get_tsf(local_to_hw(local)) : -1LLU;
> }
>
> Then in ieee80211_rx_bss_info() we can do something like:
>
> + if (local->hw.flags & IEEE80211_HW_RX_MACTIME) {
> + if (rx_status->flag & RX_FLAG_TSFT)
> + mactime = rx_status->mactime;
> + else {
> + WARN_ON(1);
> + mactime = __approx_mactime(local);
> + }
> + else {
> + mactime = __approx_mactime(local);
> + }

i'd prefer to do it without the inline just all in the function. like this:

+ if (rx_status->flag & RX_FLAG_TSFT)
+ /* in order for correct IBSS merging we need mactime*/
+ mactime = rx_status->mactime;
+ else if (local && local->ops && local->ops->get_tsf)
+ /* second best option: get current TSF */
+ mactime = local->ops->get_tsf(local_to_hw(local));
+ else
+ /* can't merge without knowing the TSF */
+ mactime = -1LLU;

comments? should i resend my patch?

bruno

2008-01-24 05:51:57

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wednesday 23 January 2008 23:49:51 Johannes Berg wrote:
> > Then there is a problem for rt2x00. Since the mactime isn't known.
> > rt2400pci is the _only_ device which has a RX_END_TIME field in the
> > RX descriptor.
>
> I don't think it's required for IBSS to function, only for this from my
> POV dubious "merge" functionality.

also you might want to check if your hard- or firmware (if you have any)
handles IBSS merges already, especially with USB devices. i can't imagine how
a device driver without access to that information is supposed to merge IBSS.

bruno

2008-01-21 01:57:30

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sunday 20 January 2008 19:17:04 Luis R. Rodriguez wrote:
> Adding Johill and Michael as they are maintainers of mac80211 and they
> need to review this. Please add them on further mac80211 patches.

oh, sorry. sure!

> On Jan 18, 2008 7:52 AM, Bruno Randolf <[email protected]> wrote:
> > +
> > + /* check if we need to merge IBSS */
> > + if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
> > + !local->sta_sw_scanning && !local->sta_hw_scanning &&
> > + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len)
> > == 0) { +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> > + static unsigned long last_tsf_debug;
> > +#endif
> > + if (rx_status->flag & RX_FLAG_TSFT)
> > + mactime = rx_status->mactime;
> > + else {
> > + mactime = -1LLU;
> > + printk(KERN_WARNING "%s: IBSS mode needs mactime
> > for " + "beacons\n", dev->name);
>
> Does this merit a WARN_ON() ? This essentially will prevent IBSS
> merges for drivers that haven't set mactime yet.
>
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 6cc1e7e..5617e17 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -1609,7 +1609,10 @@ static int prepare_for_handlers(struct
> > ieee80211_sub_if_data *sdata, case IEEE80211_IF_TYPE_IBSS:
> > if (!bssid)
> > return 0;
> > - if (!ieee80211_bssid_match(bssid, sdata->u.sta.bssid)) {
> > + if ((rx->fc & IEEE80211_FCTL_FTYPE) ==
> > IEEE80211_FTYPE_MGMT && + (rx->fc &
> > IEEE80211_FCTL_STYPE) == IEEE80211_STYPE_BEACON)
>
> Since we are letting all beacons through how about adding a check for
> ssid on ieee80211_rx_bss_info()?
>
> (memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0)
>
> Just not sure exactly we'd check for it yet.

sure, that's in the previous part of the patch:

> > + mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > + memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len)
> > == 0)

greetings,
bruno

2008-01-20 10:43:30

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Hi,

> Adding Johill and Michael as they are maintainers of mac80211 and they
> need to review this. Please add them on further mac80211 patches.
>
> Note to driver authors: zd1211rw, rt2x00 drivers and adm8211 will need
> to set mactime if they want IBSS merge to work as this functionality
> would be added with this. I believe all other drivers set it, but
> didn't do a thorough grep.

Then there is a problem for rt2x00. Since the mactime isn't known.
rt2400pci is the _only_ device which has a RX_END_TIME field in the
RX descriptor.

Ivo

2008-02-17 09:53:37

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


On Sat, 2008-02-16 at 11:29 +0900, Bruno Randolf wrote:
> enable IBSS cell merging. if an IBSS beacon with the same channel, same ESSID
> and a TSF higher than the local TSF (mactime) is received, we have to join its
> BSSID. while this might not be immediately apparent from reading the 802.11
> standard it is compliant and necessary to make IBSS mode functional in many
> cases. most drivers have a similar behaviour.

> [...]

Looks good, thanks for your patience. I have two minor comments still,
sorry for the lack of focus in earlier review.

> + } else if (local && local->ops && local->ops->get_tsf)
> + /* second best option: get current TSF */
> + rx_timestamp = local->ops->get_tsf(local_to_hw(local));

This is assuming that we don't manage to process the frame within 192
usecs. I guess that will be true since we defer it to a tasklet, but do
we want to bet on it in the future or should we simply widen the window
where the merge *won't* happen because the driver doesn't provide enough
info and also add the 24 bytes offset here?

Also something else that just occurred to me... You wrote in the
description:

> * to enable this we have to let all beacons thru in IBSS mode, even if
> they have a different BSSID.

which I think refers to the last hunk of your patch. However, hardware
also has such filters, so I think we also should set the
FIF_BCN_PRBRESP_PROMISC filter flag when IBSS is enabled, we don't want
to force drivers to do this implicitly when IBSS interfaces are added.

Again, thanks for your patience, and if you want this patch merged now
here's my

Acked-by: Johannes Berg <[email protected]>

and we can address these remaining two points in follow-up patches
instead of respinning this all the time.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-08 09:22:53

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Feb 6, 2008 10:58 PM, Jouni Malinen <[email protected]> wrote:
> On Wed, Feb 06, 2008 at 03:10:35PM -0500, John W. Linville wrote:
>
> > FWIW, that wouldn't be the first time we (i.e. Linux) chose to do
> > something that did not completely comply with a standard. If it
> > interoperates with other equipment and is good for users, I don't
> > think picky standard compliance is worthwhile.
>
> Sure, there are cases where this is reasonable.
>
> > Please note that I am not really taking a stand in favor of this ATM,
> > only asserting that blind standard compliance is not a good reason
> > to NAK it IMHO.
>
> In this case, standard compliance is certainly not the only concern I
> have. I'm worried about interoperability with non-mac80211
> implementations. If mac80211 were to hardcoded the BSSID for IBSS and
> refuse to change it no matter what (i.e., behave against the IEEE 802.11
> standard), IBSS would not interoperate with any other implementation if
> the other implementation happens to be the creator of the IBSS..

OK so you're saying that some people might actually expect that when
using the same SSID and same channel in close vicinity they'd get
separate BSSIDs generated? If so I don't see why people would expect
this functionality as a feature, I think this is more of a problem
than an expected result of how the standard is designed.

> Same issues shows up (but in somewhat less frequent form) in an IBSS
> splitting up and re-joining. In order to allow the STAs using other
> implementation (no BSSID hacks) to join the IBSS, mac80211 will need to
> be prepared to change the BSSID (or use some much more questionable
> hacks to make sure it is always the mac80211-STA that wins in the
> selection of which BSSID will survive.. ;-).

How would the IBSS be split up to generate a new BSSID? If you are
part of an IBSS and you don't see anyone generate a beacon between the
random backoff time you simply generate it, but the BSSID would still
be the same.

Luis

2008-02-05 01:50:28

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sunday 03 February 2008 08:22:12 Luis R. Rodriguez wrote:
> Sure ACK. Also as was stated, even with IBSS merge functionality
> working you could up with nodes splitting up due to missing the
> "leader" beacon due to interference or even strange radio node
> problems. A solution to this we implemented at Orbit with MadWifi was
> that instead of generating "random" BSSIDs you'd create one based on
> the hash of the SSID [1]. This ensures that nodes with identical SSIDs
> end up with identical BSSIDs, regardless of any strange problems.
> Technically from what I have gathered this doesn't break the specs but
> would prevent this split-IBSS problem. I actually like to see this
> technique added into mac80211 too.
>
> [1] http://www.orbit-lab.org/wiki/HowTo/bssidFix

this looks like a good idea to avoid many ibss merge problems, and it could be
really useful to have that (maybe as an option?) in mac80211. but unless we
have ibss merge functionality you won't be able to join older networks
(higher TSF) which don't use that scheme, so we still need that to be able to
communicate with other ("standard") drivers.

i guess the general question for mac80211 is: do we want and allow more
liberal interpretations of the standard concerning IBSS mode or do we
strictly stick with the "book"?

in my opinion these "extensions" are necessary in order to make IBSS mode
useful at all and therefore we need them! and as long as they are able to
interoperate with other drivers in IBSS mode i don't see a problem.

what's the position of the mac80211 maintainers concerning this? is there any
chance of getting these patches merged?

bruno

2008-02-08 09:48:22

by Joerg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Johannes Berg wrote:
> I think we should simply allow setting the BSSID (SIOCSIWAP maybe?) on
> IBSS interfaces and punt this issue to userspace.

This is exactly what we did/do with a Madwifi based IBSS network. I would
miss this feature when we switch to ath5k. So if you are taking votes on
this, you will get mine.

Regards
Joerg


Lesen Sie Ihre E-Mails jetzt einfach von unterwegs.
http://www.yahoo.de/go

2008-02-05 01:56:26

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Feb 4, 2008 8:50 PM, bruno randolf <[email protected]> wrote:
> On Sunday 03 February 2008 08:22:12 Luis R. Rodriguez wrote:
> > Sure ACK. Also as was stated, even with IBSS merge functionality
> > working you could up with nodes splitting up due to missing the
> > "leader" beacon due to interference or even strange radio node
> > problems. A solution to this we implemented at Orbit with MadWifi was
> > that instead of generating "random" BSSIDs you'd create one based on
> > the hash of the SSID [1]. This ensures that nodes with identical SSIDs
> > end up with identical BSSIDs, regardless of any strange problems.
> > Technically from what I have gathered this doesn't break the specs but
> > would prevent this split-IBSS problem. I actually like to see this
> > technique added into mac80211 too.
> >
> > [1] http://www.orbit-lab.org/wiki/HowTo/bssidFix
>
> this looks like a good idea to avoid many ibss merge problems, and it could be
> really useful to have that (maybe as an option?) in mac80211. but unless we
> have ibss merge functionality you won't be able to join older networks
> (higher TSF) which don't use that scheme, so we still need that to be able to
> communicate with other ("standard") drivers.
>
> i guess the general question for mac80211 is: do we want and allow more
> liberal interpretations of the standard concerning IBSS mode or do we
> strictly stick with the "book"?
>
> in my opinion these "extensions" are necessary in order to make IBSS mode
> useful at all and therefore we need them! and as long as they are able to
> interoperate with other drivers in IBSS mode i don't see a problem.

I agree 100%.

Luis

2008-02-18 11:16:07

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> > which I think refers to the last hunk of your patch. However, hardware
> > also has such filters, so I think we also should set the
> > FIF_BCN_PRBRESP_PROMISC filter flag when IBSS is enabled, we don't want
> > to force drivers to do this implicitly when IBSS interfaces are added.
>
> hmm, i don't know about that and i don't seem to need that for ath5k, so can
> we do that in a follow up patch, please?

Yeah sure, we can do that.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-14 10:19:17

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Tuesday 12 February 2008 18:52:03 Johannes Berg wrote:
> > true. i was not aware that the beacon timestamp is taking PHY delays into
> > account and wrongly asumed that the RX timestamp should always be a bit
> > later.
> > (which is also what i saw on my atheros hardware: we usually get the RX
> > timestamp about 60 to 300 usec later than the beacon timestamp, but it's
> > quite likely that we have not calibrated everything 100% correctly.)
>
> Later actually surprises me, are you sure it's not earlier? If
> everything was calibrated perfectly it should be 192 usecs earlier (24*8
> bytes at the most common 1mbit beacon rate).

yes, i'm sure that it's later. our debug printk shows diff= (rx_tsf -
beacon_tsf) and it's positive.

ulrich meis also confirmed that, but i think his message to the list got lost.

i'm not so surprised about that because the hardware would have to be
calibrated 100% correctly to compensate for the TX delay as well as the RX
delay (if both sides use ath5k) and it's really quite likely that we don't do
that right for ath5k yet.

please see also my previous mail, i think that mactime is not well defined as
well :(

bruno

2008-02-08 09:10:18

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Feb 6, 2008 10:52 PM, Jouni Malinen <[email protected]> wrote:
> On Wed, Feb 06, 2008 at 01:33:31PM -0500, Luis R. Rodriguez wrote:
>
> > Yes I see your point, this is definitely an issue, unless of course
> > the driver/stack is configured to default to use a specific channel by
> > default. But regardless its a problem.
>
> It doesn't really help even if the driver/stack would be configured to
> default to a specific channel. This has to work with other
> implementations, too.

Right, my point was just that if a stack was configured to try to use
a channel by default when creating an IBSS and you'd end up with the
same BSSID you'd eventually get a merge. But I see your point about
interoperability and I think its key.

> Hardcoding BSSID based on SSID may be fine for
> most cases when a new IBSS is created, but still, the implementation
> will need to allow this to be changed when merging with other parts of
> the IBSS since those other parts may not implement this type of hack.

I'm not quite sure I understand the problem here. The *key* feature of
using some sort of a hash is just to guarantee the "random" BSSID you
create matches that of the other stations using the same BSSID (and
channel if channel is part of the hash as well as you suggest). What
changes here is just the method for generation of the BSSID should no
beacon be received yet for the SSID chosen.

> I'm somewhat concerned about the possibility of getting the same BSSID
> on different channels, though. Using the same BSSID for different groups
> is not really a very good solution.. Including the channel number in the
> hash data would resolve this concern.

Sure.

> > Well the idea is that if it misses the timer from the leader it'll try
> > to create the IBSS but will end up using the same BSSID as the leader
> > would have, eventually causing a merge amongst the nodes. So you would
> > not merge unless its with the same BSSID.
>
> I'm not completely what the "misses the timer from the leader" is
> referring to or even what this "leader" would be in context of IBSS.
> Anyway, the implementation has to merge regardless of what BSSID is used
> (i.e., SSID match is enough).

Oh well I mean the TSF becaon leader when in IBSS in the BSS during
beacon generation. When in IBSS you have a node who has the greatest
time for the TSF, this is what I call the leader. If a STA misses the
"leader beacon" it'll generate its own beacon and may create a
separate BSS. If however, you change the algorithm for picking the
"random BSSID" and instead use some sort of hash based on desired SSID
(and channel) you'd end up generating beacons for the same BSSID,
eventually forcing a merge even if you missed the first few beacons
from the TSF leader for the BSS.

> > I haven't run the test myself but I am informed this hack helps to try
> > to solve the issue with 400 nodes on IBSS mode.
>
> Sure, if you can control all the STAs in the IBSS. That just is not the
> case in general and Linux wireless stack must not be designed with the
> view point of all STAs following the exact same rules (unless those
> rules are based on a required standard behavior)..

Agreed that we shouldn't assume everyone will have the same hacks, and
that we should focus on interoperability but I am wondering how this
could break things. All that would be changed here is the algorithm
for BSSID generation, instead of using a random BSSID you'd use one as
a hash of say SSID and channel. Can you think of issues with
interoperability with STAs who choose a random BSSID generator instead
of a hash other than those using a hash being able to guarantee
sticking to the same BSSID ?

> Taken into account that we should really implement proper IBSS merge to
> allow interoperation with non-mac80211 implementations, I don't know how
> much of a benefit this BSSID=hash(SSID,chan) mechanism would really
> provide.

It simply would solve the IBSS split issue, which can be caused by
several reasons, until that is solved within the standard or someone
thinks of a better solution.

> If it can be shown that the IBSS merges in a large network are
> not stable,

Oh we have proof for this :) and we also have proof of how using a
hash helps correct it, of course, as hack. I'll try to see if we can
come up with something a bit more formal I guess.

> this could of course be helpful and I don't think I would
> object it as long as the BSSID is going to end up being same only on the
> same channel and the mac80211 implementation allows the BSSID to be
> changed if a Beacon frame is received with a larger timestamp value than
> the local TSF timer.

Respecting the higher TSF is not affected, all that would be changed
would be the algorithm used for generating the BSSID. Once we have
IBSS merge on mac80211 I'll then test a patch for this on the 40x40
grid with ath5k and let you know how it goes. I'll try to use two
BSSes, each on a separate channel. Not sure yet what topology to use.

Luis

2008-02-12 03:25:38

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Thursday 07 February 2008 08:52:35 Johannes Berg wrote:
> Looks fine, one last question:
> > + if (mactime <=3D timestamp) {
> > + if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
> > + printk(KERN_DEBUG "%s: beacon TSF higher than "
> > + "local TSF - IBSS merge with BSSID %s\n",
> > + dev->name, print_mac(mac, mgmt->bssid));
>
> I'd rewrite that as timestamp >=3D mactime and rename the two variabl=
es
> (maybe beacon_timestamp and phy_timestamp or so?), but that's not too
> important.

i'll do that, sure.

> However, in any case "<=3D" seems wrong, shouldn't we only=20
> merge when it's actually higher? If your hardware/firmware has synced
> properly and the PHY delay is accounted for properly etc. then every
> beacon from the own BSS will fulfil the =3D=3D part of the condition,=
no?

true. i was not aware that the beacon timestamp is taking PHY delays in=
to=20
account and wrongly asumed that the RX timestamp should always be a bit=
=20
later.
(which is also what i saw on my atheros hardware: we usually get the RX=
=20
timestamp about 60 to 300 usec later than the beacon timestamp, but it'=
s=20
quite likely that we have not calibrated everything 100% correctly.)

> Also, the beacon timestamp is defined as follows:
>
> A STA sending a beacon shall set the value of the beacon=E2=80=
=99s
> timestamp so that it equals the value of the STA=E2=80=99s TS=
=46 timer at
> the time that the data symbol containing the first bit of the
> timestamp is transmitted to the PHY plus the transmitting STA=
=E2=80=99s
> delays through its local PHY from the MAC-PHY interface to it=
s
> interface with the WM [e.g., antenna, light-emitting diode (L=
ED)
> emission surface].
>
> (IEEE 802.11 11.1.2)
>
> Since we define the RX timestamp to be when the first data symbol of =
the
> frame hits the PHY, we here have to take into account the offset betw=
een
> the two, at 1 MBit that's 192 (=3D24 bytes * 8 usecs/byte) usecs earl=
ier
> than the beacon timestamp.
>
> On the other hand, you're unlikely to hit that window, but I suppose =
it
> warrants at least a comment.

thats a very good point! if everything was calibrated properly and all =
delays=20
taken into account *exactly* that would cause us to go thru a merge all=
the=20
time, on every received beacon. i think we have to take that into accou=
nt,=20
for different bitrates. what about something like:

timestamp >=3D ( mactime + (24 * 8 / beacon_phy_rate_in_mbps ))

i'll resend my patch adding that and the other proposed changes soon.

bruno

2008-02-14 06:20:11

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Tuesday 12 February 2008 18:50:19 Johannes Berg wrote:
> > thats a very good point! if everything was calibrated properly and =
all
> > delays taken into account *exactly* that would cause us to go thru =
a
> > merge all the time, on every received beacon. i think we have to ta=
ke
> > that into account, for different bitrates. what about something lik=
e:
> >
> > timestamp >=3D ( mactime + (24 * 8 / beacon_phy_rate_in_mbps ))
>
> Looks reasonable, though I'm not entirely sure how we defined 'mactim=
e'
> and whether it is at the first data or the first phy symbol. I think
> it's at the first data symbol which makes this correct.

hmm, it seems we don't have a strict definition of mactime... the kerne=
ldoc of=20
struct ieee80211_rx_status says:
* @mactime: MAC timestamp as defined by 802.11

but as far as i understand 802.11, it only defines "timestamp" for beac=
on and=20
probe response frames, where it actually means the timestamp field of t=
he=20
frame.

"Local Time" is defined as: "The value of the STA=E2=80=99s TSF timer a=
t the start of=20
reception of the first octet of the timestamp field of the received fra=
me=20
(probe response or beacon) from the found BSS." [802.11 p. 103]

also in "Process Validate_MPDU: it says:
"Save arrival time of first octet of {what may be a} timestamp field."
[802.11 p 391 and 460]

which could imply that this value is given as a rx timestamp (mactime).=
in=20
that case we could directly compare timestamp and mactime.

radiotap on the other hand defines it as the first bit of the data (MPD=
U):
* IEEE80211_RADIOTAP_TSFT u_int64_t microseconds
*
* Value in microseconds of the MAC's 64-bit 802.11 Time
* Synchronization Function timer when the first bit of the
* MPDU arrived at the MAC. For received frames, only.

so i guess right now it depends on the hardware what we actually get as=
=20
mactime...

i could not find any definition when the rx timestamp of atheros hardwa=
re is=20
taken.

do we have that information for other hardware? is mactime
*) the TSF at the reception of the first phy symbol?
*) the TSF at the reception of the first data (MPDU) symbol
*) the TSF at the reception of the byte 24 (where the timestamp field =
of
beacon and probe response frames is)?

bruno

2008-02-14 14:12:17

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> hmm, it seems we don't have a strict definition of mactime... the kerneldoc of
> struct ieee80211_rx_status says:
> * @mactime: MAC timestamp as defined by 802.11

Heh.

> radiotap on the other hand defines it as the first bit of the data (MPDU):
> * IEEE80211_RADIOTAP_TSFT u_int64_t microseconds
> *
> * Value in microseconds of the MAC's 64-bit 802.11 Time
> * Synchronization Function timer when the first bit of the
> * MPDU arrived at the MAC. For received frames, only.

Yeah I think that makes most sense and is how it is usually implemented
anyway.

> so i guess right now it depends on the hardware what we actually get as
> mactime...

> do we have that information for other hardware? is mactime
> *) the TSF at the reception of the first phy symbol?
> *) the TSF at the reception of the first data (MPDU) symbol
> *) the TSF at the reception of the byte 24 (where the timestamp field of
> beacon and probe response frames is)?

I very much doubt there is hardware that does the last option because
there may be frames that are shorted than 24 bytes :)

The difference between the first two options is a simple constant
(depending on the PHY mode) so the driver for hardware that supports
either one of those two options can easily translate them. Hence, I
think we should declare it like radiotap does.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-02 23:22:14

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Jan 25, 2008 3:01 AM, bruno randolf <[email protected]> wrote:
> On Friday 25 January 2008 01:55:04 Johannes Berg wrote:
> > > i can see now how you could interpret this as only applying to timers,
> > > but i think that does just not make sense - why would you want to sync
> > > timers and adapt beacon intervals if you don't share the same BSSID?
> >
> > Hmm. Well I thought that was pretty much just for optimising performance
> > but now that I think about it again...
> >
> > > i agree that the standard is quite ambigious when it comes to IBSS, but
> > > practically speaking, it is necessary to merge IBSS cells most of the
> > > time if you want to have IBSS working as expected. therefore i tend to
> > > interpret the ambigious parts of the standard in a way that will improve
> > > functionality.
> >
> > I think "working as expected" is stretching it a bit ;) How do you
> > expect IBSS to work?
>
> i think it's a reasonable expectation from a users point of view that two or
> more IBSS nodes using the same ESSID on the same channel can communicate with
> each other, regardless of when and where they were configured.
>
> > I'd expect only the simplest use case from it:
> > start IBSS on one station and have another one in the vicinity find it
> > while scanning and join it. That works regardless of merging or not.
>
> this is probably a valid, but a most conservative interpretation of the
> standard, which makes IBSS practically unusable in many (or most) occasions.
>
> i guess you have never actually used IBSS much. otherwise you would know from
> experience that your simplest use case practically rarely happens. usually
> you have situations where you miss the others BSS while scanning and end up
> with 2 different BSSIDs. or what happens if there is a third station within
> station 2's reach but out of range for station 1? sta2 will adopt the BSSID
> of sta3, but sta1 will not and start a new BSSID. so not even your simplest
> use case of two newly started stations will just work (without ibss merge).
> examples and scenarios are endless, do you want me to continue? ;)
>
> also i am not inventing this "non-standard merge". it is *not* non-standard,
> it's just not directly obvious from reading the standard.
>
> IBSS merges are done like this by various cards and drivers: prism54 does the
> same (in firmware), hostap driver did it like this (probably does but i
> haven't used it in quite a while), the broadcom driver in openwrt acts like
> this, madwifi merges like this (ups, maybe i shouldn't have said that ;)) and
> so on...
>
> aren't there any other serious IBSS users out there who can support this case
> and confirm that we need IBSS merge functionality?

Sure ACK. Also as was stated, even with IBSS merge functionality
working you could up with nodes splitting up due to missing the
"leader" beacon due to interference or even strange radio node
problems. A solution to this we implemented at Orbit with MadWifi was
that instead of generating "random" BSSIDs you'd create one based on
the hash of the SSID [1]. This ensures that nodes with identical SSIDs
end up with identical BSSIDs, regardless of any strange problems.
Technically from what I have gathered this doesn't break the specs but
would prevent this split-IBSS problem. I actually like to see this
technique added into mac80211 too.

[1] http://www.orbit-lab.org/wiki/HowTo/bssidFix

Luis

2008-02-12 09:53:53

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> thats a very good point! if everything was calibrated properly and all delays
> taken into account *exactly* that would cause us to go thru a merge all the
> time, on every received beacon. i think we have to take that into account,
> for different bitrates. what about something like:
>
> timestamp >= ( mactime + (24 * 8 / beacon_phy_rate_in_mbps ))

Looks reasonable, though I'm not entirely sure how we defined 'mactime'
and whether it is at the first data or the first phy symbol. I think
it's at the first data symbol which makes this correct.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-15 01:40:44

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Friday 15 February 2008 10:06:37 Luis R. Rodriguez wrote:
> > i think the point here is that we still need to be prepared to merge
> > IBSS (i.e. switch to another BSSID when we receive a beacon with an older
> > TSF) no matter how we generate our own BSSID in the first place.
>
> Well that is still possible.
>
> > using a hash of the BSSID and channel would reduce the necessity to
> > merge IBSS for mac80211 drivers but we still need to be able to join
> > older IBSS from other drivers.
>
> Sure, how is it that this prevents that?

it wouldn't :)

i agree to johannes approach to set the BSSID from userspace, so people can
use all sorts of hashing algorithms to set the bssid. we could implement a
default hashing algorithm in "iw" maybe, and introduce a flag "dont merge
ibss" to prevent ibss merge for situations where we know we don't want
that...

"iwconfig wlan0 ap xx:xx:xx:xx:xx:xx" doesn't work for ibss right now (i'll
try to look into that today).

bruno



2008-02-06 20:33:23

by John W. Linville

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wed, Feb 06, 2008 at 01:33:31PM -0500, Luis R. Rodriguez wrote:
> On Feb 5, 2008 11:34 PM, Jouni Malinen <[email protected]> wrote:

> > The BSSID generation as a hash from SSID does not sound like something
> > that would match the requirements of IEEE 802.11 11.1.6 ("a number
> > selected in a manner that minimizes the probability of STAs generating
> > the same number") since it is actually maximizing that probablity for
> > the case of same SSID ;-).
>
> Heh, good catch :)

FWIW, that wouldn't be the first time we (i.e. Linux) chose to do
something that did not completely comply with a standard. If it
interoperates with other equipment and is good for users, I don't
think picky standard compliance is worthwhile.

Please note that I am not really taking a stand in favor of this ATM,
only asserting that blind standard compliance is not a good reason
to NAK it IMHO.

John
--
John W. Linville
[email protected]

2008-02-07 03:52:17

by Jouni Malinen

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wed, Feb 06, 2008 at 01:33:31PM -0500, Luis R. Rodriguez wrote:

> Yes I see your point, this is definitely an issue, unless of course
> the driver/stack is configured to default to use a specific channel by
> default. But regardless its a problem.

It doesn't really help even if the driver/stack would be configured to
default to a specific channel. This has to work with other
implementations, too. Hardcoding BSSID based on SSID may be fine for
most cases when a new IBSS is created, but still, the implementation
will need to allow this to be changed when merging with other parts of
the IBSS since those other parts may not implement this type of hack.

I'm somewhat concerned about the possibility of getting the same BSSID
on different channels, though. Using the same BSSID for different groups
is not really a very good solution.. Including the channel number in the
hash data would resolve this concern.

> Well the idea is that if it misses the timer from the leader it'll try
> to create the IBSS but will end up using the same BSSID as the leader
> would have, eventually causing a merge amongst the nodes. So you would
> not merge unless its with the same BSSID.

I'm not completely what the "misses the timer from the leader" is
referring to or even what this "leader" would be in context of IBSS.
Anyway, the implementation has to merge regardless of what BSSID is used
(i.e., SSID match is enough).

> I haven't run the test myself but I am informed this hack helps to try
> to solve the issue with 400 nodes on IBSS mode.

Sure, if you can control all the STAs in the IBSS. That just is not the
case in general and Linux wireless stack must not be designed with the
view point of all STAs following the exact same rules (unless those
rules are based on a required standard behavior)..

Taken into account that we should really implement proper IBSS merge to
allow interoperation with non-mac80211 implementations, I don't know how
much of a benefit this BSSID=hash(SSID,chan) mechanism would really
provide. If it can be shown that the IBSS merges in a large network are
not stable, this could of course be helpful and I don't think I would
object it as long as the BSSID is going to end up being same only on the
same channel and the mac80211 implementation allows the BSSID to be
changed if a Beacon frame is received with a larger timestamp value than
the local TSF timer.

--
Jouni Malinen PGP id EFC895FA

2008-02-07 10:35:02

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

Looks fine, one last question:

> + if (mactime <= timestamp) {
> + if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
> + printk(KERN_DEBUG "%s: beacon TSF higher than "
> + "local TSF - IBSS merge with BSSID %s\n",
> + dev->name, print_mac(mac, mgmt->bssid));

I'd rewrite that as timestamp >= mactime and rename the two variables
(maybe beacon_timestamp and phy_timestamp or so?), but that's not too
important. However, in any case "<=" seems wrong, shouldn't we only
merge when it's actually higher? If your hardware/firmware has synced
properly and the PHY delay is accounted for properly etc. then every
beacon from the own BSS will fulfil the == part of the condition, no?

Also, the beacon timestamp is defined as follows:

A STA sending a beacon shall set the value of the beacon’s
timestamp so that it equals the value of the STA’s TSF timer at
the time that the data symbol containing the first bit of the
timestamp is transmitted to the PHY plus the transmitting STA’s
delays through its local PHY from the MAC-PHY interface to its
interface with the WM [e.g., antenna, light-emitting diode (LED)
emission surface].

(IEEE 802.11 11.1.2)

Since we define the RX timestamp to be when the first data symbol of the
frame hits the PHY, we here have to take into account the offset between
the two, at 1 MBit that's 192 (=24 bytes * 8 usecs/byte) usecs earlier
than the beacon timestamp.

On the other hand, you're unlikely to hit that window, but I suppose it
warrants at least a comment.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-07 03:58:27

by Jouni Malinen

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Wed, Feb 06, 2008 at 03:10:35PM -0500, John W. Linville wrote:

> FWIW, that wouldn't be the first time we (i.e. Linux) chose to do
> something that did not completely comply with a standard. If it
> interoperates with other equipment and is good for users, I don't
> think picky standard compliance is worthwhile.

Sure, there are cases where this is reasonable.

> Please note that I am not really taking a stand in favor of this ATM,
> only asserting that blind standard compliance is not a good reason
> to NAK it IMHO.

In this case, standard compliance is certainly not the only concern I
have. I'm worried about interoperability with non-mac80211
implementations. If mac80211 were to hardcoded the BSSID for IBSS and
refuse to change it no matter what (i.e., behave against the IEEE 802.11
standard), IBSS would not interoperate with any other implementation if
the other implementation happens to be the creator of the IBSS..

Same issues shows up (but in somewhat less frequent form) in an IBSS
splitting up and re-joining. In order to allow the STAs using other
implementation (no BSSID hacks) to join the IBSS, mac80211 will need to
be prepared to change the BSSID (or use some much more questionable
hacks to make sure it is always the mac80211-STA that wins in the
selection of which BSSID will survive.. ;-).

--
Jouni Malinen PGP id EFC895FA

2008-02-18 02:03:56

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sunday 17 February 2008 18:11:43 Johannes Berg wrote:
> Looks good, thanks for your patience. I have two minor comments still,
> sorry for the lack of focus in earlier review.
>
> > + } else if (local && local->ops && local->ops->get_tsf)
> > + /* second best option: get current TSF */
> > + rx_timestamp = local->ops->get_tsf(local_to_hw(local));
>
> This is assuming that we don't manage to process the frame within 192
> usecs. I guess that will be true since we defer it to a tasklet, but do
> we want to bet on it in the future or should we simply widen the window
> where the merge *won't* happen because the driver doesn't provide enough
> info and also add the 24 bytes offset here?

sorry, i was to fast in agreeing ;)
on a second thought i realized that we don't need to worry about that:

since we get to handle the frame after it was received *completely*, we can be
sure that the current time returned by get_tsf() is later than the time when
byte 24 was received. so no need for adding the offset.

i'll rebase the unmodified patch series and resend it.

bruno

2008-02-06 14:10:50

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> > A solution to this we implemented at Orbit with MadWifi was
> > that instead of generating "random" BSSIDs you'd create one based on
> > the hash of the SSID [1].

> this looks like a good idea to avoid many ibss merge problems, and it could be
> really useful to have that (maybe as an option?) in mac80211. but unless we
> have ibss merge functionality you won't be able to join older networks
> (higher TSF) which don't use that scheme, so we still need that to be able to
> communicate with other ("standard") drivers.

I think we should simply allow setting the BSSID (SIOCSIWAP maybe?) on
IBSS interfaces and punt this issue to userspace.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-18 01:43:11

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sunday 17 February 2008 18:11:43 Johannes Berg wrote:
> Looks good, thanks for your patience.

thanks for your review :)

> I have two minor comments still,
> sorry for the lack of focus in earlier review.
>
> > + } else if (local && local->ops && local->ops->get_tsf)
> > + /* second best option: get current TSF */
> > + rx_timestamp = local->ops->get_tsf(local_to_hw(local));
>
> This is assuming that we don't manage to process the frame within 192
> usecs. I guess that will be true since we defer it to a tasklet, but do
> we want to bet on it in the future or should we simply widen the window
> where the merge *won't* happen because the driver doesn't provide enough
> info and also add the 24 bytes offset here?

true. i'll change that and resend the patch. i have to rebase it anyways for
the current tree.

> Also something else that just occurred to me... You wrote in the
>
> description:
> > * to enable this we have to let all beacons thru in IBSS mode, even if
> > they have a different BSSID.
>
> which I think refers to the last hunk of your patch. However, hardware
> also has such filters, so I think we also should set the
> FIF_BCN_PRBRESP_PROMISC filter flag when IBSS is enabled, we don't want
> to force drivers to do this implicitly when IBSS interfaces are added.

hmm, i don't know about that and i don't seem to need that for ath5k, so can
we do that in a follow up patch, please?

bruno

2008-02-18 11:16:26

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> > This is assuming that we don't manage to process the frame within 192
> > usecs. I guess that will be true since we defer it to a tasklet, but do
> > we want to bet on it in the future or should we simply widen the window
> > where the merge *won't* happen because the driver doesn't provide enough
> > info and also add the 24 bytes offset here?
>
> sorry, i was to fast in agreeing ;)
> on a second thought i realized that we don't need to worry about that:
>
> since we get to handle the frame after it was received *completely*, we can be
> sure that the current time returned by get_tsf() is later than the time when
> byte 24 was received. so no need for adding the offset.

Hah, good point.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-06 04:35:07

by Jouni Malinen

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Sat, Feb 02, 2008 at 06:22:12PM -0500, Luis R. Rodriguez wrote:

> problems. A solution to this we implemented at Orbit with MadWifi was
> that instead of generating "random" BSSIDs you'd create one based on
> the hash of the SSID [1]. This ensures that nodes with identical SSIDs
> end up with identical BSSIDs, regardless of any strange problems.

How does that work with multiple channels? The patch did not seem to do
more than just use a very weak "hash" of the SSID to generate the BSSID.
In other words, I would assume it leaves IBSS STAs on whatever channel
they end up selecting, i.e., in separate groups, unless all STAs are
forced to use the same channel by not allowing them to initialize the
IBSS on other channels. This would also likely require hardcoding the
STAs not to allow merging to other BSSIDs should there be any such
(i.e., any STA that does not implement this hack).

> Technically from what I have gathered this doesn't break the specs but
> would prevent this split-IBSS problem. I actually like to see this
> technique added into mac80211 too.
>
> [1] http://www.orbit-lab.org/wiki/HowTo/bssidFix

I don't see how this alone would either prevent the split-IBSS problem
or be compliant with the IEEE 802.11 standard. The change in this patch
makes it likely to get same BSSID for different IBSS since the first six
octets of the SSID is used as the BSSID. That would not be very
desirable.. Furthermore, it is unlikely to work well with STA
implementations that do not implement the identical mechanism for
generating the BSSID.

The BSSID generation as a hash from SSID does not sound like something
that would match the requirements of IEEE 802.11 11.1.6 ("a number
selected in a manner that minimizes the probability of STAs generating
the same number") since it is actually maximizing that probablity for
the case of same SSID ;-).

As far as joining of separate STA groups in an IBSS (same SSID) is
concerned, 11.1.4 seems to indicate that the STA in IBSS shall adopt
_all_ parameters (i.e., also BSSID and channel) contained in the Beacon
frame when the Timestamp field of the received Beacon is later than the
STA's own TSF timer. This seems to be a requirement for supporting
merging of separate IBSS groups.. Actually hearing a Beacon from another
channel may be an issue, though, but at least as far as separate groups
using different BSSIDs, but same SSID, on the same channel is concerned,
the merging behavior seems to be defined in the standard.

--
Jouni Malinen PGP id EFC895FA

2008-02-06 18:33:34

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Feb 5, 2008 11:34 PM, Jouni Malinen <[email protected]> wrote:
> On Sat, Feb 02, 2008 at 06:22:12PM -0500, Luis R. Rodriguez wrote:
>
> > problems. A solution to this we implemented at Orbit with MadWifi was
> > that instead of generating "random" BSSIDs you'd create one based on
> > the hash of the SSID [1]. This ensures that nodes with identical SSIDs
> > end up with identical BSSIDs, regardless of any strange problems.
>
> How does that work with multiple channels?

Good point. It never really accounted for it. This patch to MadWifi
was just a hack to prevent IBSS split on a large grid, and should be
treated as such. I would have to check with MadWifi but my guess here
would be that we didn't see issue with channels as the channel
selected would be a default and all the cards were using the same
driver and hardware.

> The patch did not seem to do
> more than just use a very weak "hash" of the SSID to generate the BSSID.

Indeed the hash is extremely weak. If we were to use something similar
we'd have to consider a stronger hash, say maybe SHA1.

> In other words, I would assume it leaves IBSS STAs on whatever channel
> they end up selecting, i.e., in separate groups, unless all STAs are
> forced to use the same channel by not allowing them to initialize the
> IBSS on other channels.

Yes I see your point, this is definitely an issue, unless of course
the driver/stack is configured to default to use a specific channel by
default. But regardless its a problem.

> This would also likely require hardcoding the
> STAs not to allow merging to other BSSIDs should there be any such
> (i.e., any STA that does not implement this hack).

Well the idea is that if it misses the timer from the leader it'll try
to create the IBSS but will end up using the same BSSID as the leader
would have, eventually causing a merge amongst the nodes. So you would
not merge unless its with the same BSSID.

> > Technically from what I have gathered this doesn't break the specs but
> > would prevent this split-IBSS problem. I actually like to see this
> > technique added into mac80211 too.
> >
> > [1] http://www.orbit-lab.org/wiki/HowTo/bssidFix
>
> I don't see how this alone would either prevent the split-IBSS problem

I haven't run the test myself but I am informed this hack helps to try
to solve the issue with 400 nodes on IBSS mode.

> or be compliant with the IEEE 802.11 standard. The change in this patch
> makes it likely to get same BSSID for different IBSS since the first six
> octets of the SSID is used as the BSSID. That would not be very
> desirable..

Agreed, the hash used is very weak, again its a hack but perhaps it
can help find a better solution. AFAICT this is still an open problem.

> Furthermore, it is unlikely to work well with STA
> implementations that do not implement the identical mechanism for
> generating the BSSID.

Agreed, it definitely won't improve the situation in those cases, but
with those that do it would at least help.

> The BSSID generation as a hash from SSID does not sound like something
> that would match the requirements of IEEE 802.11 11.1.6 ("a number
> selected in a manner that minimizes the probability of STAs generating
> the same number") since it is actually maximizing that probablity for
> the case of same SSID ;-).

Heh, good catch :)

> As far as joining of separate STA groups in an IBSS (same SSID) is
> concerned, 11.1.4 seems to indicate that the STA in IBSS shall adopt
> _all_ parameters (i.e., also BSSID and channel) contained in the Beacon
> frame when the Timestamp field of the received Beacon is later than the
> STA's own TSF timer. This seems to be a requirement for supporting
> merging of separate IBSS groups..

ACK

> Actually hearing a Beacon from another
> channel may be an issue, though,

Broken? :)

> but at least as far as separate groups
> using different BSSIDs, but same SSID, on the same channel is concerned,
> the merging behavior seems to be defined in the standard.

ACK. Again, this seems like an open problem.

We *could* take the channel into considering for the hash along with
SSID... Anyway, good points!

Luis

2008-02-12 11:25:21

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging


> true. i was not aware that the beacon timestamp is taking PHY delays into
> account and wrongly asumed that the RX timestamp should always be a bit
> later.
> (which is also what i saw on my atheros hardware: we usually get the RX
> timestamp about 60 to 300 usec later than the beacon timestamp, but it's
> quite likely that we have not calibrated everything 100% correctly.)

Later actually surprises me, are you sure it's not earlier? If
everything was calibrated perfectly it should be 192 usecs earlier (24*8
bytes at the most common 1mbit beacon rate).

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-08 09:25:40

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Feb 6, 2008 6:52 PM, Johannes Berg <[email protected]> wrote:
> Looks fine, one last question:
>
> > + if (mactime <= timestamp) {
> > + if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
> > + printk(KERN_DEBUG "%s: beacon TSF higher than "
> > + "local TSF - IBSS merge with BSSID %s\n",
> > + dev->name, print_mac(mac, mgmt->bssid));
>
> I'd rewrite that as timestamp >= mactime and rename the two variables
> (maybe beacon_timestamp and phy_timestamp or so?), but that's not too
> important. However, in any case "<=" seems wrong, shouldn't we only
> merge when it's actually higher? If your hardware/firmware has synced
> properly and the PHY delay is accounted for properly etc. then every
> beacon from the own BSS will fulfil the == part of the condition, no?
>
> Also, the beacon timestamp is defined as follows:
>
> A STA sending a beacon shall set the value of the beacon's
> timestamp so that it equals the value of the STA's TSF timer at
> the time that the data symbol containing the first bit of the
> timestamp is transmitted to the PHY plus the transmitting STA's
> delays through its local PHY from the MAC-PHY interface to its
> interface with the WM [e.g., antenna, light-emitting diode (LED)
> emission surface].
>
> (IEEE 802.11 11.1.2)
>
> Since we define the RX timestamp to be when the first data symbol of the
> frame hits the PHY, we here have to take into account the offset between
> the two, at 1 MBit that's 192 (=24 bytes * 8 usecs/byte) usecs earlier
> than the beacon timestamp.
>
> On the other hand, you're unlikely to hit that window, but I suppose it
> warrants at least a comment.

Agreed, "mactime" is pretty vague, if we can add some of the above to
the documentation it would probably help other driver developers.

Luis

2008-02-12 02:00:11

by Bruno Randolf

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Friday 08 February 2008 18:22:52 Luis R. Rodriguez wrote:
> On Feb 6, 2008 10:58 PM, Jouni Malinen <[email protected]> wrote:
> > On Wed, Feb 06, 2008 at 03:10:35PM -0500, John W. Linville wrote:
> > > FWIW, that wouldn't be the first time we (i.e. Linux) chose to do
> > > something that did not completely comply with a standard. If it
> > > interoperates with other equipment and is good for users, I don't
> > > think picky standard compliance is worthwhile.
> >
> > Sure, there are cases where this is reasonable.
> >
> > > Please note that I am not really taking a stand in favor of this ATM,
> > > only asserting that blind standard compliance is not a good reason
> > > to NAK it IMHO.
> >
> > In this case, standard compliance is certainly not the only concern I
> > have. I'm worried about interoperability with non-mac80211
> > implementations. If mac80211 were to hardcoded the BSSID for IBSS and
> > refuse to change it no matter what (i.e., behave against the IEEE 802.11
> > standard), IBSS would not interoperate with any other implementation if
> > the other implementation happens to be the creator of the IBSS..
>
> OK so you're saying that some people might actually expect that when
> using the same SSID and same channel in close vicinity they'd get
> separate BSSIDs generated? If so I don't see why people would expect
> this functionality as a feature, I think this is more of a problem
> than an expected result of how the standard is designed.
>
> > Same issues shows up (but in somewhat less frequent form) in an IBSS
> > splitting up and re-joining. In order to allow the STAs using other
> > implementation (no BSSID hacks) to join the IBSS, mac80211 will need to
> > be prepared to change the BSSID (or use some much more questionable
> > hacks to make sure it is always the mac80211-STA that wins in the
> > selection of which BSSID will survive.. ;-).
>
> How would the IBSS be split up to generate a new BSSID? If you are
> part of an IBSS and you don't see anyone generate a beacon between the
> random backoff time you simply generate it, but the BSSID would still
> be the same.

i think the point here is that we still need to be prepared to merge IBSS
(i.e. switch to another BSSID when we receive a beacon with an older TSF) no
matter how we generate our own BSSID in the first place.

using a hash of the BSSID and channel would reduce the necessity to merge IBSS
for mac80211 drivers but we still need to be able to join older IBSS from
other drivers.

bruno



2008-02-15 01:06:39

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] mac80211: enable IBSS merging

On Mon, Feb 11, 2008 at 9:00 PM, bruno randolf <[email protected]> wrote:
>
> On Friday 08 February 2008 18:22:52 Luis R. Rodriguez wrote:
> > On Feb 6, 2008 10:58 PM, Jouni Malinen <[email protected]> wrote:
> > > On Wed, Feb 06, 2008 at 03:10:35PM -0500, John W. Linville wrote:
> > > > FWIW, that wouldn't be the first time we (i.e. Linux) chose to do
> > > > something that did not completely comply with a standard. If it
> > > > interoperates with other equipment and is good for users, I don't
> > > > think picky standard compliance is worthwhile.
> > >
> > > Sure, there are cases where this is reasonable.
> > >
> > > > Please note that I am not really taking a stand in favor of this ATM,
> > > > only asserting that blind standard compliance is not a good reason
> > > > to NAK it IMHO.
> > >
> > > In this case, standard compliance is certainly not the only concern I
> > > have. I'm worried about interoperability with non-mac80211
> > > implementations. If mac80211 were to hardcoded the BSSID for IBSS and
> > > refuse to change it no matter what (i.e., behave against the IEEE 802.11
> > > standard), IBSS would not interoperate with any other implementation if
> > > the other implementation happens to be the creator of the IBSS..
> >
> > OK so you're saying that some people might actually expect that when
> > using the same SSID and same channel in close vicinity they'd get
> > separate BSSIDs generated? If so I don't see why people would expect
> > this functionality as a feature, I think this is more of a problem
> > than an expected result of how the standard is designed.
> >
> > > Same issues shows up (but in somewhat less frequent form) in an IBSS
> > > splitting up and re-joining. In order to allow the STAs using other
> > > implementation (no BSSID hacks) to join the IBSS, mac80211 will need to
> > > be prepared to change the BSSID (or use some much more questionable
> > > hacks to make sure it is always the mac80211-STA that wins in the
> > > selection of which BSSID will survive.. ;-).
> >
> > How would the IBSS be split up to generate a new BSSID? If you are
> > part of an IBSS and you don't see anyone generate a beacon between the
> > random backoff time you simply generate it, but the BSSID would still
> > be the same.
>
> i think the point here is that we still need to be prepared to merge IBSS
> (i.e. switch to another BSSID when we receive a beacon with an older TSF) no
> matter how we generate our own BSSID in the first place.

Well that is still possible.

> using a hash of the BSSID and channel would reduce the necessity to merge IBSS
> for mac80211 drivers but we still need to be able to join older IBSS from
> other drivers.

Sure, how is it that this prevents that?

Luis