blob: 0daf233159fca2b6407edf3105476f9debadce9a [file] [log] [blame]
developer7f77d022022-06-16 14:15:52 +08001From git@z Thu Jan 1 00:00:00 1970
2Subject: [PATCH v2] net: fix premature exit from NAPI state polling in napi_disable()
3From: Alexander Lobakin <alexandr.lobakin@intel.com>
4Date: Wed, 10 Nov 2021 20:56:05 +0100
5Message-Id: <20211110195605.1304-1-alexandr.lobakin@intel.com>
6To: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>
7Cc: Alexander Lobakin <alexandr.lobakin@intel.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Michal Swiatkowski <michal.swiatkowski@intel.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, Antoine Tenart <atenart@kernel.org>, Eric Dumazet <edumazet@google.com>, Wei Wang <weiwan@google.com>,Björn Töpel <bjorn@kernel.org>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
8List-Id: <linux-kernel.vger.kernel.org>
9MIME-Version: 1.0
10Content-Type: text/plain; charset="utf-8"
11Content-Transfer-Encoding: 7bit
12
13Commit 719c57197010 ("net: make napi_disable() symmetric with
14enable") accidentally introduced a bug sometimes leading to a kernel
15BUG when bringing an iface up/down under heavy traffic load.
16
17Prior to this commit, napi_disable() was polling n->state until
18none of (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC) is set and then
19always flip them. Now there's a possibility to get away with the
20NAPIF_STATE_SCHE unset as 'continue' drops us to the cmpxchg()
21call with an unitialized variable, rather than straight to
22another round of the state check.
23
24Error path looks like:
25
26napi_disable():
27unsigned long val, new; /* new is uninitialized */
28
29do {
30 val = READ_ONCE(n->state); /* NAPIF_STATE_NPSVC and/or
31 NAPIF_STATE_SCHED is set */
32 if (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) { /* true */
33 usleep_range(20, 200);
34 continue; /* go straight to the condition check */
35 }
36 new = val | <...>
37} while (cmpxchg(&n->state, val, new) != val); /* state == val, cmpxchg()
38 writes garbage */
39
40napi_enable():
41do {
42 val = READ_ONCE(n->state);
43 BUG_ON(!test_bit(NAPI_STATE_SCHED, &val)); /* 50/50 boom */
44<...>
45
46while the typical BUG splat is like:
47
48[ 172.652461] ------------[ cut here ]------------
49[ 172.652462] kernel BUG at net/core/dev.c:6937!
50[ 172.656914] invalid opcode: 0000 [#1] PREEMPT SMP PTI
51[ 172.661966] CPU: 36 PID: 2829 Comm: xdp_redirect_cp Tainted: G I 5.15.0 #42
52[ 172.670222] Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.02.01.0014.082620210524 08/26/2021
53[ 172.680646] RIP: 0010:napi_enable+0x5a/0xd0
54[ 172.684832] Code: 07 49 81 cc 00 01 00 00 4c 89 e2 48 89 d8 80 e6 fb f0 48 0f b1 55 10 48 39 c3 74 10 48 8b 5d 10 f6 c7 04 75 3d f6 c3 01 75 b4 <0f> 0b 5b 5d 41 5c c3 65 ff 05 b8 e5 61 53 48 c7 c6 c0 f3 34 ad 48
55[ 172.703578] RSP: 0018:ffffa3c9497477a8 EFLAGS: 00010246
56[ 172.708803] RAX: ffffa3c96615a014 RBX: 0000000000000000 RCX: ffff8a4b575301a0
57< snip >
58[ 172.782403] Call Trace:
59[ 172.784857] <TASK>
60[ 172.786963] ice_up_complete+0x6f/0x210 [ice]
61[ 172.791349] ice_xdp+0x136/0x320 [ice]
62[ 172.795108] ? ice_change_mtu+0x180/0x180 [ice]
63[ 172.799648] dev_xdp_install+0x61/0xe0
64[ 172.803401] dev_xdp_attach+0x1e0/0x550
65[ 172.807240] dev_change_xdp_fd+0x1e6/0x220
66[ 172.811338] do_setlink+0xee8/0x1010
67[ 172.814917] rtnl_setlink+0xe5/0x170
68[ 172.818499] ? bpf_lsm_binder_set_context_mgr+0x10/0x10
69[ 172.823732] ? security_capable+0x36/0x50
70< snip >
71
72Fix this by replacing 'do { } while (cmpxchg())' with an "infinite"
73for-loop with an explicit break.
74
75From v1 [0]:
76 - just use a for-loop to simplify both the fix and the existing
77 code (Eric).
78
79[0] https://lore.kernel.org/netdev/20211110191126.1214-1-alexandr.lobakin@intel.com
80
81Fixes: 719c57197010 ("net: make napi_disable() symmetric with enable")
82Suggested-by: Eric Dumazet <edumazet@google.com> # for-loop
83Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
84Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
85Reviewed-by: Eric Dumazet <edumazet@google.com>
86---
87 net/core/dev.c | 7 +++++--
88 1 file changed, 5 insertions(+), 2 deletions(-)
89
90diff --git a/net/core/dev.c b/net/core/dev.c
91index c8f7c15..fe2c856 100644
92--- a/net/core/dev.c
93+++ b/net/core/dev.c
94@@ -6391,7 +6391,7 @@ void napi_disable(struct napi_struct *n)
95 might_sleep();
96 set_bit(NAPI_STATE_DISABLE, &n->state);
97
98- do {
99+ for ( ; ; ) {
100 val = READ_ONCE(n->state);
101 if (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
102 usleep_range(20, 200);
103@@ -6400,7 +6400,10 @@ void napi_disable(struct napi_struct *n)
104
105 new = val | NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC;
106 new &= ~(NAPIF_STATE_THREADED);
107- } while (cmpxchg(&n->state, val, new) != val);
108+
109+ if (cmpxchg(&n->state, val, new) == val)
110+ break;
111+ }
112
113 hrtimer_cancel(&n->timer);
114