On Mon, Jan 14 2019 14:23:44 +0100, Stefan Sperling wrote:
> Thank you for tracking this problem down.
>
> Your diff is not correct. This part introduces a memory leak because
> the mbuf is not going to be freed anymore:
>
> > @@ -411,6 +412,12 @@ ieee80211_input(struct ifnet *ifp, struct mbuf *m,
> > struct ieee80211_node *ni,
> > /* protection is on for Rx */
> > if (!(rxi->rxi_flags & IEEE80211_RXI_HWDEC)) {
> > if (!(wh->i_fc[1] & IEEE80211_FC1_PROTECTED)) {
> > + /*
> > + * 9.2.4.1.9 frames without data are
> > + * not protected
> > + */
> > + if (!hasdata)
> > + return;
>
> This should say 'goto out' instead of 'return'.
thanks. This is why I placed the disclaimer in my mail, I didn't fully
understand what I was doing :) I just thought that since normally we
just return without an explicit free after decryption & decapsulation, I
should do the same thing; I assumed the caller would handle it. But I
guess not :)
> I'd like to propose a more general solution:
>
> The diff below improves naming of so far unused frame subtype constants
> and makes it more obvious which subtypes do not carry data, it attributes
> "no data" frames to a more suitable stat counter, and it drops them early.
>
> Index: ieee80211.h
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211.h,v
> retrieving revision 1.60
> diff -u -p -r1.60 ieee80211.h
> --- ieee80211.h 2 Jul 2017 14:48:19 -0000 1.60
> +++ ieee80211.h 14 Jan 2019 13:05:08 -0000
> @@ -140,13 +140,13 @@ struct ieee80211_htframe_addr4 { /* 11n
> #define IEEE80211_FC0_SUBTYPE_CF_END_ACK 0xf0
> /* for TYPE_DATA (bit combination) */
> #define IEEE80211_FC0_SUBTYPE_DATA 0x00
> -#define IEEE80211_FC0_SUBTYPE_CF_ACK 0x10
> -#define IEEE80211_FC0_SUBTYPE_CF_POLL 0x20
> -#define IEEE80211_FC0_SUBTYPE_CF_ACPL 0x30
> +#define IEEE80211_FC0_SUBTYPE_DATA_CF_ACK 0x10
> +#define IEEE80211_FC0_SUBTYPE_DATA_CF_POLL 0x20
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_POLL 0x30
> #define IEEE80211_FC0_SUBTYPE_NODATA 0x40
> -#define IEEE80211_FC0_SUBTYPE_CFACK 0x50
> -#define IEEE80211_FC0_SUBTYPE_CFPOLL 0x60
> -#define IEEE80211_FC0_SUBTYPE_CF_ACK_CF_ACK 0x70
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_NODATA 0x50
> +#define IEEE80211_FC0_SUBTYPE_CF_POLL_NODATA 0x60
> +#define IEEE80211_FC0_SUBTYPE_CF_ACK_CF_POLL 0x70
why doesn't SUBTYPE_CF_ACK_CF_POLL have NODATA in the name? it has the
NODATA bit set (ie. & 0x40), and "QoS CF-Ack + CF_Poll (no data)" is
explicitly listed in 9.2.4.1.9.
> #define IEEE80211_FC0_SUBTYPE_QOS 0x80
>
> #define IEEE80211_FC1_DIR_MASK 0x03
> Index: ieee80211_input.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.202
> diff -u -p -r1.202 ieee80211_input.c
> --- ieee80211_input.c 7 Aug 2018 18:13:14 -0000 1.202
> +++ ieee80211_input.c 14 Jan 2019 13:19:30 -0000
> @@ -202,6 +202,19 @@ ieee80211_input(struct ifnet *ifp, struc
> goto err;
> }
> }
> +
> + /*
> + * "no data" frames are used for various MAC coordination functions,
> + * particularly in the context of QoS. We do not implement related
> + * features yet so do not process "no data" frames any further.
> + */
> + if (subtype & (IEEE80211_FC0_SUBTYPE_NODATA |
> + IEEE80211_FC0_SUBTYPE_CF_POLL_NODATA |
> + IEEE80211_FC0_SUBTYPE_CF_ACK_NODATA)) {
1) shouldn't we first check that type is data here?
2) isn't this a false positive for subtype ==
IEEE80211_FC0_SUBTYPE_DATA_CF_ACK and subtype ==
IEEE80211_FC0_SUBTYPE_DATA_FC_POLL, since the _NODATA versions are just
the _DATA_ bits ORed with FC0_SUBTYPE_NODATA? I think we should either
check (subtype & IEEE80211_FC0_SUBTYPE_NODATA), or test subtype's
equality to each of the possible NODATA macros.
3) where is IEEE80211_FC0_SUBTYPE_CF_ACK_CF_POLL?
--
Lauri Tirkkonen | lotheac @ IRCnet
