On Fri, Nov 04, 2022 at 03:23:47PM +0800, Kunbo Zhang wrote:
> 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>
>
Regarding patch description, there are several guides:
* Wrap the text (excluding code or terminal output) at 80 character
or less.
* Please write in imperative mood instead (no first-person pronouns [I, we],
"make foo do bar").
* When referring to past commits, the format should be
--pretty=format:'%h ("%s")'. The commit hash is at least 12
characters long (set core.abbrev to 12 on your .gitconfig)
* Last but not least, place your SoB at the end of description (before
three dash line).
Also, is this patch also applies to stable branches? If so, add Cc:
stable@xxxxxxxxxxxxxxx to the SoB area.
Thanks.
--
An old man doll... just what I always wanted! - Clara
Attachment:
signature.asc
Description: PGP signature
