Date: Fri, 16 May 2025 02:34:47 GMT From: Adrian Chadd <adrian@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: aa266fad5829 - main - net80211: refactor sequence number assignment code Message-ID: <202505160234.54G2YlcP026272@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by adrian: URL: https://cgit.FreeBSD.org/src/commit/?id=aa266fad58294763dc07b94b33b15318e134826b commit aa266fad58294763dc07b94b33b15318e134826b Author: Adrian Chadd <adrian@FreeBSD.org> AuthorDate: 2025-01-25 22:25:18 +0000 Commit: Adrian Chadd <adrian@FreeBSD.org> CommitDate: 2025-05-16 02:33:27 +0000 net80211: refactor sequence number assignment code Refactor out the sequence number assignment code for normal frames and beacons. Document the behaviour around fragments and packet lists. Right now this should be a no-op; there's some verification code in here to make sure that the TID selected by the existing code matches the TID populated in the passed in mbuf/frame. Locally tested: * rtwn(4), STA mode Differential Revision: https://reviews.freebsd.org/D49764 Reviewed by: bz --- sys/net80211/ieee80211_output.c | 159 +++++++++++++++++++++++++--------------- sys/net80211/ieee80211_proto.h | 4 + 2 files changed, 103 insertions(+), 60 deletions(-) diff --git a/sys/net80211/ieee80211_output.c b/sys/net80211/ieee80211_output.c index 1ebf7fa8723f..1f726f75b6c6 100644 --- a/sys/net80211/ieee80211_output.c +++ b/sys/net80211/ieee80211_output.c @@ -892,7 +892,6 @@ ieee80211_send_setup( struct ieee80211vap *vap = ni->ni_vap; struct ieee80211_tx_ampdu *tap; struct ieee80211_frame *wh = mtod(m, struct ieee80211_frame *); - ieee80211_seq seqno; IEEE80211_TX_LOCK_ASSERT(ni->ni_ic); @@ -975,25 +974,8 @@ ieee80211_send_setup( /* NB: zero out i_seq field (for s/w encryption etc) */ *(uint16_t *)&wh->i_seq[0] = 0; - } else { - if (IEEE80211_HAS_SEQ(type & IEEE80211_FC0_TYPE_MASK, - type & IEEE80211_FC0_SUBTYPE_MASK)) - /* - * 802.11-2012 9.3.2.10 - QoS multicast frames - * come out of a different seqno space. - */ - if (IEEE80211_IS_MULTICAST(wh->i_addr1)) { - seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; - } else { - seqno = ni->ni_txseqs[tid]++; - } - else - seqno = 0; - - *(uint16_t *)&wh->i_seq[0] = - htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); - M_SEQNO_SET(m, seqno); - } + } else + ieee80211_output_seqno_assign(ni, tid, m); if (IEEE80211_IS_MULTICAST(wh->i_addr1)) m->m_flags |= M_MCAST; @@ -1483,7 +1465,6 @@ ieee80211_encap(struct ieee80211vap *vap, struct ieee80211_node *ni, struct ieee80211_key *key; struct llc *llc; int hdrsize, hdrspace, datalen, addqos, txfrag, is4addr, is_mcast; - ieee80211_seq seqno; int meshhdrsize, meshae; uint8_t *qos; int is_amsdu = 0; @@ -1829,22 +1810,8 @@ ieee80211_encap(struct ieee80211vap *vap, struct ieee80211_node *ni, * and we don't need the TX lock held. */ if ((m->m_flags & M_AMPDU_MPDU) == 0) { - /* - * 802.11-2012 9.3.2.10 - - * - * If this is a multicast frame then we need - * to ensure that the sequence number comes from - * a separate seqno space and not the TID space. - * - * Otherwise multicast frames may actually cause - * holes in the TX blockack window space and - * upset various things. - */ - if (IEEE80211_IS_MULTICAST(wh->i_addr1)) - seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; - else - seqno = ni->ni_txseqs[tid]++; - + ieee80211_output_seqno_assign(ni, tid, m); + } else { /* * NB: don't assign a sequence # to potential * aggregates; we expect this happens at the @@ -1857,24 +1824,11 @@ ieee80211_encap(struct ieee80211vap *vap, struct ieee80211_node *ni, * capability; this may also change when we pull * aggregation up into net80211 */ - *(uint16_t *)wh->i_seq = - htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); - M_SEQNO_SET(m, seqno); - } else { /* NB: zero out i_seq field (for s/w encryption etc) */ *(uint16_t *)wh->i_seq = 0; } } else { - /* - * XXX TODO TX lock is needed for atomic updates of sequence - * numbers. If the driver does it, then don't do it here; - * and we don't need the TX lock held. - */ - seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; - *(uint16_t *)wh->i_seq = - htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); - M_SEQNO_SET(m, seqno); - + ieee80211_output_seqno_assign(ni, IEEE80211_NONQOS_TID, m); /* * XXX TODO: we shouldn't allow EAPOL, etc that would * be forced to be non-QoS traffic to be A-MSDU encapsulated. @@ -1975,7 +1929,6 @@ ieee80211_free_mbuf(struct mbuf *m) * This implements the fragmentation part of 802.11-2016 10.2.7 * (Fragmentation/defragmentation overview.) * - * Fragment the frame according to the specified mtu. * The size of the 802.11 header (w/o padding) is provided * so we don't need to recalculate it. We create a new * mbuf for each fragment and chain it through m_nextpkt; @@ -3827,8 +3780,6 @@ ieee80211_beacon_update(struct ieee80211_node *ni, struct mbuf *m, int mcast) struct ieee80211com *ic = ni->ni_ic; int len_changed = 0; uint16_t capinfo; - struct ieee80211_frame *wh; - ieee80211_seq seqno; IEEE80211_LOCK(ic); /* @@ -3896,8 +3847,6 @@ ieee80211_beacon_update(struct ieee80211_node *ni, struct mbuf *m, int mcast) return 1; /* just assume length changed */ } - wh = mtod(m, struct ieee80211_frame *); - /* * XXX TODO Strictly speaking this should be incremented with the TX * lock held so as to serialise access to the non-qos TID sequence @@ -3906,10 +3855,7 @@ ieee80211_beacon_update(struct ieee80211_node *ni, struct mbuf *m, int mcast) * If the driver identifies it does its own TX seqno management then * we can skip this (and still not do the TX seqno.) */ - seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; - *(uint16_t *)&wh->i_seq[0] = - htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); - M_SEQNO_SET(m, seqno); + ieee80211_output_beacon_seqno_assign(ni, m); /* XXX faster to recalculate entirely or just changes? */ capinfo = ieee80211_getcapinfo(vap, ni->ni_chan); @@ -4241,3 +4187,96 @@ ieee80211_tx_complete(struct ieee80211_node *ni, struct mbuf *m, int status) } m_freem(m); } + +/** + * @brief Assign a sequence number to the given frame. + * + * Check the frame type and TID and assign a suitable sequence number + * from the correct sequence number space. + * + * It assumes the mbuf has been encapsulated, and has the TID assigned + * if it is a QoS frame. + * + * Note this also clears any existing fragment ID in the header, so it + * must be called first before assigning fragment IDs. + * + * For now this implements parts of 802.11-2012; it doesn't do all of + * the needed checks for full compliance (notably QoS-Data NULL frames). + * + * TODO: update to 802.11-2020 10.3.2.14.2 (Transmitter Requirements) + * + * @param ni ieee80211_node this frame will be transmitted to + * @param arg_tid A temporary check, existing callers may set + * this to a TID variable they were using, and this routine + * will verify it against what's in the frame and complain if + * they don't match. For new callers, use -1. + * @param m mbuf to populate the sequence number into + */ +void +ieee80211_output_seqno_assign(struct ieee80211_node *ni, int arg_tid, + struct mbuf *m) +{ + struct ieee80211_frame *wh; + ieee80211_seq seqno; + uint8_t tid, type, subtype; + + wh = mtod(m, struct ieee80211_frame *); + tid = ieee80211_gettid(wh); + type = wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK; + subtype = wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_MASK; + + /* + * Find places where the passed in TID doesn't match gettid() + * and log. I'll have to then go and chase those down. + * + * If the caller knows its already setup the TID in the frame + * correctly then it can pass in -1 and this check will be + * skipped. + */ + if (arg_tid != -1 && tid != arg_tid) + ic_printf(ni->ni_vap->iv_ic, + "%s: called; TID mismatch; tid=%u, arg_tid=%d\n", + __func__, tid, arg_tid); + + if (IEEE80211_HAS_SEQ(type, subtype)) { + /* + * 802.11-2012 9.3.2.10 - QoS multicast frames + * come out of a different seqno space. + */ + if (IEEE80211_IS_MULTICAST(wh->i_addr1)) + seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; + else + seqno = ni->ni_txseqs[tid]++; + } else + seqno = 0; + + /* + * Assign the sequence number, clearing out any existing + * sequence and fragment numbers. + */ + *(uint16_t *)&wh->i_seq[0] = + htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); + M_SEQNO_SET(m, seqno); +} + +/** + * @brief Assign a sequence number to the given beacon frame. + * + * TODO: update to 802.11-2020 10.3.2.14.2 (Transmitter Requirements) + * + * @param ni ieee80211_node this frame will be transmitted to + * @param m mbuf to populate the sequence number into + */ +void +ieee80211_output_beacon_seqno_assign(struct ieee80211_node *ni, struct mbuf *m) +{ + struct ieee80211_frame *wh; + ieee80211_seq seqno; + + wh = mtod(m, struct ieee80211_frame *); + + seqno = ni->ni_txseqs[IEEE80211_NONQOS_TID]++; + *(uint16_t *)&wh->i_seq[0] = + htole16(seqno << IEEE80211_SEQ_SEQ_SHIFT); + M_SEQNO_SET(m, seqno); +} diff --git a/sys/net80211/ieee80211_proto.h b/sys/net80211/ieee80211_proto.h index 9ab7e644c89d..32745264b0ab 100644 --- a/sys/net80211/ieee80211_proto.h +++ b/sys/net80211/ieee80211_proto.h @@ -123,6 +123,10 @@ struct mbuf * ieee80211_ff_encap1(struct ieee80211vap *, struct mbuf *, const struct ether_header *); void ieee80211_tx_complete(struct ieee80211_node *, struct mbuf *, int); +void ieee80211_output_seqno_assign(struct ieee80211_node *, + int, struct mbuf *); +void ieee80211_output_beacon_seqno_assign(struct ieee80211_node *, + struct mbuf *); /* * The formation of ProbeResponse frames requires guidance to
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202505160234.54G2YlcP026272>