[PATCH for-5.18 v3] ath9k: Fix usage of driver-private space in tx_info

[PATCH for-5.18 v3] ath9k: Fix usage of driver-private space in tx_info

[Toke Høiland-Jørgensen]

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

The ieee80211_tx_info_clear_status() helper also clears the rate counts and
the driver-private part of struct ieee80211_tx_info, so using it breaks
quite a few other things. So back out of using it, and instead define a
ath-internal helper that only clears the area between the
status_driver_data and the rates info. Combined with moving the
ath_frame_info struct to status_driver_data, this avoids clearing anything
we shouldn't be, and so we can keep the existing code for handling the rate
information.

While fixing this I also noticed that the setting of
tx_info->status.rates[tx_rateindex].count on hardware underrun errors was
always immediately overridden by the normal setting of the same fields, so
rearrange the code so that the underrun detection actually takes effect.

The new helper could be generalised to a 'memset_between()' helper, but
leave it as a driver-internal helper for now since this needs to go to
stable.

Cc: stable@vger.kernel.org
Reported-by: Peter Seiderer <ps.report@gmx.net>
Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 drivers/net/wireless/ath/ath9k/main.c |  2 +-
 drivers/net/wireless/ath/ath9k/xmit.c | 30 ++++++++++++++++++---------
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 98090e40e1cf..e2791d45f5f5 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -839,7 +839,7 @@ static bool ath9k_txq_list_has_key(struct list_head *txq_list, u32 keyix)
 			continue;
 
 		txinfo = IEEE80211_SKB_CB(bf->bf_mpdu);
-		fi = (struct ath_frame_info *)&txinfo->rate_driver_data[0];
+		fi = (struct ath_frame_info *)&txinfo->status.status_driver_data[0];
 		if (fi->keyix == keyix)
 			return true;
 	}
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index cbcf96ac303e..db83cc4ba810 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -141,8 +141,8 @@ static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
 {
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 	BUILD_BUG_ON(sizeof(struct ath_frame_info) >
-		     sizeof(tx_info->rate_driver_data));
-	return (struct ath_frame_info *) &tx_info->rate_driver_data[0];
+		     sizeof(tx_info->status.status_driver_data));
+	return (struct ath_frame_info *) &tx_info->status.status_driver_data[0];
 }
 
 static void ath_send_bar(struct ath_atx_tid *tid, u16 seqno)
@@ -2542,6 +2542,16 @@ static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
 	spin_unlock_irqrestore(&sc->tx.txbuflock, flags);
 }
 
+static void ath_clear_tx_status(struct ieee80211_tx_info *tx_info)
+{
+	void *ptr = &tx_info->status;
+
+	memset(ptr + sizeof(tx_info->status.rates), 0,
+	       sizeof(tx_info->status) -
+	       sizeof(tx_info->status.rates) -
+	       sizeof(tx_info->status.status_driver_data));
+}
+
 static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
 			     struct ath_tx_status *ts, int nframes, int nbad,
 			     int txok)
@@ -2553,7 +2563,7 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
 	struct ath_hw *ah = sc->sc_ah;
 	u8 i, tx_rateindex;
 
-	ieee80211_tx_info_clear_status(tx_info);
+	ath_clear_tx_status(tx_info);
 
 	if (txok)
 		tx_info->status.ack_signal = ts->ts_rssi;
@@ -2569,6 +2579,13 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
 	tx_info->status.ampdu_len = nframes;
 	tx_info->status.ampdu_ack_len = nframes - nbad;
 
+	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
+
+	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
+		tx_info->status.rates[i].count = 0;
+		tx_info->status.rates[i].idx = -1;
+	}
+
 	if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
 	    (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
 		/*
@@ -2590,13 +2607,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
 			tx_info->status.rates[tx_rateindex].count =
 				hw->max_rate_tries;
 	}
-
-	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
-		tx_info->status.rates[i].count = 0;
-		tx_info->status.rates[i].idx = -1;
-	}
-
-	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
 }
 
 static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)
-- 
2.35.1

Re: [PATCH for-5.18 v3] ath9k: Fix usage of driver-private space in tx_info

[Peter Seiderer]

Hello Toke,

On Mon,  4 Apr 2022 22:48:00 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> The ieee80211_tx_info_clear_status() helper also clears the rate counts and
> the driver-private part of struct ieee80211_tx_info, so using it breaks
> quite a few other things. So back out of using it, and instead define a
> ath-internal helper that only clears the area between the
> status_driver_data and the rates info. Combined with moving the
> ath_frame_info struct to status_driver_data, this avoids clearing anything
> we shouldn't be, and so we can keep the existing code for handling the rate
> information.
> 
> While fixing this I also noticed that the setting of
> tx_info->status.rates[tx_rateindex].count on hardware underrun errors was
> always immediately overridden by the normal setting of the same fields, so
> rearrange the code so that the underrun detection actually takes effect.
> 
> The new helper could be generalised to a 'memset_between()' helper, but
> leave it as a driver-internal helper for now since this needs to go to
> stable.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Peter Seiderer <ps.report@gmx.net>
> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

And finally found time to test your latest version of the patch, you can add my

Reviewed-by: Peter Seiderer <ps.report@gmx.net>
Tested-by: Peter Seiderer <ps.report@gmx.net>

Thanks,

Peter Seiderer


> ---
>  drivers/net/wireless/ath/ath9k/main.c |  2 +-
>  drivers/net/wireless/ath/ath9k/xmit.c | 30 ++++++++++++++++++---------
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index 98090e40e1cf..e2791d45f5f5 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -839,7 +839,7 @@ static bool ath9k_txq_list_has_key(struct list_head *txq_list, u32 keyix)
>  			continue;
>  
>  		txinfo = IEEE80211_SKB_CB(bf->bf_mpdu);
> -		fi = (struct ath_frame_info *)&txinfo->rate_driver_data[0];
> +		fi = (struct ath_frame_info *)&txinfo->status.status_driver_data[0];
>  		if (fi->keyix == keyix)
>  			return true;
>  	}
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index cbcf96ac303e..db83cc4ba810 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -141,8 +141,8 @@ static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
>  {
>  	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
>  	BUILD_BUG_ON(sizeof(struct ath_frame_info) >
> -		     sizeof(tx_info->rate_driver_data));
> -	return (struct ath_frame_info *) &tx_info->rate_driver_data[0];
> +		     sizeof(tx_info->status.status_driver_data));
> +	return (struct ath_frame_info *) &tx_info->status.status_driver_data[0];
>  }
>  
>  static void ath_send_bar(struct ath_atx_tid *tid, u16 seqno)
> @@ -2542,6 +2542,16 @@ static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
>  	spin_unlock_irqrestore(&sc->tx.txbuflock, flags);
>  }
>  
> +static void ath_clear_tx_status(struct ieee80211_tx_info *tx_info)
> +{
> +	void *ptr = &tx_info->status;
> +
> +	memset(ptr + sizeof(tx_info->status.rates), 0,
> +	       sizeof(tx_info->status) -
> +	       sizeof(tx_info->status.rates) -
> +	       sizeof(tx_info->status.status_driver_data));
> +}
> +
>  static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  			     struct ath_tx_status *ts, int nframes, int nbad,
>  			     int txok)
> @@ -2553,7 +2563,7 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  	struct ath_hw *ah = sc->sc_ah;
>  	u8 i, tx_rateindex;
>  
> -	ieee80211_tx_info_clear_status(tx_info);
> +	ath_clear_tx_status(tx_info);
>  
>  	if (txok)
>  		tx_info->status.ack_signal = ts->ts_rssi;
> @@ -2569,6 +2579,13 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  	tx_info->status.ampdu_len = nframes;
>  	tx_info->status.ampdu_ack_len = nframes - nbad;
>  
> +	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
> +
> +	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
> +		tx_info->status.rates[i].count = 0;
> +		tx_info->status.rates[i].idx = -1;
> +	}
> +
>  	if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
>  	    (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
>  		/*
> @@ -2590,13 +2607,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  			tx_info->status.rates[tx_rateindex].count =
>  				hw->max_rate_tries;
>  	}
> -
> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
> -		tx_info->status.rates[i].count = 0;
> -		tx_info->status.rates[i].idx = -1;
> -	}
> -
> -	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
>  }
>  
>  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)

Re: [PATCH for-5.18 v3] ath9k: Fix usage of driver-private space in tx_info

[Toke Høiland-Jørgensen]

Peter Seiderer <ps.report@gmx.net> writes:

> Hello Toke,
>
> On Mon,  4 Apr 2022 22:48:00 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> The ieee80211_tx_info_clear_status() helper also clears the rate counts and
>> the driver-private part of struct ieee80211_tx_info, so using it breaks
>> quite a few other things. So back out of using it, and instead define a
>> ath-internal helper that only clears the area between the
>> status_driver_data and the rates info. Combined with moving the
>> ath_frame_info struct to status_driver_data, this avoids clearing anything
>> we shouldn't be, and so we can keep the existing code for handling the rate
>> information.
>> 
>> While fixing this I also noticed that the setting of
>> tx_info->status.rates[tx_rateindex].count on hardware underrun errors was
>> always immediately overridden by the normal setting of the same fields, so
>> rearrange the code so that the underrun detection actually takes effect.
>> 
>> The new helper could be generalised to a 'memset_between()' helper, but
>> leave it as a driver-internal helper for now since this needs to go to
>> stable.
>> 
>> Cc: stable@vger.kernel.org
>> Reported-by: Peter Seiderer <ps.report@gmx.net>
>> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> And finally found time to test your latest version of the patch, you can add my
>
> Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> Tested-by: Peter Seiderer <ps.report@gmx.net>

Awesome! Thanks for testing! :)

-Toke

Re: [PATCH for-5.18 v3] ath9k: Fix usage of driver-private space in tx_info

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> The ieee80211_tx_info_clear_status() helper also clears the rate counts and
> the driver-private part of struct ieee80211_tx_info, so using it breaks
> quite a few other things. So back out of using it, and instead define a
> ath-internal helper that only clears the area between the
> status_driver_data and the rates info. Combined with moving the
> ath_frame_info struct to status_driver_data, this avoids clearing anything
> we shouldn't be, and so we can keep the existing code for handling the rate
> information.
> 
> While fixing this I also noticed that the setting of
> tx_info->status.rates[tx_rateindex].count on hardware underrun errors was
> always immediately overridden by the normal setting of the same fields, so
> rearrange the code so that the underrun detection actually takes effect.
> 
> The new helper could be generalised to a 'memset_between()' helper, but
> leave it as a driver-internal helper for now since this needs to go to
> stable.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Peter Seiderer <ps.report@gmx.net>
> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> Tested-by: Peter Seiderer <ps.report@gmx.net>

Patch applied to wireless.git, thanks.

5a6b06f5927c ath9k: Fix usage of driver-private space in tx_info

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220404204800.2681133-1-toke@toke.dk/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches