VLAN traffic appearing on the wrong iface

VLAN traffic appearing on the wrong iface

[Blaž Bačnik]

I use RADIUS-assigned vlans with my AP. Hostapd reports vlan change
during authentication and the station appears on correct vlan
according to "iw dev ... station dump". But actual packets keep coming
in on the default interface (wlan0), not the vlan one (eg. wlan0.2).
Moreover, I am able to ping this interface's (wlan0) IP address from
the station, even though "iw dev wlan0 station dump" does not list it.


I also tried taking RADIUS out of the loop, so I used hostapd's
"accept_mac_file" with specified vlan and I think I might have found
another bug. After applying the patch below (for I believe a rather
obvious typo), kernel started oopsing and I gave up. If needed, I can
provide config files for either hostapd or freeradius server, though
it looks like this is a driver problem.


Best regards,
Blaz



--- hostapd-0.6.9/hostapd/ieee802_11.c	2009-07-05 18:24:47.000000000 +0200
+++ hostapd-new/hostapd/ieee802_11.c	2009-09-30 02:16:56.000000000 +0200
@@ -583,11 +583,11 @@
 		goto fail;
 	}

 	if (vlan_id > 0) {
 		if (hostapd_get_vlan_id_ifname(hapd->conf->vlan,
-					       sta->vlan_id) == NULL) {
+					       vlan_id) == NULL) {
 			hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS,
 				       HOSTAPD_LEVEL_INFO, "Invalid VLAN ID "
 				       "%d received from RADIUS server",
 				       vlan_id);
 			resp = WLAN_STATUS_UNSPECIFIED_FAILURE;

Re: VLAN traffic appearing on the wrong iface

[Jouni Malinen]

On Wed, Sep 30, 2009 at 02:54:10AM +0200, Blaž Bačnik wrote:
> I use RADIUS-assigned vlans with my AP. Hostapd reports vlan change
> during authentication and the station appears on correct vlan
> according to "iw dev ... station dump". But actual packets keep coming
> in on the default interface (wlan0), not the vlan one (eg. wlan0.2).

You will need (at least) the following fix in hostapd to get this
working:
http://w1.fi/gitweb/gitweb.cgi?p=hostap.git;a=commitdiff;h=1c766b094a82a669a1d0bb7f8d132b322e56e81d

I have not tested whether this actually works, so no guarantees on that
being the only remaining issue.

> I also tried taking RADIUS out of the loop, so I used hostapd's
> "accept_mac_file" with specified vlan and I think I might have found
> another bug. After applying the patch below (for I believe a rather
> obvious typo), kernel started oopsing and I gave up. If needed, I can
> provide config files for either hostapd or freeradius server, though
> it looks like this is a driver problem.

Thanks for the hostapd patch (even better thanks would have been given
should it have been sent to the hostap mailing list or me ;-). It is now
in the current development (0.7.x) tree for hostapd (with the additional
fix I mentioned above). If you can test one, it would be interesting to
hear whether VLANs are actually working now.

As far as the kernel oops is concerned, that does not sound good..
hostapd should not have been able to trigger such a thing even with the
not-yey-fixed driver_nl80211.c.. Some more details on this could be
useful if I cannot easily reproduce the oops.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: VLAN traffic appearing on the wrong iface

[Johannes Berg]

On Wed, 2009-09-30 at 02:54 +0200, Blaž Bačnik wrote:
> I use RADIUS-assigned vlans with my AP. Hostapd reports vlan change
> during authentication and the station appears on correct vlan
> according to "iw dev ... station dump". But actual packets keep coming
> in on the default interface (wlan0), not the vlan one (eg. wlan0.2).

Actually, it looks like they come in on both :)

> I also tried taking RADIUS out of the loop, so I used hostapd's
> "accept_mac_file" with specified vlan and I think I might have found
> another bug. After applying the patch below (for I believe a rather
> obvious typo), kernel started oopsing and I gave up. If needed, I can
> provide config files for either hostapd or freeradius server, though
> it looks like this is a driver problem.

I can't reproduce that oops, but we have a fix to the issue which I'll
send separately.

johannes

[PATCH 2.6.32] mac80211: fix vlan and optimise RX

[Johannes Berg]

When receiving data frames, we can send them only to
the interface they belong to based on transmitting
station (this doesn't work for probe requests). Also,
don't try to handle other frames for AP_VLAN at all
since those interface should only receive data.

Additionally, the transmit side must check that the
station we're sending a frame to is actually on the
interface we're transmitting on, and not transmit
packets to functions that live on other interfaces,
so validate that as well.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: stable@kernel.org
---
 net/mac80211/rx.c |   10 ++++++++--
 net/mac80211/tx.c |    3 ++-
 2 files changed, 10 insertions(+), 3 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2009-09-30 21:38:59.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2009-09-30 22:09:54.000000000 +0200
@@ -2164,11 +2164,17 @@ static void __ieee80211_rx_handle_packet
 
 	skb = rx.skb;
 
-	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+	if (rx.sdata && ieee80211_is_data(hdr->frame_control)) {
+		rx.flags |= IEEE80211_RX_RA_MATCH;
+		prepares = prepare_for_handlers(rx.sdata, &rx, hdr);
+		if (prepares)
+			prev = rx.sdata;
+	} else list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		if (!netif_running(sdata->dev))
 			continue;
 
-		if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
+		if (sdata->vif.type == NL80211_IFTYPE_MONITOR ||
+		    sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
 			continue;
 
 		rx.flags |= IEEE80211_RX_RA_MATCH;
--- wireless-testing.orig/net/mac80211/tx.c	2009-09-30 21:49:34.000000000 +0200
+++ wireless-testing/net/mac80211/tx.c	2009-09-30 22:04:25.000000000 +0200
@@ -1704,7 +1704,8 @@ netdev_tx_t ieee80211_subif_start_xmit(s
 	if (!is_multicast_ether_addr(hdr.addr1)) {
 		rcu_read_lock();
 		sta = sta_info_get(local, hdr.addr1);
-		if (sta)
+		/* XXX: in the future, use sdata to look up the sta */
+		if (sta && sta->sdata == sdata)
 			sta_flags = get_sta_flags(sta);
 		rcu_read_unlock();
 	}

[PATCH v2 2.6.32] mac80211: fix vlan and optimise RX

[Johannes Berg]

When receiving data frames, we can send them only to
the interface they belong to based on transmitting
station (this doesn't work for probe requests). Also,
don't try to handle other frames for AP_VLAN at all
since those interface should only receive data.

Additionally, the transmit side must check that the
station we're sending a frame to is actually on the
interface we're transmitting on, and not transmit
packets to functions that live on other interfaces,
so validate that as well.

Another bug fix is needed in sta_info.c where in the
VLAN case when adding/removing stations we overwrite
the sdata variable we still need.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: stable@kernel.org
---
 net/mac80211/rx.c       |   10 ++++++++--
 net/mac80211/sta_info.c |    2 ++
 net/mac80211/tx.c       |    3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2009-09-30 21:38:59.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2009-09-30 22:09:54.000000000 +0200
@@ -2164,11 +2164,17 @@ static void __ieee80211_rx_handle_packet
 
 	skb = rx.skb;
 
-	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+	if (rx.sdata && ieee80211_is_data(hdr->frame_control)) {
+		rx.flags |= IEEE80211_RX_RA_MATCH;
+		prepares = prepare_for_handlers(rx.sdata, &rx, hdr);
+		if (prepares)
+			prev = rx.sdata;
+	} else list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		if (!netif_running(sdata->dev))
 			continue;
 
-		if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
+		if (sdata->vif.type == NL80211_IFTYPE_MONITOR ||
+		    sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
 			continue;
 
 		rx.flags |= IEEE80211_RX_RA_MATCH;
--- wireless-testing.orig/net/mac80211/tx.c	2009-09-30 21:49:34.000000000 +0200
+++ wireless-testing/net/mac80211/tx.c	2009-09-30 22:04:25.000000000 +0200
@@ -1704,7 +1704,8 @@ netdev_tx_t ieee80211_subif_start_xmit(s
 	if (!is_multicast_ether_addr(hdr.addr1)) {
 		rcu_read_lock();
 		sta = sta_info_get(local, hdr.addr1);
-		if (sta)
+		/* XXX: in the future, use sdata to look up the sta */
+		if (sta && sta->sdata == sdata)
 			sta_flags = get_sta_flags(sta);
 		rcu_read_unlock();
 	}
--- wireless-testing.orig/net/mac80211/sta_info.c	2009-10-01 21:59:00.000000000 +0200
+++ wireless-testing/net/mac80211/sta_info.c	2009-10-01 22:05:00.000000000 +0200
@@ -361,6 +361,7 @@ int sta_info_insert(struct sta_info *sta
 					     u.ap);
 
 		drv_sta_notify(local, &sdata->vif, STA_NOTIFY_ADD, &sta->sta);
+		sdata = sta->sdata;
 	}
 
 #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
@@ -496,6 +497,7 @@ static void __sta_info_unlink(struct sta
 
 		drv_sta_notify(local, &sdata->vif, STA_NOTIFY_REMOVE,
 			       &(*sta)->sta);
+		sdata = (*sta)->sdata;
 	}
 
 	if (ieee80211_vif_is_mesh(&sdata->vif)) {

Re: VLAN traffic appearing on the wrong iface

[Blaž Bačnik]

On Wed, Sep 30, 2009 at 10:16 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> I can't reproduce that oops, but we have a fix to the issue which I'll
> send separately.

Thanks for patches. However, I've investigated that oops and I don't really
understand it. Problem seems to be in __sta_info_unlink where code checks
if vif is a mesh. Since this is a vlan iface, sdata used actually belongs to ap
iface due to previous if but that really shouldn't be a problem since ap's
sdata should have vif->type as well, right?

While testing I've found out that sdata_of_ap->vif points to funny addresses
like 248 and such. So I've changed the code and that seems to have fixed
oops. I think this is now the right behaviour, anyway.

--- a/net/mac80211/sta_info.c	2009-10-01 16:27:31.000000000 +0200
+++ b/net/mac80211/sta_info.c	2009-10-01 16:24:34.000000000 +0200
@@ -498,7 +498,7 @@ static void __sta_info_unlink(struct sta
 			       &(*sta)->sta);
 	}

-	if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {
+	if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		mesh_accept_plinks_update(sdata);
 #ifdef CONFIG_MAC80211_MESH
 		del_timer(&(*sta)->plink_timer);

Re: VLAN traffic appearing on the wrong iface

[Blaž Bačnik]

Meh, I messed up the patch. Take two, hopefully better. Sorry.

Regards, Blaz


--- a/net/mac80211/sta_info.c	2009-10-01 16:24:34.000000000 +0200
+++ b/net/mac80211/sta_info.c	2009-10-01 16:27:31.000000000 +0200
@@ -498,7 +498,7 @@ static void __sta_info_unlink(struct sta
 			       &(*sta)->sta);
 	}

-	if (ieee80211_vif_is_mesh(&sdata->vif)) {
+	if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {
 		mesh_accept_plinks_update(sdata);
 #ifdef CONFIG_MAC80211_MESH
 		del_timer(&(*sta)->plink_timer);


On Thu, Oct 1, 2009 at 4:32 PM, Blaž Bačnik <bacnik@gmail.com> wrote:
> On Wed, Sep 30, 2009 at 10:16 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> I can't reproduce that oops, but we have a fix to the issue which I'll
>> send separately.
>
> Thanks for patches. However, I've investigated that oops and I don't really
> understand it. Problem seems to be in __sta_info_unlink where code checks
> if vif is a mesh. Since this is a vlan iface, sdata used actually belongs to ap
> iface due to previous if but that really shouldn't be a problem since ap's
> sdata should have vif->type as well, right?
>
> While testing I've found out that sdata_of_ap->vif points to funny addresses
> like 248 and such. So I've changed the code and that seems to have fixed
> oops. I think this is now the right behaviour, anyway.
>
> --- a/net/mac80211/sta_info.c   2009-10-01 16:27:31.000000000 +0200
> +++ b/net/mac80211/sta_info.c   2009-10-01 16:24:34.000000000 +0200
> @@ -498,7 +498,7 @@ static void __sta_info_unlink(struct sta
>                               &(*sta)->sta);
>        }
>
> -       if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {
> +       if (ieee80211_vif_is_mesh(&sdata->vif)) {
>                mesh_accept_plinks_update(sdata);
>  #ifdef CONFIG_MAC80211_MESH
>                del_timer(&(*sta)->plink_timer);
>

Re: VLAN traffic appearing on the wrong iface

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

Thanks for looking into the crash -- I can't explain it.

> -	if (ieee80211_vif_is_mesh(&sdata->vif)) {
> +	if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {

However, that doesn't make any sense, given

	struct ieee80211_sub_if_data *sdata = (*sta)->sdata;

Could the compiler be playing tricks on us?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: VLAN traffic appearing on the wrong iface

[Blaž Bačnik]

On Thu, Oct 1, 2009 at 8:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Thanks for looking into the crash -- I can't explain it.
>
>> -     if (ieee80211_vif_is_mesh(&sdata->vif)) {
>> +     if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {
>
> However, that doesn't make any sense, given
>
>        struct ieee80211_sub_if_data *sdata = (*sta)->sdata;
>
> Could the compiler be playing tricks on us?

There's the sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, u.ap)
in if-clause right in front of the ieee80211_vif_is_mesh and since
this is a vlan
interface that if-clause gets executed. This is done because we need to tell the
real, non-vlan interface what is going on. It changes sdata which is
understandable.
But I've really no idea why the new sdata (of non-vlan ap subiface)
does not have initialized vif member. Since this is the reason behind the oops:
trying to dereference a vif->type member of AP/VLAN's AP iface.

Basically, that if-clause changes sdata if iface type is AP/VLAN
(which it is), then tries
to do some stuff on the new sdata. And I think it should be doing it
on the old sdata --
this is what this patch does.

Regards, Blaz

Re: VLAN traffic appearing on the wrong iface

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Thu, 2009-10-01 at 21:55 +0200, Blaž Bačnik wrote:
> On Thu, Oct 1, 2009 at 8:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > Thanks for looking into the crash -- I can't explain it.
> >
> >> -     if (ieee80211_vif_is_mesh(&sdata->vif)) {
> >> +     if (ieee80211_vif_is_mesh(&(*sta)->sdata->vif)) {
> >
> > However, that doesn't make any sense, given
> >
> >        struct ieee80211_sub_if_data *sdata = (*sta)->sdata;
> >
> > Could the compiler be playing tricks on us?
> 
> There's the sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, u.ap)
> in if-clause right in front of the ieee80211_vif_is_mesh and since
> this is a vlan interface that if-clause gets executed.

Ahh. hwsim has no sta_notify callback. I'll amend my other patch
appropriately, since your fix is, while correct, kinda hard to
understand.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]