We found GCC (at least 9.4.0 and 12.1) introduces a double-fetch of `i8042_ports[I8042_AUX_PORT_NO].serio` at drivers/input/serio/i8042.c:408.
One comparison of the global variable `i8042_ports[I8042_AUX_PORT_NO].serio` has been compiled to three ones,
and thus two extra fetches are introduced.
As in the source code, the global variable is tested (at line 408) before three assignments of irq_bit, disable_bit and port_name.
However, as shown in the following disassembly of i8042_port_close(),
the variable (0x0(%rip)) is fetched and tested three times for each
assignment of irq_bit, disable_bit and port_name.
0000000000000e50 <i8042_port_close>:
i8042_port_close():
./drivers/input/serio/i8042.c:408
e50: 48 39 3d 00 00 00 00 cmp %rdi,0x0(%rip) # first load
./drivers/input/serio/i8042.c:403
e57: 41 54 push %r12
./drivers/input/serio/i8042.c:408
e59: b8 ef ff ff ff mov $0xffffffef,%eax
e5e: 49 c7 c4 00 00 00 00 mov $0x0,%r12
./drivers/input/serio/i8042.c:403
e65: 55 push %rbp
./drivers/input/serio/i8042.c:408
e66: 48 c7 c2 00 00 00 00 mov $0x0,%rdx
./drivers/input/serio/i8042.c:419
e6d: be 60 10 00 00 mov $0x1060,%esi
./drivers/input/serio/i8042.c:403
e72: 53 push %rbx
./drivers/input/serio/i8042.c:408
e73: bb df ff ff ff mov $0xffffffdf,%ebx
e78: 0f 45 d8 cmovne %eax,%ebx
e7b: 0f 95 c0 setne %al
e7e: 83 e8 03 sub $0x3,%eax
e81: 48 39 3d 00 00 00 00 cmp %rdi,0x0(%rip) # second load
e88: 40 0f 94 c5 sete %bpl
e8c: 83 c5 01 add $0x1,%ebp
e8f: 48 39 3d 00 00 00 00 cmp %rdi,0x0(%rip) # third load
./drivers/input/serio/i8042.c:419
e96: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
./drivers/input/serio/i8042.c:408
e9d: 4c 0f 45 e2 cmovne %rdx,%r12
We have not found any lock protection for the three fetches of `i8042_ports[I8042_AUX_PORT_NO].serio` yet.
If the value of this global variable is modified concurrently among the three fetches, the corresponding assignment of
disable_bit or port_name will possibly be incorrect.
This patch fixs this by saving the checked value in advance and using a barrier() to prevent compiler optimizations.
This is inspired by a similar compiler-introduced double fetch situation [1] in driver/xen (?).
[1] GitHub link of commit <8135cf8b092723dbfcc611fe6fdcb3a36c9951c5> ( Save xen_pci_op commands before processing it )
---
drivers/input/serio/i8042.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index f9486495baef..554a2340ca84 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -405,7 +405,9 @@ static void i8042_port_close(struct serio *serio)
int disable_bit;
const char *port_name;
- if (serio == i8042_ports[I8042_AUX_PORT_NO].serio) {
+ struct serio *tmp = i8042_ports[I8042_AUX_PORT_NO].serio;
+ barrier();
+ if (serio == tmp) {
irq_bit = I8042_CTR_AUXINT;
disable_bit = I8042_CTR_AUXDIS;
port_name = "AUX";
Signed-off-by: Kunbo Zhang <absoler@xxxxxxxxxxxxxxxx>
--
2.25.1
