[PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls

[PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls

[Felix Fietkau]

The callers of these functions are all within RCU locked sections

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/mesh_hwmp.c    | 26 +++++++-------------------
 net/mac80211/mesh_pathtbl.c |  2 +-
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
index f7517668e77a..dcbca7c8c67d 100644
--- a/net/mac80211/mesh_hwmp.c
+++ b/net/mac80211/mesh_hwmp.c
@@ -1130,16 +1130,13 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata,
 	struct mesh_path *mpath;
 	struct sk_buff *skb_to_free = NULL;
 	u8 *target_addr = hdr->addr3;
-	int err = 0;
 
 	/* Nulls are only sent to peers for PS and should be pre-addressed */
 	if (ieee80211_is_qos_nullfunc(hdr->frame_control))
 		return 0;
 
-	rcu_read_lock();
-	err = mesh_nexthop_lookup(sdata, skb);
-	if (!err)
-		goto endlookup;
+	if (!mesh_nexthop_lookup(sdata, skb))
+		return 0;
 
 	/* no nexthop found, start resolving */
 	mpath = mesh_path_lookup(sdata, target_addr);
@@ -1147,8 +1144,7 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata,
 		mpath = mesh_path_add(sdata, target_addr);
 		if (IS_ERR(mpath)) {
 			mesh_path_discard_frame(sdata, skb);
-			err = PTR_ERR(mpath);
-			goto endlookup;
+			return PTR_ERR(mpath);
 		}
 	}
 
@@ -1161,13 +1157,10 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata,
 	info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
 	ieee80211_set_qos_hdr(sdata, skb);
 	skb_queue_tail(&mpath->frame_queue, skb);
-	err = -ENOENT;
 	if (skb_to_free)
 		mesh_path_discard_frame(sdata, skb_to_free);
 
-endlookup:
-	rcu_read_unlock();
-	return err;
+	return -ENOENT;
 }
 
 /**
@@ -1187,13 +1180,10 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data *sdata,
 	struct sta_info *next_hop;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	u8 *target_addr = hdr->addr3;
-	int err = -ENOENT;
 
-	rcu_read_lock();
 	mpath = mesh_path_lookup(sdata, target_addr);
-
 	if (!mpath || !(mpath->flags & MESH_PATH_ACTIVE))
-		goto endlookup;
+		return -ENOENT;
 
 	if (time_after(jiffies,
 		       mpath->exp_time -
@@ -1208,12 +1198,10 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data *sdata,
 		memcpy(hdr->addr1, next_hop->sta.addr, ETH_ALEN);
 		memcpy(hdr->addr2, sdata->vif.addr, ETH_ALEN);
 		ieee80211_mps_set_frame_flags(sdata, next_hop, hdr);
-		err = 0;
+		return 0;
 	}
 
-endlookup:
-	rcu_read_unlock();
-	return err;
+	return -ENOENT;
 }
 
 void mesh_path_timer(struct timer_list *t)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 0a1148328f19..498bf580bff4 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -219,7 +219,7 @@ static struct mesh_path *mpath_lookup(struct mesh_table *tbl, const u8 *dst,
 {
 	struct mesh_path *mpath;
 
-	mpath = rhashtable_lookup_fast(&tbl->rhead, dst, mesh_rht_params);
+	mpath = rhashtable_lookup(&tbl->rhead, dst, mesh_rht_params);
 
 	if (mpath && mpath_expired(mpath)) {
 		spin_lock_bh(&mpath->state_lock);
-- 
2.17.0

[PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation

[Felix Fietkau]

skb->truesize can change due to memory reallocation or when adding extra
fragments. Adjust fq->memory_usage accordingly

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/tx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 51cc37802439..0b73a0fe8218 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3227,6 +3227,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 	u8 max_subframes = sta->sta.max_amsdu_subframes;
 	int max_frags = local->hw.max_tx_fragments;
 	int max_amsdu_len = sta->sta.max_amsdu_len;
+	int orig_truesize;
 	__be16 len;
 	void *data;
 	bool ret = false;
@@ -3267,6 +3268,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 	if (!head || skb_is_gso(head))
 		goto out;
 
+	orig_truesize = head->truesize;
 	orig_len = head->len;
 
 	if (skb->len + head->len > max_amsdu_len)
@@ -3324,6 +3326,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 	*frag_tail = skb;
 
 out_recalc:
+	fq->memory_usage += head->truesize - orig_truesize;
 	if (head->len != orig_len) {
 		flow->backlog += head->len - orig_len;
 		tin->backlog_bytes += head->len - orig_len;
-- 
2.17.0

Re: [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> skb->truesize can change due to memory reallocation or when adding extra
> fragments. Adjust fq->memory_usage accordingly

Nice catch.

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

[PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue

[Felix Fietkau]

Reduces lock contention on enqueue/dequeue of iTXQ packets

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 include/net/fq_impl.h | 18 ++++++++++--------
 net/mac80211/tx.c     | 15 ++++++++++-----
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h
index be7c0fab3478..2caa86660ab0 100644
--- a/include/net/fq_impl.h
+++ b/include/net/fq_impl.h
@@ -107,21 +107,23 @@ static struct sk_buff *fq_tin_dequeue(struct fq *fq,
 	return skb;
 }
 
+static u32 fq_flow_idx(struct fq *fq, struct sk_buff *skb)
+{
+	u32 hash = skb_get_hash_perturb(skb, fq->perturbation);
+
+	return reciprocal_scale(hash, fq->flows_cnt);
+}
+
 static struct fq_flow *fq_flow_classify(struct fq *fq,
-					struct fq_tin *tin,
+					struct fq_tin *tin, u32 idx,
 					struct sk_buff *skb,
 					fq_flow_get_default_t get_default_func)
 {
 	struct fq_flow *flow;
-	u32 hash;
-	u32 idx;
 
 	lockdep_assert_held(&fq->lock);
 
-	hash = skb_get_hash_perturb(skb, fq->perturbation);
-	idx = reciprocal_scale(hash, fq->flows_cnt);
 	flow = &fq->flows[idx];
-
 	if (flow->tin && flow->tin != tin) {
 		flow = get_default_func(fq, tin, idx, skb);
 		tin->collisions++;
@@ -153,7 +155,7 @@ static void fq_recalc_backlog(struct fq *fq,
 }
 
 static void fq_tin_enqueue(struct fq *fq,
-			   struct fq_tin *tin,
+			   struct fq_tin *tin, u32 idx,
 			   struct sk_buff *skb,
 			   fq_skb_free_t free_func,
 			   fq_flow_get_default_t get_default_func)
@@ -163,7 +165,7 @@ static void fq_tin_enqueue(struct fq *fq,
 
 	lockdep_assert_held(&fq->lock);
 
-	flow = fq_flow_classify(fq, tin, skb, get_default_func);
+	flow = fq_flow_classify(fq, tin, idx, skb, get_default_func);
 
 	flow->tin = tin;
 	flow->backlog += skb->len;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0b73a0fe8218..8127e43e12b1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1398,11 +1398,15 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local,
 {
 	struct fq *fq = &local->fq;
 	struct fq_tin *tin = &txqi->tin;
+	u32 flow_idx = fq_flow_idx(fq, skb);
 
 	ieee80211_set_skb_enqueue_time(skb);
-	fq_tin_enqueue(fq, tin, skb,
+
+	spin_lock_bh(&fq->lock);
+	fq_tin_enqueue(fq, tin, flow_idx, skb,
 		       fq_skb_free_func,
 		       fq_flow_get_default_func);
+	spin_unlock_bh(&fq->lock);
 }
 
 static bool fq_vlan_filter_func(struct fq *fq, struct fq_tin *tin,
@@ -1589,7 +1593,6 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
 				struct sta_info *sta,
 				struct sk_buff *skb)
 {
-	struct fq *fq = &local->fq;
 	struct ieee80211_vif *vif;
 	struct txq_info *txqi;
 
@@ -1607,9 +1610,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
 	if (!txqi)
 		return false;
 
-	spin_lock_bh(&fq->lock);
 	ieee80211_txq_enqueue(local, txqi, skb);
-	spin_unlock_bh(&fq->lock);
 
 	schedule_and_wake_txq(local, txqi);
 
@@ -3227,6 +3228,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 	u8 max_subframes = sta->sta.max_amsdu_subframes;
 	int max_frags = local->hw.max_tx_fragments;
 	int max_amsdu_len = sta->sta.max_amsdu_len;
+	u32 flow_idx;
 	int orig_truesize;
 	__be16 len;
 	void *data;
@@ -3256,6 +3258,8 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 		max_amsdu_len = min_t(int, max_amsdu_len,
 				      sta->sta.max_tid_amsdu_len[tid]);
 
+	flow_idx = fq_flow_idx(fq, skb);
+
 	spin_lock_bh(&fq->lock);
 
 	/* TODO: Ideally aggregation should be done on dequeue to remain
@@ -3263,7 +3267,8 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
 	 */
 
 	tin = &txqi->tin;
-	flow = fq_flow_classify(fq, tin, skb, fq_flow_get_default_func);
+	flow = fq_flow_classify(fq, tin, flow_idx, skb,
+				fq_flow_get_default_func);
 	head = skb_peek_tail(&flow->queue);
 	if (!head || skb_is_gso(head))
 		goto out;
-- 
2.17.0

Re: [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

Seems reasonable.

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

[PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Felix Fietkau]

Reduces lock contention on enqueue/dequeue of iTXQ packets

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/tx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8127e43e12b1..f85344c9af62 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	ieee80211_tx_result r;
 	struct ieee80211_vif *vif = txq->vif;
 
+begin:
 	spin_lock_bh(&fq->lock);
 
 	if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
@@ -3560,11 +3561,12 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	if (skb)
 		goto out;
 
-begin:
 	skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
 	if (!skb)
 		goto out;
 
+	spin_unlock_bh(&fq->lock);
+
 	hdr = (struct ieee80211_hdr *)skb->data;
 	info = IEEE80211_SKB_CB(skb);
 
@@ -3610,8 +3612,11 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 
 		skb = __skb_dequeue(&tx.skbs);
 
-		if (!skb_queue_empty(&tx.skbs))
+		if (!skb_queue_empty(&tx.skbs)) {
+			spin_lock_bh(&fq->lock);
 			skb_queue_splice_tail(&tx.skbs, &txqi->frags);
+			spin_unlock_bh(&fq->lock);
+		}
 	}
 
 	if (skb_has_frag_list(skb) &&
@@ -3650,6 +3655,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+	return skb;
 
 out:
 	spin_unlock_bh(&fq->lock);
-- 
2.17.0

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

Also reasonable.

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

On 3/17/2019 1:06 AM, Felix Fietkau wrote:
> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/tx.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 8127e43e12b1..f85344c9af62 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>   	ieee80211_tx_result r;
>   	struct ieee80211_vif *vif = txq->vif;
>   
> +begin:
>   	spin_lock_bh(&fq->lock);
Maybe use-after-free will happened?

You can see ieee80211_tx_dequeue() in tx.c as below, after 
ieee80211_free_txskb(), it will goto begin,
If goto out happened in below check, then the skb which is freed will be 
returned, and use-after-free will happen.

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538
begin:
     spin_lock_bh(&fq->lock);

     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
         goto out;

     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
         goto out;
     }

     /* Make sure fragments stay together. */
     skb = __skb_dequeue(&txqi->frags);
     if (skb)
         goto out;

     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
     if (!skb)
         goto out;

     spin_unlock_bh(&fq->lock);

Maybe "skb = NULL;" should be added after "begin:".

...

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

On 3/17/2019 1:06 AM, Felix Fietkau wrote:
> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/tx.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 8127e43e12b1..f85344c9af62 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>   	ieee80211_tx_result r;
>   	struct ieee80211_vif *vif = txq->vif;
>   
> +begin:
>   	spin_lock_bh(&fq->lock);
Maybe use-after-free will happened?

You can see ieee80211_tx_dequeue() in tx.c as below, after 
ieee80211_free_txskb(), it will goto begin,
If goto out happened in below check, then the skb which is freed will be 
returned, and use-after-free will happen.

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538
begin:
     spin_lock_bh(&fq->lock);

     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
         goto out;

     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
         goto out;
     }

     /* Make sure fragments stay together. */
     skb = __skb_dequeue(&txqi->frags);
     if (skb)
         goto out;

     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
     if (!skb)
         goto out;

     spin_unlock_bh(&fq->lock);

Maybe "skb = NULL;" should be added after "begin:".

...


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

Hi Johannes,

do you know it?

On 12/5/2022 5:46 PM, Wen Gong wrote:
> On 3/17/2019 1:06 AM, Felix Fietkau wrote:
>> Reduces lock contention on enqueue/dequeue of iTXQ packets
>>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/tx.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 8127e43e12b1..f85344c9af62 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
>> ieee80211_hw *hw,
>>       ieee80211_tx_result r;
>>       struct ieee80211_vif *vif = txq->vif;
>>   +begin:
>>       spin_lock_bh(&fq->lock);
> Maybe use-after-free will happened?
>
> You can see ieee80211_tx_dequeue() in tx.c as below, after 
> ieee80211_free_txskb(), it will goto begin,
> If goto out happened in below check, then the skb which is freed will 
> be returned, and use-after-free will happen.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 
>
> begin:
>     spin_lock_bh(&fq->lock);
>
>     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
>         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
>         goto out;
>
>     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
>         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
>         goto out;
>     }
>
>     /* Make sure fragments stay together. */
>     skb = __skb_dequeue(&txqi->frags);
>     if (skb)
>         goto out;
>
>     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
>     if (!skb)
>         goto out;
>
>     spin_unlock_bh(&fq->lock);
>
> Maybe "skb = NULL;" should be added after "begin:".
>
> ...
>

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

Hi Johannes,

do you know it?

On 12/5/2022 5:46 PM, Wen Gong wrote:
> On 3/17/2019 1:06 AM, Felix Fietkau wrote:
>> Reduces lock contention on enqueue/dequeue of iTXQ packets
>>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/tx.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 8127e43e12b1..f85344c9af62 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
>> ieee80211_hw *hw,
>>       ieee80211_tx_result r;
>>       struct ieee80211_vif *vif = txq->vif;
>>   +begin:
>>       spin_lock_bh(&fq->lock);
> Maybe use-after-free will happened?
>
> You can see ieee80211_tx_dequeue() in tx.c as below, after 
> ieee80211_free_txskb(), it will goto begin,
> If goto out happened in below check, then the skb which is freed will 
> be returned, and use-after-free will happen.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 
>
> begin:
>     spin_lock_bh(&fq->lock);
>
>     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
>         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
>         goto out;
>
>     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
>         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
>         goto out;
>     }
>
>     /* Make sure fragments stay together. */
>     skb = __skb_dequeue(&txqi->frags);
>     if (skb)
>         goto out;
>
>     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
>     if (!skb)
>         goto out;
>
>     spin_unlock_bh(&fq->lock);
>
> Maybe "skb = NULL;" should be added after "begin:".
>
> ...
>

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

I will send a patch for it to avoid the potential user-after-free risk.

On 12/7/2022 2:30 PM, Wen Gong wrote:
> Hi Johannes,
>
> do you know it?
>
> On 12/5/2022 5:46 PM, Wen Gong wrote:
>> On 3/17/2019 1:06 AM, Felix Fietkau wrote:
>>> Reduces lock contention on enqueue/dequeue of iTXQ packets
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>   net/mac80211/tx.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>>> index 8127e43e12b1..f85344c9af62 100644
>>> --- a/net/mac80211/tx.c
>>> +++ b/net/mac80211/tx.c
>>> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
>>> ieee80211_hw *hw,
>>>       ieee80211_tx_result r;
>>>       struct ieee80211_vif *vif = txq->vif;
>>>   +begin:
>>>       spin_lock_bh(&fq->lock);
>> Maybe use-after-free will happened?
>>
>> You can see ieee80211_tx_dequeue() in tx.c as below, after 
>> ieee80211_free_txskb(), it will goto begin,
>> If goto out happened in below check, then the skb which is freed will 
>> be returned, and use-after-free will happen.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 
>>
>> begin:
>>     spin_lock_bh(&fq->lock);
>>
>>     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
>>         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
>>         goto out;
>>
>>     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
>>         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
>>         goto out;
>>     }
>>
>>     /* Make sure fragments stay together. */
>>     skb = __skb_dequeue(&txqi->frags);
>>     if (skb)
>>         goto out;
>>
>>     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
>>     if (!skb)
>>         goto out;
>>
>>     spin_unlock_bh(&fq->lock);
>>
>> Maybe "skb = NULL;" should be added after "begin:".
>>
>> ...
>>

Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

[Wen Gong]

I will send a patch for it to avoid the potential user-after-free risk.

On 12/7/2022 2:30 PM, Wen Gong wrote:
> Hi Johannes,
>
> do you know it?
>
> On 12/5/2022 5:46 PM, Wen Gong wrote:
>> On 3/17/2019 1:06 AM, Felix Fietkau wrote:
>>> Reduces lock contention on enqueue/dequeue of iTXQ packets
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>   net/mac80211/tx.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>>> index 8127e43e12b1..f85344c9af62 100644
>>> --- a/net/mac80211/tx.c
>>> +++ b/net/mac80211/tx.c
>>> @@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
>>> ieee80211_hw *hw,
>>>       ieee80211_tx_result r;
>>>       struct ieee80211_vif *vif = txq->vif;
>>>   +begin:
>>>       spin_lock_bh(&fq->lock);
>> Maybe use-after-free will happened?
>>
>> You can see ieee80211_tx_dequeue() in tx.c as below, after 
>> ieee80211_free_txskb(), it will goto begin,
>> If goto out happened in below check, then the skb which is freed will 
>> be returned, and use-after-free will happen.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 
>>
>> begin:
>>     spin_lock_bh(&fq->lock);
>>
>>     if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
>>         test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
>>         goto out;
>>
>>     if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
>>         set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
>>         goto out;
>>     }
>>
>>     /* Make sure fragments stay together. */
>>     skb = __skb_dequeue(&txqi->frags);
>>     if (skb)
>>         goto out;
>>
>>     skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
>>     if (!skb)
>>         goto out;
>>
>>     spin_unlock_bh(&fq->lock);
>>
>> Maybe "skb = NULL;" should be added after "begin:".
>>
>> ...
>>

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

[PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Felix Fietkau]

When using iTXQ, tx sequence number allocation and statistics are run at
dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
allows tx handlers to run on multiple CPUs in parallel.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/iface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 8ab23bbfba3e..7a4ea97d15af 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1227,6 +1227,7 @@ static void ieee80211_if_setup(struct net_device *dev)
 static void ieee80211_if_setup_no_queue(struct net_device *dev)
 {
 	ieee80211_if_setup(dev);
+	dev->features |= NETIF_F_LLTX;
 	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
-- 
2.17.0

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> When using iTXQ, tx sequence number allocation and statistics are run at
> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
> allows tx handlers to run on multiple CPUs in parallel.

Cool, didn't know about that flag.

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

+ Herbert

On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> When using iTXQ, tx sequence number allocation and statistics are run at
>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>> allows tx handlers to run on multiple CPUs in parallel.
> 
> Cool, didn't know about that flag.

It is water under the bridge as this patch got applied already, but I 
stumbled upon it just recently and didn't know about that flag either. 
So I looked for more information about it and found the definition [1], 
but the comment seemed important enough to send this reply.

	NETIF_F_LLTX_BIT,	/* LockLess TX - deprecated. Please */
				/* do not use LLTX in new drivers */

Here is the commit that marked it deprecated:

commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
Author: Christian Borntraeger <borntraeger@de.ibm.com>
Date:   Tue Sep 25 19:42:02 2007 -0700

     [NET]: note that NETIF_F_LLTX is deprecated

So I am not sure we should really do this in mac80211. Maybe Herbert can 
comment although it has been over a decade ago.

Regards,
Arend

[1] 
https://elixir.bootlin.com/linux/latest/source/include/linux/netdev_features.h#L32

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Felix Fietkau]

On 2019-04-14 11:44, Arend Van Spriel wrote:
> + Herbert
> 
> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>> allows tx handlers to run on multiple CPUs in parallel.
>> 
>> Cool, didn't know about that flag.
> 
> It is water under the bridge as this patch got applied already, but I 
> stumbled upon it just recently and didn't know about that flag either. 
> So I looked for more information about it and found the definition [1], 
> but the comment seemed important enough to send this reply.
> 
> 	NETIF_F_LLTX_BIT,	/* LockLess TX - deprecated. Please */
> 				/* do not use LLTX in new drivers */
> 
> Here is the commit that marked it deprecated:
> 
> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
> Author: Christian Borntraeger <borntraeger@de.ibm.com>
> Date:   Tue Sep 25 19:42:02 2007 -0700
> 
>      [NET]: note that NETIF_F_LLTX is deprecated
> 
> So I am not sure we should really do this in mac80211. Maybe Herbert can 
> comment although it has been over a decade ago.
There is a lot of comparable code that also uses this flag, e.g.
batman-adv, bridge, vlan, various tunnel implementations. I think
mac80211 fits well with those kinds of use cases.
If I remember correctly, the deprecation was added to avoid quirky
custom locking schemes in ethernet drivers.

- Felix

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

On April 14, 2019 1:19:49 PM Felix Fietkau <nbd@nbd.name> wrote:

> On 2019-04-14 11:44, Arend Van Spriel wrote:
>> + Herbert
>> 
>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>> 
>>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>>> allows tx handlers to run on multiple CPUs in parallel.
>>> 
>>> Cool, didn't know about that flag.
>> 
>> It is water under the bridge as this patch got applied already, but I
>> stumbled upon it just recently and didn't know about that flag either.
>> So I looked for more information about it and found the definition [1],
>> but the comment seemed important enough to send this reply.
>> 
>> 	NETIF_F_LLTX_BIT,	/* LockLess TX - deprecated. Please */
>> 				/* do not use LLTX in new drivers */
>> 
>> Here is the commit that marked it deprecated:
>> 
>> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
>> Author: Christian Borntraeger <borntraeger@de.ibm.com>
>> Date:   Tue Sep 25 19:42:02 2007 -0700
>> 
>>      [NET]: note that NETIF_F_LLTX is deprecated
>> 
>> So I am not sure we should really do this in mac80211. Maybe Herbert can
>> comment although it has been over a decade ago.
> There is a lot of comparable code that also uses this flag, e.g.
> batman-adv, bridge, vlan, various tunnel implementations. I think
> mac80211 fits well with those kinds of use cases.

Ok. As said I was not sure so I can/will not argue.

> If I remember correctly, the deprecation was added to avoid quirky
> custom locking schemes in ethernet drivers.

What do you mean by "quirky custom locking schemes"? You mean that TX path 
would use driver data that should actually be accessed under some lock?

When seeing the deprecated comment I wanted to know the why and was hoping 
the commit message would divulge. It just mentions it is not needed. So now 
I am curious as to why it wouldn't be needed especially as you say there 
are (valid) use-cases in the kernel today.

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

On 4/14/2019 2:34 PM, Arend Van Spriel wrote:
> On April 14, 2019 1:19:49 PM Felix Fietkau <nbd@nbd.name> wrote:
> 
>> On 2019-04-14 11:44, Arend Van Spriel wrote:
>>> + Herbert
>>>
>>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>
>>>>> When using iTXQ, tx sequence number allocation and statistics are 
>>>>> run at
>>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, 
>>>>> which
>>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>>
>>>> Cool, didn't know about that flag.
>>>
>>> It is water under the bridge as this patch got applied already, but I
>>> stumbled upon it just recently and didn't know about that flag either.
>>> So I looked for more information about it and found the definition [1],
>>> but the comment seemed important enough to send this reply.
>>>
>>>     NETIF_F_LLTX_BIT,    /* LockLess TX - deprecated. Please */
>>>                 /* do not use LLTX in new drivers */
>>>
>>> Here is the commit that marked it deprecated:
>>>
>>> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
>>> Author: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Date:   Tue Sep 25 19:42:02 2007 -0700
>>>
>>>      [NET]: note that NETIF_F_LLTX is deprecated
>>>
>>> So I am not sure we should really do this in mac80211. Maybe Herbert can
>>> comment although it has been over a decade ago.
>> There is a lot of comparable code that also uses this flag, e.g.
>> batman-adv, bridge, vlan, various tunnel implementations. I think
>> mac80211 fits well with those kinds of use cases.
> 
> Ok. As said I was not sure so I can/will not argue.
> 
>> If I remember correctly, the deprecation was added to avoid quirky
>> custom locking schemes in ethernet drivers.
> 
> What do you mean by "quirky custom locking schemes"? You mean that TX 
> path would use driver data that should actually be accessed under some 
> lock?
> 
> When seeing the deprecated comment I wanted to know the why and was 
> hoping the commit message would divulge. It just mentions it is not 
> needed. So now I am curious as to why it wouldn't be needed especially 
> as you say there are (valid) use-cases in the kernel today.

Getting back to this in an attempt to clarify my question. So from what 
Felix is saying the NETIF_F_LLTX flag is not deprecated, but restricted. 
What I would like to know is what exactly is required from a driver to 
allow the use of this flag.

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
> + Herbert
> 
> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
> > Felix Fietkau <nbd@nbd.name> writes:
> > 
> > > When using iTXQ, tx sequence number allocation and statistics are run at
> > > dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
> > > allows tx handlers to run on multiple CPUs in parallel.
> > 
> > Cool, didn't know about that flag.
> 
> It is water under the bridge as this patch got applied already, but I
> stumbled upon it just recently and didn't know about that flag either. So I
> looked for more information about it and found the definition [1], but the
> comment seemed important enough to send this reply.
> 
> 	NETIF_F_LLTX_BIT,	/* LockLess TX - deprecated. Please */
> 				/* do not use LLTX in new drivers */

The most obvious problem with LLTX is that it can cause AF_PACKET
to see packets twice.  But the more subtle issue is that with a
proper design this lock should have no contention anyway.

So please explain why you want to use this in your driver and where
the contention is coming from?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

On 4/16/2019 9:44 AM, Herbert Xu wrote:
> On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
>> + Herbert
>>
>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>>
>>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>
>>> Cool, didn't know about that flag.
>>
>> It is water under the bridge as this patch got applied already, but I
>> stumbled upon it just recently and didn't know about that flag either. So I
>> looked for more information about it and found the definition [1], but the
>> comment seemed important enough to send this reply.
>>
>> 	NETIF_F_LLTX_BIT,	/* LockLess TX - deprecated. Please */
>> 				/* do not use LLTX in new drivers */
> 
> The most obvious problem with LLTX is that it can cause AF_PACKET
> to see packets twice.  But the more subtle issue is that with a
> proper design this lock should have no contention anyway.
> 
> So please explain why you want to use this in your driver and where
> the contention is coming from?

Hi Herbert,

I was just writing up an email clarifying my question. But let me 
summarize this email thread. The patch from Felix adds this flag in 
mac80211 for drivers that indicate to support pulling packets from the 
internal TXQ in mac80211. I found it is deprecated, but as Felix 
mentioned it is used in various parts of the network subsystem, ie. 
batman-adv, bridge, vlan, tunnel implementations. So its use seems to be 
restricted rather than deprecated. Given your response above I guess my 
question would be to get details about what you call "proper design" as 
I think you are saying with that it is not needed, right?

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
> 
> I was just writing up an email clarifying my question. But let me summarize
> this email thread. The patch from Felix adds this flag in mac80211 for
> drivers that indicate to support pulling packets from the internal TXQ in
> mac80211. I found it is deprecated, but as Felix mentioned it is used in
> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> implementations. So its use seems to be restricted rather than deprecated.
> Given your response above I guess my question would be to get details about
> what you call "proper design" as I think you are saying with that it is not
> needed, right?

Essentially the only time it would be OK to use LLTX in its current
form is if you have no TX queue/congestion feedback which is clearly
not the case with wireless drivers.

Chers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
> > 
> > I was just writing up an email clarifying my question. But let me summarize
> > this email thread. The patch from Felix adds this flag in mac80211 for
> > drivers that indicate to support pulling packets from the internal TXQ in
> > mac80211. I found it is deprecated, but as Felix mentioned it is used in
> > various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> > implementations. So its use seems to be restricted rather than deprecated.
> > Given your response above I guess my question would be to get details about
> > what you call "proper design" as I think you are saying with that it is not
> > needed, right?
> 
> Essentially the only time it would be OK to use LLTX in its current
> form is if you have no TX queue/congestion feedback which is clearly
> not the case with wireless drivers.

It is true because we have an entire buffering layer in mac80211 (in
this case at least) and never push back to the stack.

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

On 4/16/2019 10:37 AM, Johannes Berg wrote:
> On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>>>
>>> I was just writing up an email clarifying my question. But let me summarize
>>> this email thread. The patch from Felix adds this flag in mac80211 for
>>> drivers that indicate to support pulling packets from the internal TXQ in
>>> mac80211. I found it is deprecated, but as Felix mentioned it is used in
>>> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
>>> implementations. So its use seems to be restricted rather than deprecated.
>>> Given your response above I guess my question would be to get details about
>>> what you call "proper design" as I think you are saying with that it is not
>>> needed, right?
>>
>> Essentially the only time it would be OK to use LLTX in its current
>> form is if you have no TX queue/congestion feedback which is clearly
>> not the case with wireless drivers.
> 
> It is true because we have an entire buffering layer in mac80211 (in
> this case at least) and never push back to the stack.

Ok, so the crux is the "never push back to the stack" part? Well, the 
internal TXQ and how that is used is obviously enabling that ;-)

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:17:53AM +0200, Arend Van Spriel wrote:
> On 4/16/2019 10:37 AM, Johannes Berg wrote:
> > It is true because we have an entire buffering layer in mac80211 (in
> > this case at least) and never push back to the stack.
> 
> Ok, so the crux is the "never push back to the stack" part? Well, the
> internal TXQ and how that is used is obviously enabling that ;-)

So assuming that these drivers all have a TX queue length of zero
and therefore do not make use of Linux queueing disciplines then
yes techincally LLTX is fine.

However, I must say that it is much better to provide real
congestion feedback to the stack when you can because otherwise
things like UDP may fall apart.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>> > 
>> > I was just writing up an email clarifying my question. But let me summarize
>> > this email thread. The patch from Felix adds this flag in mac80211 for
>> > drivers that indicate to support pulling packets from the internal TXQ in
>> > mac80211. I found it is deprecated, but as Felix mentioned it is used in
>> > various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
>> > implementations. So its use seems to be restricted rather than deprecated.
>> > Given your response above I guess my question would be to get details about
>> > what you call "proper design" as I think you are saying with that it is not
>> > needed, right?
>> 
>> Essentially the only time it would be OK to use LLTX in its current
>> form is if you have no TX queue/congestion feedback which is clearly
>> not the case with wireless drivers.
>
> It is true because we have an entire buffering layer in mac80211 (in
> this case at least) and never push back to the stack.

I'm wondering if we should be? For instance, fq_codel returns
NET_XMIT_CN if it drops a packet from the same flow that it enqueued to.
We could conceivably do the same in mac80211, although we'd have to
carry the return value out through quite a few layers. Not sure if this
is worth it?

Eric, do you have any insight into what impact the _CN return has from
fq_codel?

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
> 
> > It is true because we have an entire buffering layer in mac80211 (in
> > this case at least) and never push back to the stack.
> 
> I'm wondering if we should be?

I don't think so? We'd just buffer packets in yet another place.

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
> On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
> > 
> > > It is true because we have an entire buffering layer in mac80211 (in
> > > this case at least) and never push back to the stack.
> > 
> > I'm wondering if we should be?
> 
> I don't think so? We'd just buffer packets in yet another place.

But you do realise that you're giving up on the rich queueing
functionality that Linux provides (net/sched), not to mention
breaking certain applications that rely on congestion feedback?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 17:37 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
> > On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
> > > 
> > > > It is true because we have an entire buffering layer in mac80211 (in
> > > > this case at least) and never push back to the stack.
> > > 
> > > I'm wondering if we should be?
> > 
> > I don't think so? We'd just buffer packets in yet another place.
> 
> But you do realise that you're giving up on the rich queueing
> functionality that Linux provides (net/sched),

Yes, that was a trade-off we always knew about. The model that Linux
provides is just not suited for wifi.

> not to mention
> breaking certain applications that rely on congestion feedback?

This I don't understand. The congestion feedback happens through socket
buffer space etc. which is still there (as long as nobody sneaks in an
skb_orphan() call)

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2019-04-16 at 17:37 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
>> > On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
>> > > 
>> > > > It is true because we have an entire buffering layer in mac80211 (in
>> > > > this case at least) and never push back to the stack.
>> > > 
>> > > I'm wondering if we should be?
>> > 
>> > I don't think so? We'd just buffer packets in yet another place.
>> 
>> But you do realise that you're giving up on the rich queueing
>> functionality that Linux provides (net/sched),
>
> Yes, that was a trade-off we always knew about. The model that Linux
> provides is just not suited for wifi.

As explained at great length here:
https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
(you already know that of course, Johannes)

>> not to mention
>> breaking certain applications that rely on congestion feedback?
>
> This I don't understand. The congestion feedback happens through socket
> buffer space etc. which is still there (as long as nobody sneaks in an
> skb_orphan() call)

Sure, for TCP, the TSQ mechanism should keep the upper-level queue low
as long as the SKBs are alive. But is this also the case for UDP?

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:02:50AM +0100, Toke Høiland-Jørgensen wrote:
> 
> As explained at great length here:
> https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
> (you already know that of course, Johannes)

I can understand that wireless needs its own queueing scheme, but
it still seems wrong to place that under net/mac80211 as opposed
to having it as a first-class citizen under net/sched.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 11:02:50AM +0100, Toke Høiland-Jørgensen wrote:
>> 
>> As explained at great length here:
>> https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
>> (you already know that of course, Johannes)
>
> I can understand that wireless needs its own queueing scheme, but it
> still seems wrong to place that under net/mac80211 as opposed to
> having it as a first-class citizen under net/sched.

This is because we need to resolve the MAC-layer destination station (or
rather, TID) and tie the queueing to that, because of aggregation. We
also use the queueing structure for scheduling stations to achieve
airtime fairness. Both of these would be decidedly non-trivial to pull
up to the qdisc layer. Rather, having them in mac80211 means drivers
don't need to do their own ad-hoc queueing (which was the case before,
leading extra bufferbloat).

Most of the actual queueing structure code lives in
include/net/fq_impl.h, though, so it's not actually that
mac80211-specific. I've been thinking about porting the relevant qdiscs
to use the same code, but I'm not sure that it's worth the trouble.

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
> 
> > not to mention
> > breaking certain applications that rely on congestion feedback?
> 
> This I don't understand. The congestion feedback happens through socket
> buffer space etc. which is still there (as long as nobody sneaks in an
> skb_orphan() call)

The congestion control happens at two levels.  You are right that
the socket buffer acts as one limit.  However, other applications
may also rely on the TX queue being full as the throttle (by setting
a sufficiently large socket buffer size).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
>> 
>> > not to mention
>> > breaking certain applications that rely on congestion feedback?
>> 
>> This I don't understand. The congestion feedback happens through socket
>> buffer space etc. which is still there (as long as nobody sneaks in an
>> skb_orphan() call)
>
> The congestion control happens at two levels. You are right that the
> socket buffer acts as one limit. However, other applications may also
> rely on the TX queue being full as the throttle (by setting a
> sufficiently large socket buffer size).

Do you happen to have an example of an application that does this that
could be used for testing? :)

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>
> > The congestion control happens at two levels. You are right that the
> > socket buffer acts as one limit. However, other applications may also
> > rely on the TX queue being full as the throttle (by setting a
> > sufficiently large socket buffer size).
> 
> Do you happen to have an example of an application that does this that
> could be used for testing? :)

Have a look at

commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Wed Sep 2 18:05:33 2009 -0700

    ip: Report qdisc packet drops

You should be able to do a UDP flood while setting IP_RECVERR to
detect the packet drop due to a full queue which AFAICS will never
happen with the current mac80211 setup.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>
>> > The congestion control happens at two levels. You are right that the
>> > socket buffer acts as one limit. However, other applications may also
>> > rely on the TX queue being full as the throttle (by setting a
>> > sufficiently large socket buffer size).
>> 
>> Do you happen to have an example of an application that does this that
>> could be used for testing? :)
>
> Have a look at
>
> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Wed Sep 2 18:05:33 2009 -0700
>
>     ip: Report qdisc packet drops
>
> You should be able to do a UDP flood while setting IP_RECVERR to
> detect the packet drop due to a full queue which AFAICS will never
> happen with the current mac80211 setup.

Yup, got that part. Was just wondering if you know of any applications
that already do this, that I could test without having to write my
own... :)

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

+ Bob

On 4/17/2019 11:09 AM, Toke Høiland-Jørgensen wrote:
> Herbert Xu <herbert@gondor.apana.org.au> writes:
> 
>> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>>
>>>> The congestion control happens at two levels. You are right that the
>>>> socket buffer acts as one limit. However, other applications may also
>>>> rely on the TX queue being full as the throttle (by setting a
>>>> sufficiently large socket buffer size).
>>>
>>> Do you happen to have an example of an application that does this that
>>> could be used for testing? :)
>>
>> Have a look at
>>
>> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>> Date:   Wed Sep 2 18:05:33 2009 -0700
>>
>>      ip: Report qdisc packet drops
>>
>> You should be able to do a UDP flood while setting IP_RECVERR to
>> detect the packet drop due to a full queue which AFAICS will never
>> happen with the current mac80211 setup.
> 
> Yup, got that part. Was just wondering if you know of any applications
> that already do this, that I could test without having to write my
> own... :)

Hi Bob,

Is this something that could be easily implemented in iperf?

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>
>> > The congestion control happens at two levels. You are right that the
>> > socket buffer acts as one limit. However, other applications may also
>> > rely on the TX queue being full as the throttle (by setting a
>> > sufficiently large socket buffer size).
>> 
>> Do you happen to have an example of an application that does this that
>> could be used for testing? :)
>
> Have a look at
>
> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Wed Sep 2 18:05:33 2009 -0700
>
>     ip: Report qdisc packet drops
>
> You should be able to do a UDP flood while setting IP_RECVERR to
> detect the packet drop due to a full queue which AFAICS will never
> happen with the current mac80211 setup.

Also, looking at udp.c, it seems it uses net_xmit_errno() - which means
that returning NET_XMIT_CN has the same effect as NET_XMIT_SUCCESS when
propagated back to userspace? Which would kinda defeat the point of
going to the trouble of propagating up the return code (the mac80211
queue will never drop the most recently enqueued packet)...

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Wed, 2019-04-17 at 10:17 +0100, Toke Høiland-Jørgensen wrote:
> Herbert Xu <herbert@gondor.apana.org.au> writes:
> 
> > On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
> > > 
> > > > The congestion control happens at two levels. You are right that the
> > > > socket buffer acts as one limit. However, other applications may also
> > > > rely on the TX queue being full as the throttle (by setting a
> > > > sufficiently large socket buffer size).
> > > 
> > > Do you happen to have an example of an application that does this that
> > > could be used for testing? :)
> > 
> > Have a look at
> > 
> > commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> > Author: Eric Dumazet <eric.dumazet@gmail.com>
> > Date:   Wed Sep 2 18:05:33 2009 -0700
> > 
> >     ip: Report qdisc packet drops
> > 
> > You should be able to do a UDP flood while setting IP_RECVERR to
> > detect the packet drop due to a full queue which AFAICS will never
> > happen with the current mac80211 setup.
> 
> Also, looking at udp.c, it seems it uses net_xmit_errno() - which means
> that returning NET_XMIT_CN has the same effect as NET_XMIT_SUCCESS when
> propagated back to userspace? Which would kinda defeat the point of
> going to the trouble of propagating up the return code (the mac80211
> queue will never drop the most recently enqueued packet)...

I guess there might be value in returning NET_XMIT_CN anyway, but I
think you're right in that we can never return anything but
NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
just older ones.

Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
>
> I guess there might be value in returning NET_XMIT_CN anyway, but I
> think you're right in that we can never return anything but
> NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> just older ones.
> 
> Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?

Pretty sure codel does return NET_XMIT_CN.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Thu, 2019-04-25 at 16:35 +0800, Herbert Xu wrote:
> On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
> > 
> > I guess there might be value in returning NET_XMIT_CN anyway, but I
> > think you're right in that we can never return anything but
> > NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> > just older ones.
> > 
> > Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?
> 
> Pretty sure codel does return NET_XMIT_CN.

Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
NET_XMIT_CN. This will not trigger the code you mentioned before though.

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
>
> Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> NET_XMIT_CN. This will not trigger the code you mentioned before though.

You are right that it does not.  However, the fact that this
congestion indication is lost is a bug rather than a feature.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Thu, 2019-04-25 at 16:44 +0800, Herbert Xu wrote:
> On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
> > 
> > Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> > NET_XMIT_CN. This will not trigger the code you mentioned before though.
> 
> You are right that it does not.  However, the fact that this
> congestion indication is lost is a bug rather than a feature.

You can argue that way, I guess.

However, *any* queue management algorithm that doesn't *solely* employ
tail drops will necessarily behave this way.

I would argue that you get better queue management if you don't solely
rely on tail drops, so I'd rather pick better queue management than this
(relatively obscure) congestion notification. The more commonly relevant
socket buffer size will work for both.

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 21:13 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
> > 
> > > not to mention
> > > breaking certain applications that rely on congestion feedback?
> > 
> > This I don't understand. The congestion feedback happens through socket
> > buffer space etc. which is still there (as long as nobody sneaks in an
> > skb_orphan() call)
> 
> The congestion control happens at two levels.  You are right that
> the socket buffer acts as one limit.  However, other applications
> may also rely on the TX queue being full as the throttle (by setting
> a sufficiently large socket buffer size).

I'm not sure how they'd even realize this, other than packets getting
dropped? Which of course we do here as well, we didn't invent something
that let's us expand memory arbitrarily ;-)

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 09:13:16PM +0200, Johannes Berg wrote:
>
> I'm not sure how they'd even realize this, other than packets getting
> dropped? Which of course we do here as well, we didn't invent something
> that let's us expand memory arbitrarily ;-)

They rely on the queueing mechanism providing feedback via an
error return value when the queue becomes full.  From my cursory
look at net/mac80211 you seem to be always returning success in
ieee80211_subif_start_xmit.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
>> 
>> > It is true because we have an entire buffering layer in mac80211 (in
>> > this case at least) and never push back to the stack.
>> 
>> I'm wondering if we should be?
>
> I don't think so? We'd just buffer packets in yet another place.

Wouldn't we get pushback all the way to the application socket? I.e.,
an UDP application would get sendto() failures?

-Toke