lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4aadb1a9-b4a8-595e-b413-99bc3aef480a@quicinc.com>
Date: Fri, 18 Oct 2024 12:57:48 +0530
From: Vasanthakumar Thiagarajan <quic_vthiagar@...cinc.com>
To: Remi Pommarel <repk@...plefau.lt>, <ath10k@...ts.infradead.org>,
        <linux-wireless@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: Kalle Valo <kvalo@...nel.org>, Jeff Johnson <jjohnson@...nel.org>,
        Cedric
 Veilleux <veilleux.cedric@...il.com>
Subject: Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback



On 10/12/2024 7:43 PM, Remi Pommarel wrote:
> When a STA reassociates, mac80211's _sta_info_move_state() waits for all
> pending frame to be flushed before removing the key (so that no frame
> get sent unencrypted after key removable [0]). When a driver does not
> implement the flush_sta callback, ieee80211_flush_queues() is called
> instead which effectively stops the whole queue until it is completely
> drained.
> 
> The ath10k driver configure all STAs of one vdev to share the same
> queue. So when flushing one STA this is the whole vdev queue that is
> blocked until completely drained causing Tx to other STA to also stall
> this whole time.
> 
> One easy way to reproduce the issue is to connect two STAs (STA0 and
> STA1) to an ath10k AP. While Generating a bunch of traffic from AP to
> STA0 (e.g. fping -l -p 20 <STA0-IP>) disconnect STA0 from AP without
> clean disassociation (e.g. remove power, reboot -f). Then as soon as
> STA0 is effectively disconnected from AP (either after inactivity
> timeout or forced with iw dev AP station del STA0), its queues get
> flushed using ieee80211_flush_queues(). This causes STA1 to suffer a
> connectivity stall for about 5 seconds (see ATH10K_FLUSH_TIMEOUT_HZ).
> 
> Implement a flush_sta callback in ath10k to wait only for a specific
> STA pending frames to be drained (without stopping the whole HW queue)
> to fix that.
> 
> [0]: commit 0b75a1b1e42e ("wifi: mac80211: flush queues on STA removal")
> 
> Reported-by: Cedric Veilleux <veilleux.cedric@...il.com>
> Signed-off-by: Remi Pommarel <repk@...plefau.lt>
> ---
>   drivers/net/wireless/ath/ath10k/core.h   |  4 +++
>   drivers/net/wireless/ath/ath10k/htt.h    |  4 +++
>   drivers/net/wireless/ath/ath10k/htt_tx.c | 32 ++++++++++++++++++
>   drivers/net/wireless/ath/ath10k/mac.c    | 43 +++++++++++++++++++++++-
>   drivers/net/wireless/ath/ath10k/txrx.c   |  3 ++
>   5 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 446dca74f06a..4709e4887efc 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -558,6 +558,10 @@ struct ath10k_sta {
>   	u8 rate_ctrl[ATH10K_TID_MAX];
>   	u32 rate_code[ATH10K_TID_MAX];
>   	int rtscts[ATH10K_TID_MAX];
> +	/* protects num_fw_queued */
> +	spinlock_t sta_tx_lock;
> +	wait_queue_head_t empty_tx_wq;
> +	unsigned int num_fw_queued;
>   };
>   
>   #define ATH10K_VDEV_SETUP_TIMEOUT_HZ	(5 * HZ)
> diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
> index 603f6de62b0a..d150f9330941 100644
> --- a/drivers/net/wireless/ath/ath10k/htt.h
> +++ b/drivers/net/wireless/ath/ath10k/htt.h
> @@ -2452,6 +2452,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt);
>   void ath10k_htt_tx_mgmt_dec_pending(struct ath10k_htt *htt);
>   int ath10k_htt_tx_mgmt_inc_pending(struct ath10k_htt *htt, bool is_mgmt,
>   				   bool is_presp);
> +void ath10k_htt_tx_sta_inc_pending(struct ath10k_htt *htt,
> +				   struct ieee80211_sta *sta);
> +void ath10k_htt_tx_sta_dec_pending(struct ath10k_htt *htt,
> +				   struct ieee80211_sta *sta);
>   
>   int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt, struct sk_buff *skb);
>   void ath10k_htt_tx_free_msdu_id(struct ath10k_htt *htt, u16 msdu_id);
> diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
> index 9725feecefd6..7477cb8f5d10 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_tx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
> @@ -195,6 +195,38 @@ void ath10k_htt_tx_mgmt_dec_pending(struct ath10k_htt *htt)
>   	htt->num_pending_mgmt_tx--;
>   }
>   
> +void ath10k_htt_tx_sta_inc_pending(struct ath10k_htt *htt,
> +				   struct ieee80211_sta *sta)
> +{
> +	struct ath10k_sta *arsta;
> +
> +	if (!sta)
> +		return;
> +
> +	arsta = (struct ath10k_sta *)sta->drv_priv;
> +
> +	spin_lock_bh(&arsta->sta_tx_lock);
> +	arsta->num_fw_queued++;
> +	spin_unlock_bh(&arsta->sta_tx_lock);
> +}
> +
> +void ath10k_htt_tx_sta_dec_pending(struct ath10k_htt *htt,
> +				   struct ieee80211_sta *sta)
> +{
> +	struct ath10k_sta *arsta;
> +
> +	if (!sta)
> +		return;
> +
> +	arsta = (struct ath10k_sta *)sta->drv_priv;
> +
> +	spin_lock_bh(&arsta->sta_tx_lock);
> +	arsta->num_fw_queued--;
> +	if (arsta->num_fw_queued == 0)
> +		wake_up(&arsta->empty_tx_wq);
> +	spin_unlock_bh(&arsta->sta_tx_lock);
> +}
> +
>   int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt, struct sk_buff *skb)
>   {
>   	struct ath10k *ar = htt->ar;
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 646e1737d4c4..373a0aa6b01c 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4423,6 +4423,8 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
>   		spin_unlock_bh(&ar->htt.tx_lock);
>   	}
>   
> +	ath10k_htt_tx_sta_inc_pending(&ar->htt, sta);
> +
>   	ret = ath10k_mac_tx(ar, vif, txmode, txpath, skb, false);
>   	if (unlikely(ret)) {
>   		ath10k_warn(ar, "failed to push frame: %d\n", ret);
> @@ -4432,6 +4434,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
>   		if (is_mgmt)
>   			ath10k_htt_tx_mgmt_dec_pending(htt);
>   		spin_unlock_bh(&ar->htt.tx_lock);
> +		ath10k_htt_tx_sta_dec_pending(&ar->htt, sta);
>   
>   		return ret;
>   	}
> @@ -7474,7 +7477,8 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
>   		arsta->peer_ps_state = WMI_PEER_PS_STATE_DISABLED;
>   		INIT_WORK(&arsta->update_wk, ath10k_sta_rc_update_wk);
>   		INIT_WORK(&arsta->tid_config_wk, ath10k_sta_tid_cfg_wk);
> -
> +		spin_lock_init(&arsta->sta_tx_lock);
> +		init_waitqueue_head(&arsta->empty_tx_wq);
>   		for (i = 0; i < ARRAY_SIZE(sta->txq); i++)
>   			ath10k_mac_txq_init(sta->txq[i]);
>   	}
> @@ -8098,6 +8102,42 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>   	mutex_unlock(&ar->conf_mutex);
>   }
>   
> +static void ath10k_flush_sta(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +			     struct ieee80211_sta *sta)
> +{
> +	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> +	struct ath10k *ar = hw->priv;
> +	bool skip;
> +	long time_left;
> +
> +	/* TODO do we need drop implemented here ? */
> +
> +	mutex_lock(&ar->conf_mutex);
> +
> +	if (ar->state == ATH10K_STATE_WEDGED)
> +		goto out;
> +
> +	time_left = wait_event_timeout(arsta->empty_tx_wq, ({
> +			bool empty;
> +
> +			spin_lock_bh(&arsta->sta_tx_lock);
> +			empty = (arsta->num_fw_queued == 0);
> +			spin_unlock_bh(&arsta->sta_tx_lock);
> +
> +			skip = (ar->state == ATH10K_STATE_WEDGED) ||
> +			       test_bit(ATH10K_FLAG_CRASH_FLUSH,
> +					&ar->dev_flags);
> +
> +			(empty || skip);
> +		}), ATH10K_FLUSH_TIMEOUT_HZ);
> +
> +	if (time_left == 0 || skip)
> +		ath10k_warn(ar, "failed to flush sta txq (sta %pM skip %i ar-state %i): %ld\n",
> +			    sta->addr, skip, ar->state, time_left);
> +out:
> +	mutex_unlock(&ar->conf_mutex);
> +}
> +
>   /* TODO: Implement this function properly
>    * For now it is needed to reply to Probe Requests in IBSS mode.
>    * Probably we need this information from FW.
> @@ -9444,6 +9484,7 @@ static const struct ieee80211_ops ath10k_ops = {
>   	.set_rts_threshold		= ath10k_set_rts_threshold,
>   	.set_frag_threshold		= ath10k_mac_op_set_frag_threshold,
>   	.flush				= ath10k_flush,
> +	.flush_sta			= ath10k_flush_sta,
>   	.tx_last_beacon			= ath10k_tx_last_beacon,
>   	.set_antenna			= ath10k_set_antenna,
>   	.get_antenna			= ath10k_get_antenna,
> diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
> index da3bc35e41aa..ece56379b0f0 100644
> --- a/drivers/net/wireless/ath/ath10k/txrx.c
> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> @@ -91,6 +91,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>   					       skb_cb->airtime_est, 0);
>   	rcu_read_unlock();
>   
> +	if (txq)
> +		ath10k_htt_tx_sta_dec_pending(htt, txq->sta);
> +

This should be called within rcu?

Vasanth

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ