[PATCH 1/3] mac80211: agg-tx: call drv_wake_tx_queue in proper context

[PATCH 1/3] mac80211: agg-tx: call drv_wake_tx_queue in proper context

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Since drv_wake_tx_queue() is normally called in the TX path, which
is already in an RCU critical section, we should call it the same
way in the aggregation code path, so if the driver expects to be
able to use RCU, it'll already be protected without having to enter
a nested critical section.

Additionally, disable soft-IRQs, since not doing so could cause
issues in a driver that relies on them already being disabled like
in the other path.

Fixes: ba8c3d6f16a1 ("mac80211: add an intermediate software queue implementation")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/agg-tx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index cbd48762256c..4f9dd3e59091 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -226,7 +226,11 @@ ieee80211_agg_start_txq(struct sta_info *sta, int tid, bool enable)
 		clear_bit(IEEE80211_TXQ_AMPDU, &txqi->flags);
 
 	clear_bit(IEEE80211_TXQ_STOP, &txqi->flags);
+	local_bh_disable();
+	rcu_read_lock();
 	drv_wake_tx_queue(sta->sdata->local, txqi);
+	rcu_read_unlock();
+	local_bh_enable();
 }
 
 /*
-- 
2.11.0

[PATCH 2/3] mac80211: avoid allocating TXQs that won't be used

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

For AP_VLAN and monitor interfaces we'll never use the TXQs
we allocated, so avoid doing so.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/iface.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 9228ac73c429..fbf631b330b9 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1758,7 +1758,9 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 				 sizeof(void *));
 		int txq_size = 0;
 
-		if (local->ops->wake_tx_queue)
+		if (local->ops->wake_tx_queue &&
+		    sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
+		    sdata->vif.type != NL80211_IFTYPE_MONITOR)
 			txq_size += sizeof(struct txq_info) +
 				    local->hw.txq_data_size;
 
-- 
2.11.0

[PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

With TXQs, the AP_VLAN interfaces are resolved to their owner AP
interface when enqueuing the frame, which makes sense since the
frame really goes out on that as far as the driver is concerned.

However, this introduces a problem: frames to be encrypted with
a VLAN-specific GTK will now be encrypted with the AP GTK, since
the information about which virtual interface to use to select
the key is taken from the TXQ.

Fix this by preserving info->control.vif and using that in the
dequeue function. This now requires doing the driver-mapping
in the dequeue as well.

Since there's no way to filter the frames that are sitting on a
TXQ, drop all frames, which may affect other interfaces, when an
AP_VLAN is removed.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h | 15 ++-------------
 net/mac80211/iface.c   | 17 +++++++++++++++--
 net/mac80211/tx.c      | 39 +++++++++++++++++++++++++++++++--------
 3 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b2b5419467cc..263cb30d77c8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -919,21 +919,10 @@ struct ieee80211_tx_info {
 				unsigned long jiffies;
 			};
 			/* NB: vif can be NULL for injected frames */
-			union {
-				/* NB: vif can be NULL for injected frames */
-				struct ieee80211_vif *vif;
-
-				/* When packets are enqueued on txq it's easy
-				 * to re-construct the vif pointer. There's no
-				 * more space in tx_info so it can be used to
-				 * store the necessary enqueue time for packet
-				 * sojourn time computation.
-				 */
-				codel_time_t enqueue_time;
-			};
+			struct ieee80211_vif *vif;
 			struct ieee80211_key_conf *hw_key;
 			u32 flags;
-			/* 4 bytes free */
+			codel_time_t enqueue_time;
 		} control;
 		struct {
 			u64 cookie;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index fbf631b330b9..9e1bce27b5ac 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -792,6 +792,7 @@ static int ieee80211_open(struct net_device *dev)
 static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 			      bool going_down)
 {
+	struct ieee80211_sub_if_data *txq_sdata = sdata;
 	struct ieee80211_local *local = sdata->local;
 	struct fq *fq = &local->fq;
 	unsigned long flags;
@@ -937,6 +938,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP_VLAN:
+		txq_sdata = container_of(sdata->bss,
+					 struct ieee80211_sub_if_data, u.ap);
+
 		mutex_lock(&local->mtx);
 		list_del(&sdata->u.vlan.list);
 		mutex_unlock(&local->mtx);
@@ -1007,8 +1011,17 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	}
 	spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
 
-	if (sdata->vif.txq) {
-		struct txq_info *txqi = to_txq_info(sdata->vif.txq);
+	if (txq_sdata->vif.txq) {
+		struct txq_info *txqi = to_txq_info(txq_sdata->vif.txq);
+
+		/*
+		 * FIXME FIXME
+		 *
+		 * We really shouldn't purge the *entire* txqi since that
+		 * contains frames for the other AP_VLANs (and possibly
+		 * the AP itself) as well, but there's no API in FQ now
+		 * to be able to filter.
+		 */
 
 		spin_lock_bh(&fq->lock);
 		ieee80211_txq_purge(local, txqi);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8858f4f185e9..bd609326d77c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1276,11 +1276,6 @@ static void ieee80211_set_skb_enqueue_time(struct sk_buff *skb)
 	IEEE80211_SKB_CB(skb)->control.enqueue_time = codel_get_time();
 }
 
-static void ieee80211_set_skb_vif(struct sk_buff *skb, struct txq_info *txqi)
-{
-	IEEE80211_SKB_CB(skb)->control.vif = txqi->txq.vif;
-}
-
 static u32 codel_skb_len_func(const struct sk_buff *skb)
 {
 	return skb->len;
@@ -3414,6 +3409,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	struct ieee80211_tx_info *info;
 	struct ieee80211_tx_data tx;
 	ieee80211_tx_result r;
+	struct ieee80211_vif *vif;
 
 	spin_lock_bh(&fq->lock);
 
@@ -3430,8 +3426,6 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	if (!skb)
 		goto out;
 
-	ieee80211_set_skb_vif(skb, txqi);
-
 	hdr = (struct ieee80211_hdr *)skb->data;
 	info = IEEE80211_SKB_CB(skb);
 
@@ -3439,7 +3433,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	__skb_queue_head_init(&tx.skbs);
 	tx.local = local;
 	tx.skb = skb;
-	tx.sdata = vif_to_sdata(info->control.vif);
+	if (info->control.vif)
+		tx.sdata = vif_to_sdata(info->control.vif);
 
 	if (txq->sta)
 		tx.sta = container_of(txq->sta, struct sta_info, sta);
@@ -3488,6 +3483,34 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 		}
 	}
 
+	switch (tx.sdata->vif.type) {
+	case NL80211_IFTYPE_MONITOR:
+		if (tx.sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
+			vif = &tx.sdata->vif;
+			break;
+		}
+		tx.sdata = rcu_dereference(local->monitor_sdata);
+		if (tx.sdata) {
+			vif = &tx.sdata->vif;
+			info->hw_queue =
+				vif->hw_queue[skb_get_queue_mapping(skb)];
+		} else if (ieee80211_hw_check(&local->hw, QUEUE_CONTROL)) {
+			ieee80211_free_txskb(&local->hw, skb);
+			goto begin;
+		} else {
+			vif = NULL;
+		}
+		break;
+	case NL80211_IFTYPE_AP_VLAN:
+		tx.sdata = container_of(tx.sdata->bss,
+					struct ieee80211_sub_if_data, u.ap);
+		/* fall through */
+	default:
+		vif = &tx.sdata->vif;
+		break;
+	}
+
+	IEEE80211_SKB_CB(skb)->control.vif = vif;
 out:
 	spin_unlock_bh(&fq->lock);
 
-- 
2.11.0

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index b2b5419467cc..263cb30d77c8 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -919,21 +919,10 @@ struct ieee80211_tx_info {
>  				unsigned long jiffies;
>  			};
>  			/* NB: vif can be NULL for injected frames */
> -			union {
> -				/* NB: vif can be NULL for injected frames */
> -				struct ieee80211_vif *vif;
> -
> -				/* When packets are enqueued on txq it's easy
> -				 * to re-construct the vif pointer. There's no
> -				 * more space in tx_info so it can be used to
> -				 * store the necessary enqueue time for packet
> -				 * sojourn time computation.
> -				 */
> -				codel_time_t enqueue_time;
> -			};
> +			struct ieee80211_vif *vif;
>  			struct ieee80211_key_conf *hw_key;
>  			u32 flags;
> -			/* 4 bytes free */
> +			codel_time_t enqueue_time;

A side effect of this is that enqueue_time will be valid in the driver;
which is good as far as I'm concerned (I've been thinking about using it
to make decisions about when to stop retrying a frame).

If we want to save the four bytes, is there any reason we can't just
change the codel code to use skb->tstamp instead?

-Toke

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Johannes Berg]

On Mon, 2017-08-21 at 15:32 +0200, Toke Høiland-Jørgensen wrote:
> 
> > +			struct ieee80211_vif *vif;
> >  			struct ieee80211_key_conf *hw_key;
> >  			u32 flags;
> > -			/* 4 bytes free */
> > +			codel_time_t enqueue_time;
> 
> A side effect of this is that enqueue_time will be valid in the
> driver; which is good as far as I'm concerned (I've been thinking
> about using it to make decisions about when to stop retrying a
> frame).

Well, I think we need to do a lot more to allow that, but I guess
ultimately it would be possible - though we have this as codel_time_t
so the driver doesn't really know the reference etc. immediately.

> If we want to save the four bytes, is there any reason we can't just
> change the codel code to use skb->tstamp instead?

I didn't really want to go into that.

Any comments on the patch itself? I don't think I even merged this yet.

johannes

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2017-08-21 at 15:32 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>=20
>> > +			struct ieee80211_vif *vif;
>> > =C2=A0			struct ieee80211_key_conf *hw_key;
>> > =C2=A0			u32 flags;
>> > -			/* 4 bytes free */
>> > +			codel_time_t enqueue_time;
>>=20
>> A side effect of this is that enqueue_time will be valid in the
>> driver; which is good as far as I'm concerned (I've been thinking
>> about using it to make decisions about when to stop retrying a
>> frame).
>
> Well, I think we need to do a lot more to allow that, but I guess
> ultimately it would be possible - though we have this as codel_time_t
> so the driver doesn't really know the reference etc. immediately.

Well, the driver could be taught how to handle CoDel timestamps, but
you're right that that would be somewhat cludgy. Which is another reason
to use the tstamp; guess I'll ask Eric about that.

>> If we want to save the four bytes, is there any reason we can't just
>> change the codel code to use skb->tstamp instead?
>
> I didn't really want to go into that.
>
> Any comments on the patch itself? I don't think I even merged this
> yet.

Hmm, not apart from agreeing with you that it would be better to not
drop everything when removing a VLAN. Not sure how often this happens,
though (and hence how big of a problem it is). What happens in scenarios
where hostapd is setup to automatically generate a bunch of VLANs for
every client, for instance?

-Toke

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Johannes Berg]

On Mon, 2017-09-04 at 16:23 +0200, Toke Høiland-Jørgensen wrote:
> 
> Hmm, not apart from agreeing with you that it would be better to not
> drop everything when removing a VLAN. Not sure how often this
> happens, though (and hence how big of a problem it is). What happens
> in scenarios where hostapd is setup to automatically generate a bunch
> of VLANs for every client, for instance?

I'm not sure. However, I think it's less bad than one might guess since
it really should only affect multicast frames, right? All unicast
frames should go directly to the per-STA TXQ.

Right now, with TXQ, encrypted VLANs are completely broken, so surely
it's a step in the right direction?

johannes

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2017-09-04 at 16:23 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>=20
>> Hmm, not apart from agreeing with you that it would be better to not
>> drop everything when removing a VLAN. Not sure how often this
>> happens, though (and hence how big of a problem it is). What happens
>> in scenarios where hostapd is setup to automatically generate a bunch
>> of VLANs for every client, for instance?
>
> I'm not sure. However, I think it's less bad than one might guess
> since it really should only affect multicast frames, right? All
> unicast frames should go directly to the per-STA TXQ.

Ah, right, that is the interface txq that is being purged. Gotcha.

But, erm, what happens with unicast traffic sent on the VLAN interface
when it goes down? Shouldn't that be purged from the per-station TXQs as
well?

> Right now, with TXQ, encrypted VLANs are completely broken, so surely
> it's a step in the right direction?

Yeah, fair point ;)

One nit with the patch:

> -	tx.sdata =3D vif_to_sdata(info->control.vif);
> +	if (info->control.vif)
> +		tx.sdata =3D vif_to_sdata(info->control.vif);

Why the conditional assignment? The code below unconditionally
dereferences tx.sdata, so if info->control.vif is null it is going to
crash anyway?

-Toke

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Johannes Berg]

On Tue, 2017-09-05 at 11:02 +0200, Toke Høiland-Jørgensen wrote:
> 
> > I'm not sure. However, I think it's less bad than one might guess
> > since it really should only affect multicast frames, right? All
> > unicast frames should go directly to the per-STA TXQ.
> 
> Ah, right, that is the interface txq that is being purged. Gotcha.
> 
> But, erm, what happens with unicast traffic sent on the VLAN
> interface when it goes down? Shouldn't that be purged from the per-
> station TXQs as well?

Well, those entire TXQs are destroyed with the station :)

> One nit with the patch:
> 
> > -	tx.sdata = vif_to_sdata(info->control.vif);
> > +	if (info->control.vif)
> > +		tx.sdata = vif_to_sdata(info->control.vif);
> 
> Why the conditional assignment? The code below unconditionally
> dereferences tx.sdata, so if info->control.vif is null it is going to
> crash anyway?

Yeah, I agree that makes no sense, I'll remove that piece again.

johannes

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2017-09-05 at 11:02 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>=20
>> > I'm not sure. However, I think it's less bad than one might guess
>> > since it really should only affect multicast frames, right? All
>> > unicast frames should go directly to the per-STA TXQ.
>>=20
>> Ah, right, that is the interface txq that is being purged. Gotcha.
>>=20
>> But, erm, what happens with unicast traffic sent on the VLAN
>> interface when it goes down? Shouldn't that be purged from the per-
>> station TXQs as well?
>
> Well, those entire TXQs are destroyed with the station :)

Ah, so the station is attached to the VLAN interface, not the parent
interface?

I guess that the only case where there is likely to be any significant
effects of dropping the whole queue is if someone is sending large
amounts of multicast traffic (live video streaming? is that even
feasible over WiFi?) while reconfiguring their VLAN setup. That is
probably not a terribly common combination...

-Toke

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Johannes Berg]

On Tue, 2017-09-05 at 11:49 +0200, Toke Høiland-Jørgensen wrote:
> 
> Ah, so the station is attached to the VLAN interface, not the parent
> interface?

Doesn't actually matter, but if the VLAN goes where the station belongs
then either the station must've moved somewhere else or have been
destroyed, you can't have the station pointing to a VLAN that no longer
exists :)

> I guess that the only case where there is likely to be any
> significant effects of dropping the whole queue is if someone is
> sending large amounts of multicast traffic (live video streaming? is
> that even feasible over WiFi?) while reconfiguring their VLAN setup.
> That is probably not a terribly common combination...

Right.

Anyway I've applied this with the pointless null check removed.

johannes

Re: [PATCH 3/3] mac80211: fix VLAN handling with TXQs

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2017-09-05 at 11:49 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>=20
>> Ah, so the station is attached to the VLAN interface, not the parent
>> interface?
>
> Doesn't actually matter, but if the VLAN goes where the station belongs
> then either the station must've moved somewhere else or have been
> destroyed, you can't have the station pointing to a VLAN that no longer
> exists :)

Yeah, that makes sense. I was thinking of VLANs as something that was
carried "on top" of the station<->ap connection (so that the VLAN tag is
propagated to the client).

>> I guess that the only case where there is likely to be any
>> significant effects of dropping the whole queue is if someone is
>> sending large amounts of multicast traffic (live video streaming? is
>> that even feasible over WiFi?) while reconfiguring their VLAN setup.
>> That is probably not a terribly common combination...
>
> Right.
>
> Anyway I've applied this with the pointless null check removed.

Cool :)

-Toke