| Message ID | 20191101111138.9086-1-linus.luessing@c0d3.blue (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ath10k: fix RX of frames with broken FCS in monitor mode | expand |
On 11/1/19 4:11 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. > > 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. > > I'm also not that familiar with the ath10k code yet. So not 100% sure if > it's the right place for this check. Therefore sending as RFC. > --- > drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c > index 53f1095de8ff..ce0a16ebb8bb 100644 > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c > @@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb) > > status = IEEE80211_SKB_RXCB(skb); > > + if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) && > + status->flag & RX_FLAG_FAILED_FCS_CRC) { > + dev_kfree_skb_any(skb); > + return; > + } Maybe worth adding a counter like 'rx_drop_crc' to the ath10k_debug struct and increment it here and also show in debugfs and/or ethtool stats? And, maybe no check for ar->monitor, in case somehow the frame is still received with bad CRC even without monitor mode? Thanks, Ben
On Fri, Nov 01, 2019 at 08:46:53AM -0700, Ben Greear wrote: > > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c > > index 53f1095de8ff..ce0a16ebb8bb 100644 > > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c > > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c > > @@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb) > > status = IEEE80211_SKB_RXCB(skb); > > + if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) && > > + status->flag & RX_FLAG_FAILED_FCS_CRC) { > > + dev_kfree_skb_any(skb); > > + return; > > + } > > Maybe worth adding a counter like 'rx_drop_crc' to the ath10k_debug struct and increment it here > and also show in debugfs and/or ethtool stats? Ok. > > And, maybe no check for ar->monitor, in case somehow the frame is still received > with bad CRC even without monitor mode? I think ath9k is also checking for "ah->is_monitoring" here: https://elixir.bootlin.com/linux/v5.3.8/source/drivers/net/wireless/ath/ath9k/common.c#L96 And I didn't want to divert from this. Should I remove it anyway?
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c index 53f1095de8ff..ce0a16ebb8bb 100644 --- a/drivers/net/wireless/ath/ath10k/htt_rx.c +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c @@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb) status = IEEE80211_SKB_RXCB(skb); + if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) && + status->flag & RX_FLAG_FAILED_FCS_CRC) { + 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,