| Message ID | 20200628220512.28535ebc@mathy-work.localhost (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: keep non-zero sequence counter of injected frames | expand |
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
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
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
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
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 >
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
> 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.
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)))
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(-)