On Mon, Jan 14, 2019 at 04:10:36PM +0200, Lauri Tirkkonen wrote:
> 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.
Yes, that was a mistake. See this follow-up patch:
https://marc.info/?l=openbsd-tech&m=154747407319373&w=2
> > +
> > + /*
> > + * "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?
Indeed, my diff was bad as well. Thanks for spotting these issues.
I hadn't run this diff yet cause I was still building a new snapshot
to test it. Could you also test this new version please?
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:29:35 -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_DATA_CF_ACKPOLL 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_NODATA_CF_ACK 0x50
+#define IEEE80211_FC0_SUBTYPE_NODATA_CF_POLL 0x60
+#define IEEE80211_FC0_SUBTYPE_NODATA_CF_ACKPOLL 0x70
#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 14:29:27 -0000
@@ -202,6 +202,18 @@ 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 (type == IEEE80211_FC0_TYPE_DATA &&
+ (subtype & IEEE80211_FC0_SUBTYPE_NODATA)) {
+ ic->ic_stats.is_rx_badsubtype++;
+ goto out;
+ }
+
if ((hasqos = ieee80211_has_qos(wh))) {
qos = ieee80211_get_qos(wh);
tid = qos & IEEE80211_QOS_TID;
@@ -211,7 +223,6 @@ ieee80211_input(struct ifnet *ifp, struc
}
if (type == IEEE80211_FC0_TYPE_DATA && hasqos &&
- (subtype & IEEE80211_FC0_SUBTYPE_NODATA) == 0 &&
!(rxi->rxi_flags & IEEE80211_RXI_AMPDU_DONE)) {
int ba_state = ni->ni_rx_ba[tid].ba_state;
