[PATCH net-next] ath10k: fix RX of frames with broken FCS in monitor mode

[PATCH net-next] ath10k: fix RX of frames with broken FCS in monitor mode

[Linus Lüssing]

From: Linus Lüssing <ll@simonwunderlich.de>

So far, frames were forwarded regardless of the FCS correctness leading
to userspace applications listening on the monitor mode interface to
receive potentially broken frames, even with the "fcsfail" flag unset.

By default, with the "fcsfail" flag of a monitor mode interface
unset, frames with FCS errors should be dropped. With this patch, the
fcsfail flag is taken into account correctly.

Cc: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Linus Lüssing <ll@simonwunderlich.de>
---
This was tested on an Open Mesh A41 device, featuring a QCA4019. And
with this firmware:

https://www.candelatech.com/downloads/ath10k-4019-10-4b/firmware-5-ct-full-community-12.bin-lede.011

But from looking at the code it seems that the vanilla ath10k has the
same issue, therefore submitting it here.

Changelog RFC->v1:

* removed "ar->monitor" check
* added a debug counter

---
 drivers/net/wireless/ath/ath10k/core.h   |  1 +
 drivers/net/wireless/ath/ath10k/debug.c  |  2 ++
 drivers/net/wireless/ath/ath10k/htt_rx.c | 10 ++++++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4d7db07db6ba..787065a9eb3f 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -1171,6 +1171,7 @@ struct ath10k {
 
 	struct {
 		/* protected by data_lock */
+		u32 rx_crc_err_drop;
 		u32 fw_crash_counter;
 		u32 fw_warm_reset_counter;
 		u32 fw_cold_reset_counter;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index bd2b5628f850..5e4cd2966e6f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1094,6 +1094,7 @@ static const char ath10k_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"d_rts_good",
 	"d_tx_power", /* in .5 dbM I think */
 	"d_rx_crc_err", /* fcs_bad */
+	"d_rx_crc_err_drop", /* frame with FCS error, dropped late in kernel */
 	"d_no_beacon",
 	"d_tx_mpdus_queued",
 	"d_tx_msdu_queued",
@@ -1193,6 +1194,7 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 	data[i++] = pdev_stats->rts_good;
 	data[i++] = pdev_stats->chan_tx_power;
 	data[i++] = pdev_stats->fcs_bad;
+	data[i++] = ar->stats.rx_crc_err_drop;
 	data[i++] = pdev_stats->no_beacons;
 	data[i++] = pdev_stats->mpdu_enqued;
 	data[i++] = pdev_stats->msdu_enqued;
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 53f1095de8ff..149728588e80 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1285,6 +1285,16 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb)
 
 	status = IEEE80211_SKB_RXCB(skb);
 
+	if (!(ar->filter_flags & FIF_FCSFAIL) &&
+	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
+		spin_lock_bh(&ar->data_lock);
+		ar->stats.rx_crc_err_drop++;
+		spin_unlock_bh(&ar->data_lock);
+
+		dev_kfree_skb_any(skb);
+		return;
+	}
+
 	ath10k_dbg(ar, ATH10K_DBG_DATA,
 		   "rx skb %pK len %u peer %pM %s %s sn %u %s%s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
 		   skb,
-- 
2.24.0.rc2

Re: [PATCH net-next] ath10k: fix RX of frames with broken FCS in monitor mode

[Ben Greear]

On 11/5/19 8:49 AM, Linus Lüssing wrote:
> From: Linus Lüssing <ll@simonwunderlich.de>
> 
> So far, frames were forwarded regardless of the FCS correctness leading
> to userspace applications listening on the monitor mode interface to
> receive potentially broken frames, even with the "fcsfail" flag unset.
> 
> By default, with the "fcsfail" flag of a monitor mode interface
> unset, frames with FCS errors should be dropped. With this patch, the
> fcsfail flag is taken into account correctly.
> 
> Cc: Simon Wunderlich <sw@simonwunderlich.de>
> Signed-off-by: Linus Lüssing <ll@simonwunderlich.de>
> ---
> This was tested on an Open Mesh A41 device, featuring a QCA4019. And
> with this firmware:
> 
> https://www.candelatech.com/downloads/ath10k-4019-10-4b/firmware-5-ct-full-community-12.bin-lede.011
> 
> But from looking at the code it seems that the vanilla ath10k has the
> same issue, therefore submitting it here.
> 
> Changelog RFC->v1:
> 
> * removed "ar->monitor" check
> * added a debug counter

Thanks for adding the counter.  Since it us u32, I doubt you need the spin lock
below?

--Ben

> +	if (!(ar->filter_flags & FIF_FCSFAIL) &&
> +	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
> +		spin_lock_bh(&ar->data_lock);
> +		ar->stats.rx_crc_err_drop++;
> +		spin_unlock_bh(&ar->data_lock);
> +
> +		dev_kfree_skb_any(skb);
> +		return;
> +	}
> +
>   	ath10k_dbg(ar, ATH10K_DBG_DATA,
>   		   "rx skb %pK len %u peer %pM %s %s sn %u %s%s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
>   		   skb,
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH net-next] ath10k: fix RX of frames with broken FCS in monitor mode

[Linus Lüssing]

On Tue, Nov 05, 2019 at 09:19:20AM -0800, Ben Greear wrote:
> Thanks for adding the counter.  Since it us u32, I doubt you need the spin lock
> below?

Ok, I can remove the spin-lock.

Just for clarification though, if I recall correctly then an increment operator
is not guaranteed to work atomically. But you think it's unlikely
to race with a concurrent ++ and therefore it's fine for just a debug counter?
(and if it were racing, it'd just be a missed +1)

Or is there another mechanism that avoids concurrency in the
ath10k RX path?


> 
> --Ben
> 
> > +	if (!(ar->filter_flags & FIF_FCSFAIL) &&
> > +	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
> > +		spin_lock_bh(&ar->data_lock);
> > +		ar->stats.rx_crc_err_drop++;
> > +		spin_unlock_bh(&ar->data_lock);
> > +
> > +		dev_kfree_skb_any(skb);
> > +		return;
> > +	}
> > +
> >   	ath10k_dbg(ar, ATH10K_DBG_DATA,
> >   		   "rx skb %pK len %u peer %pM %s %s sn %u %s%s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
> >   		   skb,
> > 
> 
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
> 

Re: [PATCH net-next] ath10k: fix RX of frames with broken FCS in monitor mode

[Ben Greear]

On 11/07/2019 06:03 AM, Linus Lüssing wrote:
> On Tue, Nov 05, 2019 at 09:19:20AM -0800, Ben Greear wrote:
>> Thanks for adding the counter.  Since it us u32, I doubt you need the spin lock
>> below?
>
> Ok, I can remove the spin-lock.
>
> Just for clarification though, if I recall correctly then an increment operator
> is not guaranteed to work atomically. But you think it's unlikely
> to race with a concurrent ++ and therefore it's fine for just a debug counter?
> (and if it were racing, it'd just be a missed +1)

I think it is fine to be off-by-one, and u32 is atomic so you would never read a really
weird number, like you can if u64 is non-atomically being incremented.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com