| Message ID | 20191105164932.11799-1-linus.luessing@c0d3.blue (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [net-next] ath10k: fix RX of frames with broken FCS in monitor mode | expand |
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, >
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 >
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
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,