Skip to content

Commit 3107549

Browse files
lguohanlguohan
authored
uart: fix race between uart_put_char() and uart_shutdown() (sonic-net#67)
Signed-off-by: Guohan Lu <[email protected]>
1 parent 95db35e commit 3107549

3 files changed

+216
-0
lines changed

‎patch/driver-uart-fix-another-race-between-uart_put_char-and-uart_shutdow.patch

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
uart: fix another race between uart_put_char() and uart_shutdown()
2+
3+
From: Guohan Lu <[email protected]>
4+
5+
this is complete the fix in a5ba1d95e46ecaea638ddd7cd144107c783acb5d
6+
---
7+
drivers/tty/serial/serial_core.c | 6 ++++--
8+
1 file changed, 4 insertions(+), 2 deletions(-)
9+
10+
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
11+
index 53e6db8..ec1187d 100644
12+
--- a/drivers/tty/serial/serial_core.c
13+
+++ b/drivers/tty/serial/serial_core.c
14+
@@ -530,10 +530,12 @@ static int uart_put_char(struct tty_struct *tty, unsigned char c)
15+
int ret = 0;
16+
17+
circ = &state->xmit;
18+
- if (!circ->buf)
19+
+ port = uart_port_lock(state, flags);
20+
+ if (!circ->buf) {
21+
+ uart_port_unlock(port, flags);
22+
return 0;
23+
+ }
24+
25+
- port = uart_port_lock(state, flags);
26+
if (port && uart_circ_chars_free(circ) != 0) {
27+
circ->buf[circ->head] = c;
28+
circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);

‎patch/driver-uart-fix-race-between-uart_put_char-and-uart_shutdow.patch

+186
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
From a5ba1d95e46ecaea638ddd7cd144107c783acb5d Mon Sep 17 00:00:00 2001
2+
From: Tycho Andersen <[email protected]>
3+
Date: Fri, 6 Jul 2018 10:24:57 -0600
4+
Subject: [PATCH] uart: fix race between uart_put_char() and uart_shutdown()
5+
MIME-Version: 1.0
6+
Content-Type: text/plain; charset=UTF-8
7+
Content-Transfer-Encoding: 8bit
8+
9+
We have reports of the following crash:
10+
11+
PID: 7 TASK: ffff88085c6d61c0 CPU: 1 COMMAND: "kworker/u25:0"
12+
#0 [ffff88085c6db710] machine_kexec at ffffffff81046239
13+
#1 [ffff88085c6db760] crash_kexec at ffffffff810fc248
14+
#2 [ffff88085c6db830] oops_end at ffffffff81008ae7
15+
#3 [ffff88085c6db860] no_context at ffffffff81050b8f
16+
#4 [ffff88085c6db8b0] __bad_area_nosemaphore at ffffffff81050d75
17+
#5 [ffff88085c6db900] bad_area_nosemaphore at ffffffff81050e83
18+
#6 [ffff88085c6db910] __do_page_fault at ffffffff8105132e
19+
#7 [ffff88085c6db9b0] do_page_fault at ffffffff8105152c
20+
#8 [ffff88085c6db9c0] page_fault at ffffffff81a3f122
21+
[exception RIP: uart_put_char+149]
22+
RIP: ffffffff814b67b5 RSP: ffff88085c6dba78 RFLAGS: 00010006
23+
RAX: 0000000000000292 RBX: ffffffff827c5120 RCX: 0000000000000081
24+
RDX: 0000000000000000 RSI: 000000000000005f RDI: ffffffff827c5120
25+
RBP: ffff88085c6dba98 R8: 000000000000012c R9: ffffffff822ea320
26+
R10: ffff88085fe4db04 R11: 0000000000000001 R12: ffff881059f9c000
27+
R13: 0000000000000001 R14: 000000000000005f R15: 0000000000000fba
28+
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
29+
#9 [ffff88085c6dbaa0] tty_put_char at ffffffff81497544
30+
#10 [ffff88085c6dbac0] do_output_char at ffffffff8149c91c
31+
#11 [ffff88085c6dbae0] __process_echoes at ffffffff8149cb8b
32+
#12 [ffff88085c6dbb30] commit_echoes at ffffffff8149cdc2
33+
#13 [ffff88085c6dbb60] n_tty_receive_buf_fast at ffffffff8149e49b
34+
#14 [ffff88085c6dbbc0] __receive_buf at ffffffff8149ef5a
35+
#15 [ffff88085c6dbc20] n_tty_receive_buf_common at ffffffff8149f016
36+
#16 [ffff88085c6dbca0] n_tty_receive_buf2 at ffffffff8149f194
37+
#17 [ffff88085c6dbcb0] flush_to_ldisc at ffffffff814a238a
38+
#18 [ffff88085c6dbd50] process_one_work at ffffffff81090be2
39+
#19 [ffff88085c6dbe20] worker_thread at ffffffff81091b4d
40+
#20 [ffff88085c6dbeb0] kthread at ffffffff81096384
41+
#21 [ffff88085c6dbf50] ret_from_fork at ffffffff81a3d69f​
42+
43+
after slogging through some dissasembly:
44+
45+
ffffffff814b6720 <uart_put_char>:
46+
ffffffff814b6720: 55 push %rbp
47+
ffffffff814b6721: 48 89 e5 mov %rsp,%rbp
48+
ffffffff814b6724: 48 83 ec 20 sub $0x20,%rsp
49+
ffffffff814b6728: 48 89 1c 24 mov %rbx,(%rsp)
50+
ffffffff814b672c: 4c 89 64 24 08 mov %r12,0x8(%rsp)
51+
ffffffff814b6731: 4c 89 6c 24 10 mov %r13,0x10(%rsp)
52+
ffffffff814b6736: 4c 89 74 24 18 mov %r14,0x18(%rsp)
53+
ffffffff814b673b: e8 b0 8e 58 00 callq ffffffff81a3f5f0 <mcount>
54+
ffffffff814b6740: 4c 8b a7 88 02 00 00 mov 0x288(%rdi),%r12
55+
ffffffff814b6747: 45 31 ed xor %r13d,%r13d
56+
ffffffff814b674a: 41 89 f6 mov %esi,%r14d
57+
ffffffff814b674d: 49 83 bc 24 70 01 00 cmpq $0x0,0x170(%r12)
58+
ffffffff814b6754: 00 00
59+
ffffffff814b6756: 49 8b 9c 24 80 01 00 mov 0x180(%r12),%rbx
60+
ffffffff814b675d: 00
61+
ffffffff814b675e: 74 2f je ffffffff814b678f <uart_put_char+0x6f>
62+
ffffffff814b6760: 48 89 df mov %rbx,%rdi
63+
ffffffff814b6763: e8 a8 67 58 00 callq ffffffff81a3cf10 <_raw_spin_lock_irqsave>
64+
ffffffff814b6768: 41 8b 8c 24 78 01 00 mov 0x178(%r12),%ecx
65+
ffffffff814b676f: 00
66+
ffffffff814b6770: 89 ca mov %ecx,%edx
67+
ffffffff814b6772: f7 d2 not %edx
68+
ffffffff814b6774: 41 03 94 24 7c 01 00 add 0x17c(%r12),%edx
69+
ffffffff814b677b: 00
70+
ffffffff814b677c: 81 e2 ff 0f 00 00 and $0xfff,%edx
71+
ffffffff814b6782: 75 23 jne ffffffff814b67a7 <uart_put_char+0x87>
72+
ffffffff814b6784: 48 89 c6 mov %rax,%rsi
73+
ffffffff814b6787: 48 89 df mov %rbx,%rdi
74+
ffffffff814b678a: e8 e1 64 58 00 callq ffffffff81a3cc70 <_raw_spin_unlock_irqrestore>
75+
ffffffff814b678f: 44 89 e8 mov %r13d,%eax
76+
ffffffff814b6792: 48 8b 1c 24 mov (%rsp),%rbx
77+
ffffffff814b6796: 4c 8b 64 24 08 mov 0x8(%rsp),%r12
78+
ffffffff814b679b: 4c 8b 6c 24 10 mov 0x10(%rsp),%r13
79+
ffffffff814b67a0: 4c 8b 74 24 18 mov 0x18(%rsp),%r14
80+
ffffffff814b67a5: c9 leaveq
81+
ffffffff814b67a6: c3 retq
82+
ffffffff814b67a7: 49 8b 94 24 70 01 00 mov 0x170(%r12),%rdx
83+
ffffffff814b67ae: 00
84+
ffffffff814b67af: 48 63 c9 movslq %ecx,%rcx
85+
ffffffff814b67b2: 41 b5 01 mov $0x1,%r13b
86+
ffffffff814b67b5: 44 88 34 0a mov %r14b,(%rdx,%rcx,1)
87+
ffffffff814b67b9: 41 8b 94 24 78 01 00 mov 0x178(%r12),%edx
88+
ffffffff814b67c0: 00
89+
ffffffff814b67c1: 83 c2 01 add $0x1,%edx
90+
ffffffff814b67c4: 81 e2 ff 0f 00 00 and $0xfff,%edx
91+
ffffffff814b67ca: 41 89 94 24 78 01 00 mov %edx,0x178(%r12)
92+
ffffffff814b67d1: 00
93+
ffffffff814b67d2: eb b0 jmp ffffffff814b6784 <uart_put_char+0x64>
94+
ffffffff814b67d4: 66 66 66 2e 0f 1f 84 data32 data32 nopw %cs:0x0(%rax,%rax,1)
95+
ffffffff814b67db: 00 00 00 00 00
96+
97+
for our build, this is crashing at:
98+
99+
circ->buf[circ->head] = c;
100+
101+
Looking in uart_port_startup(), it seems that circ->buf (state->xmit.buf)
102+
protected by the "per-port mutex", which based on uart_port_check() is
103+
state->port.mutex. Indeed, the lock acquired in uart_put_char() is
104+
uport->lock, i.e. not the same lock.
105+
106+
Anyway, since the lock is not acquired, if uart_shutdown() is called, the
107+
last chunk of that function may release state->xmit.buf before its assigned
108+
to null, and cause the race above.
109+
110+
To fix it, let's lock uport->lock when allocating/deallocating
111+
state->xmit.buf in addition to the per-port mutex.
112+
113+
v2: switch to locking uport->lock on allocation/deallocation instead of
114+
locking the per-port mutex in uart_put_char. Note that since
115+
uport->lock is a spin lock, we have to switch the allocation to
116+
GFP_ATOMIC.
117+
v3: move the allocation outside the lock, so we can switch back to
118+
GFP_KERNEL
119+
120+
Signed-off-by: Tycho Andersen <[email protected]>
121+
Cc: stable <[email protected]>
122+
Signed-off-by: Greg Kroah-Hartman <[email protected]>
123+
---
124+
drivers/tty/serial/serial_core.c | 17 ++++++++++++-----
125+
1 file changed, 12 insertions(+), 5 deletions(-)
126+
127+
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
128+
index 9c14a45..80bb56f 100644
129+
--- a/drivers/tty/serial/serial_core.c
130+
+++ b/drivers/tty/serial/serial_core.c
131+
@@ -182,6 +182,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
132+
{
133+
struct uart_port *uport = uart_port_check(state);
134+
unsigned long page;
135+
+ unsigned long flags = 0;
136+
int retval = 0;
137+
138+
if (uport->type == PORT_UNKNOWN)
139+
@@ -196,15 +197,18 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
140+
* Initialise and allocate the transmit and temporary
141+
* buffer.
142+
*/
143+
- if (!state->xmit.buf) {
144+
- /* This is protected by the per port mutex */
145+
- page = get_zeroed_page(GFP_KERNEL);
146+
- if (!page)
147+
- return -ENOMEM;
148+
+ page = get_zeroed_page(GFP_KERNEL);
149+
+ if (!page)
150+
+ return -ENOMEM;
151+
152+
+ uart_port_lock(state, flags);
153+
+ if (!state->xmit.buf) {
154+
state->xmit.buf = (unsigned char *) page;
155+
uart_circ_clear(&state->xmit);
156+
+ } else {
157+
+ free_page(page);
158+
}
159+
+ uart_port_unlock(uport, flags);
160+
161+
retval = uport->ops->startup(uport);
162+
if (retval == 0) {
163+
@@ -263,6 +267,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
164+
{
165+
struct uart_port *uport = uart_port_check(state);
166+
struct tty_port *port = &state->port;
167+
+ unsigned long flags = 0;
168+
169+
/*
170+
* Set the TTY IO error marker
171+
@@ -295,10 +300,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
172+
/*
173+
* Free the transmit buffer page.
174+
*/
175+
+ uart_port_lock(state, flags);
176+
if (state->xmit.buf) {
177+
free_page((unsigned long)state->xmit.buf);
178+
state->xmit.buf = NULL;
179+
}
180+
+ uart_port_unlock(uport, flags);
181+
}
182+
183+
/**
184+
--
185+
2.7.4
186+

‎patch/series

+2
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ driver-sff-8436-use-nvmem-framework.patch
2121
driver-sff-8436-use-nvmem_device_read.patch
2222
driver-support-sff-8436-read-write-fix.patch
2323
driver-i2c-bus-intel-ismt-add-delay-param.patch
24+
driver-uart-fix-race-between-uart_put_char-and-uart_shutdow.patch
25+
driver-uart-fix-another-race-between-uart_put_char-and-uart_shutdow.patch
2426
driver-pca954x-i2c-mux-force-deselect-on-exit-flag.patch
2527
kernel-add-kexec-reboot-string.patch
2628
driver-hwmon-max6620.patch

0 commit comments

Comments
 (0)