[PATCH] mac80211: keep non-zero sequence counter of injected frames

[PATCH] mac80211: keep non-zero sequence counter of injected frames

[Mathy Vanhoef]

The sequence number of injected frames is being overwritten by the
function ieee80211_tx_h_sequence when the following two conditions
are met:

1. The frame is injected on a virtual interface, and a second virtual
   interface on this device is operating in managed/AP/.. mode.

2. The sender MAC address of the injected frame matches the MAC
   address of the second interface operating in managed/AP/.. mode.

In some cases this may be desired, for instance when hostap is
configured to send certain frames using a monitor interface, in which
case the user-space will not assign a sequence number and instead
injects frames with a sequence number of zero.

However, in case the user-space does assign a non-zero sequence
number, this number should not be overwritten by the kernel. This
patch adds a check to see if injected frames have already been assigned
a non-zero sequence number, and if so, this sequence number will not
be overwritten by the kernel.

Signed-off-by: Mathy Vanhoef <mathy.vanhoef@nyu.edu>
---
 net/mac80211/tx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1a2941e52..8bb8c548c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -808,11 +808,14 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 	int tid;
 
 	/*
-	 * Packet injection may want to control the sequence
-	 * number, if we have no matching interface then we
-	 * neither assign one ourselves nor ask the driver to.
+	 * Packet injection may want to control the sequence number.
+	 * Do not assign one ourselves, and do not ask the driver to,
+	 * if there is no matching interface or if the injected frame
+	 * was already assigned a non-zero sequence number.
 	 */
-	if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR))
+	if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
+		     (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
+		      hdr->seq_ctrl != 0)))
 		return TX_CONTINUE;
 
 	if (unlikely(ieee80211_is_ctl(hdr->frame_control)))
-- 
2.20.1

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[Johannes Berg]

On Sun, 2020-06-28 at 22:05 +0400, Mathy Vanhoef wrote:
> The sequence number of injected frames is being overwritten by the
> function ieee80211_tx_h_sequence when the following two conditions
> are met:
> 
> 1. The frame is injected on a virtual interface, and a second virtual
>    interface on this device is operating in managed/AP/.. mode.
> 
> 2. The sender MAC address of the injected frame matches the MAC
>    address of the second interface operating in managed/AP/.. mode.
> 
> In some cases this may be desired, for instance when hostap is
> configured to send certain frames using a monitor interface, in which
> case the user-space will not assign a sequence number and instead
> injects frames with a sequence number of zero.

Yeah, this is where that used to be used. I'm thinking we should "break"
this stuff eventually, tell people to use modern hostapd versions, and
see who cares ... because all this "cooked monitor" etc. is all quite
ugly.

> However, in case the user-space does assign a non-zero sequence
> number, this number should not be overwritten by the kernel. This
> patch adds a check to see if injected frames have already been assigned
> a non-zero sequence number, and if so, this sequence number will not
> be overwritten by the kernel.

Seems reasonable, but I have to wonder what you're up to now ;-)

Anyway, I'll apply this unless I find some tests fail or something, but
I'll be going on vacation soon, so it'll be a while (since it's for the
-next tree as a feature).

johannes

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4335 bytes --]

Hi Mathy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mac80211-next/master]
[also build test WARNING on mac80211/master v5.8-rc3 next-20200626]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mathy-Vanhoef/mac80211-keep-non-zero-sequence-counter-of-injected-frames/20200629-021517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from net/mac80211/tx.c:13:
   net/mac80211/tx.c: In function 'ieee80211_tx_h_sequence':
>> net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^

vim +817 net/mac80211/tx.c

   802	
   803	static ieee80211_tx_result debug_noinline
   804	ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
   805	{
   806		struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
   807		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
   808		int tid;
   809	
   810		/*
   811		 * Packet injection may want to control the sequence number.
   812		 * Do not assign one ourselves, and do not ask the driver to,
   813		 * if there is no matching interface or if the injected frame
   814		 * was already assigned a non-zero sequence number.
   815		 */
   816		if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
 > 817			     (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
   818			      hdr->seq_ctrl != 0)))
   819			return TX_CONTINUE;
   820	
   821		if (unlikely(ieee80211_is_ctl(hdr->frame_control)))
   822			return TX_CONTINUE;
   823	
   824		if (ieee80211_hdrlen(hdr->frame_control) < 24)
   825			return TX_CONTINUE;
   826	
   827		if (ieee80211_is_qos_nullfunc(hdr->frame_control))
   828			return TX_CONTINUE;
   829	
   830		/*
   831		 * Anything but QoS data that has a sequence number field
   832		 * (is long enough) gets a sequence number from the global
   833		 * counter.  QoS data frames with a multicast destination
   834		 * also use the global counter (802.11-2012 9.3.2.10).
   835		 */
   836		if (!ieee80211_is_data_qos(hdr->frame_control) ||
   837		    is_multicast_ether_addr(hdr->addr1)) {
   838			if (tx->flags & IEEE80211_TX_NO_SEQNO)
   839				return TX_CONTINUE;
   840			/* driver should assign sequence number */
   841			info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
   842			/* for pure STA mode without beacons, we can do it */
   843			hdr->seq_ctrl = cpu_to_le16(tx->sdata->sequence_number);
   844			tx->sdata->sequence_number += 0x10;
   845			if (tx->sta)
   846				tx->sta->tx_stats.msdu[IEEE80211_NUM_TIDS]++;
   847			return TX_CONTINUE;
   848		}
   849	
   850		/*
   851		 * This should be true for injected/management frames only, for
   852		 * management frames we have set the IEEE80211_TX_CTL_ASSIGN_SEQ
   853		 * above since they are not QoS-data frames.
   854		 */
   855		if (!tx->sta)
   856			return TX_CONTINUE;
   857	
   858		/* include per-STA, per-TID sequence counter */
   859		tid = ieee80211_get_tid(hdr);
   860		tx->sta->tx_stats.msdu[tid]++;
   861	
   862		hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
   863	
   864		return TX_CONTINUE;
   865	}
   866	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62154 bytes --]

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5250 bytes --]

Hi Mathy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mac80211-next/master]
[also build test WARNING on mac80211/master v5.8-rc3 next-20200629]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mathy-Vanhoef/mac80211-keep-non-zero-sequence-counter-of-injected-frames/20200629-021517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a28d38a6bca1726d56c9b373f4c7dc5264fc7716)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/mac80211/tx.c:817:21: warning: & has lower precedence than !=; != will be evaluated first [-Wparentheses]
                        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                               ^
   net/mac80211/tx.c:817:21: note: place parentheses around the '!=' expression to silence this warning
                        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
                                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                               ^
   net/mac80211/tx.c:817:21: note: place parentheses around the & expression to evaluate it first
                        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
                         ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                               ^
   1 warning generated.

vim +817 net/mac80211/tx.c

   802	
   803	static ieee80211_tx_result debug_noinline
   804	ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
   805	{
   806		struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
   807		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
   808		int tid;
   809	
   810		/*
   811		 * Packet injection may want to control the sequence number.
   812		 * Do not assign one ourselves, and do not ask the driver to,
   813		 * if there is no matching interface or if the injected frame
   814		 * was already assigned a non-zero sequence number.
   815		 */
   816		if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
 > 817			     (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
   818			      hdr->seq_ctrl != 0)))
   819			return TX_CONTINUE;
   820	
   821		if (unlikely(ieee80211_is_ctl(hdr->frame_control)))
   822			return TX_CONTINUE;
   823	
   824		if (ieee80211_hdrlen(hdr->frame_control) < 24)
   825			return TX_CONTINUE;
   826	
   827		if (ieee80211_is_qos_nullfunc(hdr->frame_control))
   828			return TX_CONTINUE;
   829	
   830		/*
   831		 * Anything but QoS data that has a sequence number field
   832		 * (is long enough) gets a sequence number from the global
   833		 * counter.  QoS data frames with a multicast destination
   834		 * also use the global counter (802.11-2012 9.3.2.10).
   835		 */
   836		if (!ieee80211_is_data_qos(hdr->frame_control) ||
   837		    is_multicast_ether_addr(hdr->addr1)) {
   838			if (tx->flags & IEEE80211_TX_NO_SEQNO)
   839				return TX_CONTINUE;
   840			/* driver should assign sequence number */
   841			info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
   842			/* for pure STA mode without beacons, we can do it */
   843			hdr->seq_ctrl = cpu_to_le16(tx->sdata->sequence_number);
   844			tx->sdata->sequence_number += 0x10;
   845			if (tx->sta)
   846				tx->sta->tx_stats.msdu[IEEE80211_NUM_TIDS]++;
   847			return TX_CONTINUE;
   848		}
   849	
   850		/*
   851		 * This should be true for injected/management frames only, for
   852		 * management frames we have set the IEEE80211_TX_CTL_ASSIGN_SEQ
   853		 * above since they are not QoS-data frames.
   854		 */
   855		if (!tx->sta)
   856			return TX_CONTINUE;
   857	
   858		/* include per-STA, per-TID sequence counter */
   859		tid = ieee80211_get_tid(hdr);
   860		tx->sta->tx_stats.msdu[tid]++;
   861	
   862		hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
   863	
   864		return TX_CONTINUE;
   865	}
   866	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74480 bytes --]

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10172 bytes --]

Hi Mathy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mac80211-next/master]
[also build test WARNING on mac80211/master v5.8-rc3 next-20200629]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mathy-Vanhoef/mac80211-keep-non-zero-sequence-counter-of-injected-frames/20200629-021517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: riscv-randconfig-r013-20200629 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from net/mac80211/tx.c:13:
   net/mac80211/tx.c: In function 'ieee80211_tx_h_sequence':
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
      69 |  (cond) ?     \
         |   ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
      56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                            ^~~~~~~~~~~~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~
   net/mac80211/tx.c:817:21: warning: suggest parentheses around comparison in operand of '&' [-Wparentheses]
     817 |        (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
         |                     ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
      69 |  (cond) ?     \
         |   ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
      56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                            ^~~~~~~~~~~~~~
   net/mac80211/tx.c:816:2: note: in expansion of macro 'if'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |  ^~
   include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
      48 | #  define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
         |                        ^~~~~~~~~~~~~~~~
>> net/mac80211/tx.c:816:6: note: in expansion of macro 'unlikely'
     816 |  if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
         |      ^~~~~~~~

vim +/unlikely +816 net/mac80211/tx.c

   802	
   803	static ieee80211_tx_result debug_noinline
   804	ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
   805	{
   806		struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
   807		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
   808		int tid;
   809	
   810		/*
   811		 * Packet injection may want to control the sequence number.
   812		 * Do not assign one ourselves, and do not ask the driver to,
   813		 * if there is no matching interface or if the injected frame
   814		 * was already assigned a non-zero sequence number.
   815		 */
 > 816		if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
   817			     (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
   818			      hdr->seq_ctrl != 0)))
   819			return TX_CONTINUE;
   820	
   821		if (unlikely(ieee80211_is_ctl(hdr->frame_control)))
   822			return TX_CONTINUE;
   823	
   824		if (ieee80211_hdrlen(hdr->frame_control) < 24)
   825			return TX_CONTINUE;
   826	
   827		if (ieee80211_is_qos_nullfunc(hdr->frame_control))
   828			return TX_CONTINUE;
   829	
   830		/*
   831		 * Anything but QoS data that has a sequence number field
   832		 * (is long enough) gets a sequence number from the global
   833		 * counter.  QoS data frames with a multicast destination
   834		 * also use the global counter (802.11-2012 9.3.2.10).
   835		 */
   836		if (!ieee80211_is_data_qos(hdr->frame_control) ||
   837		    is_multicast_ether_addr(hdr->addr1)) {
   838			if (tx->flags & IEEE80211_TX_NO_SEQNO)
   839				return TX_CONTINUE;
   840			/* driver should assign sequence number */
   841			info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
   842			/* for pure STA mode without beacons, we can do it */
   843			hdr->seq_ctrl = cpu_to_le16(tx->sdata->sequence_number);
   844			tx->sdata->sequence_number += 0x10;
   845			if (tx->sta)
   846				tx->sta->tx_stats.msdu[IEEE80211_NUM_TIDS]++;
   847			return TX_CONTINUE;
   848		}
   849	
   850		/*
   851		 * This should be true for injected/management frames only, for
   852		 * management frames we have set the IEEE80211_TX_CTL_ASSIGN_SEQ
   853		 * above since they are not QoS-data frames.
   854		 */
   855		if (!tx->sta)
   856			return TX_CONTINUE;
   857	
   858		/* include per-STA, per-TID sequence counter */
   859		tid = ieee80211_get_tid(hdr);
   860		tx->sta->tx_stats.msdu[tid]++;
   861	
   862		hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
   863	
   864		return TX_CONTINUE;
   865	}
   866	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32313 bytes --]

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[Mathy Vanhoef]

The kernel test robot highlighted a bug in the patch. So for now do not
apply this one.

I'm currently testing the injection behavior of some new devices I
have, since my current ones are getting old, and I'll send updated
patches within a week or two if all goes well.

On Sun, 28 Jun 2020 20:59:56 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Sun, 2020-06-28 at 22:05 +0400, Mathy Vanhoef wrote:
> > The sequence number of injected frames is being overwritten by the
> > function ieee80211_tx_h_sequence when the following two conditions
> > are met:
> > 
> > 1. The frame is injected on a virtual interface, and a second
> > virtual interface on this device is operating in managed/AP/.. mode.
> > 
> > 2. The sender MAC address of the injected frame matches the MAC
> >    address of the second interface operating in managed/AP/.. mode.
> > 
> > In some cases this may be desired, for instance when hostap is
> > configured to send certain frames using a monitor interface, in
> > which case the user-space will not assign a sequence number and
> > instead injects frames with a sequence number of zero.  
> 
> Yeah, this is where that used to be used. I'm thinking we should
> "break" this stuff eventually, tell people to use modern hostapd
> versions, and see who cares ... because all this "cooked monitor"
> etc. is all quite ugly.
> 
> > However, in case the user-space does assign a non-zero sequence
> > number, this number should not be overwritten by the kernel. This
> > patch adds a check to see if injected frames have already been
> > assigned a non-zero sequence number, and if so, this sequence
> > number will not be overwritten by the kernel.  
> 
> Seems reasonable, but I have to wonder what you're up to now ;-)
> 
> Anyway, I'll apply this unless I find some tests fail or something,
> but I'll be going on vacation soon, so it'll be a while (since it's
> for the -next tree as a feature).
> 
> johannes
> 

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 6767 bytes --]

Hi Mathy,

url:    https://github.com/0day-ci/linux/commits/Mathy-Vanhoef/mac80211-keep-non-zero-sequence-counter-of-injected-frames/20200629-021517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: arm-randconfig-m031-20200701 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
net/mac80211/tx.c:816 ieee80211_tx_h_sequence() warn: compare has higher precedence than mask
net/mac80211/tx.c:816 ieee80211_tx_h_sequence() warn: add some parenthesis here?

Old smatch warnings:
net/mac80211/tx.c:1831 invoke_tx_handlers_late() warn: variable dereferenced before check 'tx->skb' (see line 1809)
net/mac80211/tx.c:3426 ieee80211_xmit_fast_finish() error: we previously assumed 'key' could be null (see line 3394)

# https://github.com/0day-ci/linux/commit/f452608b92bbd4fff530a8f234d4e909287d249c
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f452608b92bbd4fff530a8f234d4e909287d249c
vim +816 net/mac80211/tx.c

f591fa5dbbbeae Johannes Berg 2008-07-10  803  static ieee80211_tx_result debug_noinline
f591fa5dbbbeae Johannes Berg 2008-07-10  804  ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
f591fa5dbbbeae Johannes Berg 2008-07-10  805  {
f591fa5dbbbeae Johannes Berg 2008-07-10  806  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
f591fa5dbbbeae Johannes Berg 2008-07-10  807  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
f591fa5dbbbeae Johannes Berg 2008-07-10  808  	int tid;
f591fa5dbbbeae Johannes Berg 2008-07-10  809  
25d834e16294c8 Johannes Berg 2008-09-12  810  	/*
f452608b92bbd4 Mathy Vanhoef 2020-06-28  811  	 * Packet injection may want to control the sequence number.
f452608b92bbd4 Mathy Vanhoef 2020-06-28  812  	 * Do not assign one ourselves, and do not ask the driver to,
f452608b92bbd4 Mathy Vanhoef 2020-06-28  813  	 * if there is no matching interface or if the injected frame
f452608b92bbd4 Mathy Vanhoef 2020-06-28  814  	 * was already assigned a non-zero sequence number.
25d834e16294c8 Johannes Berg 2008-09-12  815  	 */
f452608b92bbd4 Mathy Vanhoef 2020-06-28 @816  	if (unlikely(info->control.vif->type == NL80211_IFTYPE_MONITOR ||
f452608b92bbd4 Mathy Vanhoef 2020-06-28  817  		     (info->flags & IEEE80211_TX_CTL_INJECTED != 0 &&
                                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is wrong.  Also != 0 doesn't add anything.  It should just be:

		(info->flags & IEEE80211_TX_CTL_INJECTED) &&

The times when it's right to talk about == 0 and != 0 are when you are
talking about numbers as in "if (len == 0) " or "if (cnt != 0)" and when
you are using cmp() functions.  "if (strcmp(foo, bar) < 0)" means
foo is less than bar.  "if (strcmp(foo, bar) == 0)" means foo == bar.

f452608b92bbd4 Mathy Vanhoef 2020-06-28  818  		      hdr->seq_ctrl != 0)))
25d834e16294c8 Johannes Berg 2008-09-12  819  		return TX_CONTINUE;
25d834e16294c8 Johannes Berg 2008-09-12  820  
f591fa5dbbbeae Johannes Berg 2008-07-10  821  	if (unlikely(ieee80211_is_ctl(hdr->frame_control)))
f591fa5dbbbeae Johannes Berg 2008-07-10  822  		return TX_CONTINUE;
f591fa5dbbbeae Johannes Berg 2008-07-10  823  
f591fa5dbbbeae Johannes Berg 2008-07-10  824  	if (ieee80211_hdrlen(hdr->frame_control) < 24)
f591fa5dbbbeae Johannes Berg 2008-07-10  825  		return TX_CONTINUE;
f591fa5dbbbeae Johannes Berg 2008-07-10  826  
49a59543eb5a5d Johannes Berg 2011-09-29  827  	if (ieee80211_is_qos_nullfunc(hdr->frame_control))
49a59543eb5a5d Johannes Berg 2011-09-29  828  		return TX_CONTINUE;
49a59543eb5a5d Johannes Berg 2011-09-29  829  
94778280fabdb6 Johannes Berg 2008-10-10  830  	/*
94778280fabdb6 Johannes Berg 2008-10-10  831  	 * Anything but QoS data that has a sequence number field
94778280fabdb6 Johannes Berg 2008-10-10  832  	 * (is long enough) gets a sequence number from the global
c4c205f3cd17b5 Bob Copeland  2013-08-23  833  	 * counter.  QoS data frames with a multicast destination
c4c205f3cd17b5 Bob Copeland  2013-08-23  834  	 * also use the global counter (802.11-2012 9.3.2.10).
94778280fabdb6 Johannes Berg 2008-10-10  835  	 */
c4c205f3cd17b5 Bob Copeland  2013-08-23  836  	if (!ieee80211_is_data_qos(hdr->frame_control) ||
c4c205f3cd17b5 Bob Copeland  2013-08-23  837  	    is_multicast_ether_addr(hdr->addr1)) {
b9771d41aee7aa Johannes Berg 2018-05-28  838  		if (tx->flags & IEEE80211_TX_NO_SEQNO)
b9771d41aee7aa Johannes Berg 2018-05-28  839  			return TX_CONTINUE;
94778280fabdb6 Johannes Berg 2008-10-10  840  		/* driver should assign sequence number */
f591fa5dbbbeae Johannes Berg 2008-07-10  841  		info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
94778280fabdb6 Johannes Berg 2008-10-10  842  		/* for pure STA mode without beacons, we can do it */
94778280fabdb6 Johannes Berg 2008-10-10  843  		hdr->seq_ctrl = cpu_to_le16(tx->sdata->sequence_number);
94778280fabdb6 Johannes Berg 2008-10-10  844  		tx->sdata->sequence_number += 0x10;
79c892b85027d5 Johannes Berg 2014-11-21  845  		if (tx->sta)
e5a9f8d04660da Johannes Berg 2015-10-16  846  			tx->sta->tx_stats.msdu[IEEE80211_NUM_TIDS]++;
f591fa5dbbbeae Johannes Berg 2008-07-10  847  		return TX_CONTINUE;
f591fa5dbbbeae Johannes Berg 2008-07-10  848  	}
f591fa5dbbbeae Johannes Berg 2008-07-10  849  
f591fa5dbbbeae Johannes Berg 2008-07-10  850  	/*
f591fa5dbbbeae Johannes Berg 2008-07-10  851  	 * This should be true for injected/management frames only, for
f591fa5dbbbeae Johannes Berg 2008-07-10  852  	 * management frames we have set the IEEE80211_TX_CTL_ASSIGN_SEQ
f591fa5dbbbeae Johannes Berg 2008-07-10  853  	 * above since they are not QoS-data frames.
f591fa5dbbbeae Johannes Berg 2008-07-10  854  	 */
f591fa5dbbbeae Johannes Berg 2008-07-10  855  	if (!tx->sta)
f591fa5dbbbeae Johannes Berg 2008-07-10  856  		return TX_CONTINUE;
f591fa5dbbbeae Johannes Berg 2008-07-10  857  
f591fa5dbbbeae Johannes Berg 2008-07-10  858  	/* include per-STA, per-TID sequence counter */
a1f2ba04cc9241 Sara Sharon   2018-02-19  859  	tid = ieee80211_get_tid(hdr);
e5a9f8d04660da Johannes Berg 2015-10-16  860  	tx->sta->tx_stats.msdu[tid]++;
f591fa5dbbbeae Johannes Berg 2008-07-10  861  
ba8c3d6f16a1f9 Felix Fietkau 2015-03-27  862  	hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
f591fa5dbbbeae Johannes Berg 2008-07-10  863  
f591fa5dbbbeae Johannes Berg 2008-07-10  864  	return TX_CONTINUE;
f591fa5dbbbeae Johannes Berg 2008-07-10  865  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27196 bytes --]

Re: [PATCH] mac80211: keep non-zero sequence counter of injected frames

[Mathy Vanhoef]

> This is wrong.  Also != 0 doesn't add anything.  It should just be:

Yup, the disadvantage of late-night work. Hence also my follow-up mail
to drop the patch, will send updated patches at a later time.