linux-kernel - Re: [PATCH] x86/ioperm: Use atomic64_inc_return() in ksys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFULd4a5+er=7xk+oXOtOsJVUqg86ZWxxvX7jdtOEBcMX60fKg@mail.gmail.com>
Date: Fri, 25 Oct 2024 23:01:20 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: "H. Peter Anvin" <hpa@...or.com>
Cc: Dave Hansen <dave.hansen@...el.com>, x86@...nel.org, linux-kernel@...r.kernel.org, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH] x86/ioperm: Use atomic64_inc_return() in ksys_ioperm()

On Fri, Oct 25, 2024 at 7:13 PM H. Peter Anvin <hpa@...or.com> wrote:
>
> On October 24, 2024 9:20:01 AM PDT, Uros Bizjak <ubizjak@...il.com> wrote:
> >On Thu, Oct 24, 2024 at 5:21 PM Dave Hansen <dave.hansen@...el.com> wrote:
> >>
> >> On 10/7/24 01:33, Uros Bizjak wrote:
> >> > Use atomic64_inc_return(&ref) instead of atomic64_add_return(1, &ref)
> >> > to use optimized implementation and ease register pressure around
> >> > the primitive for targets that implement optimized variant.
> >>
> >> Ease register pressure at the end of a syscall?
> >>
> >> I'll accept that we're doing this just as a matter of hygiene.  But it's
> >> a stretch to say there are any performance concerns whatsoever at the
> >> end of the ioperm() syscall.
> >>
> >> So what is the real reason for this patch?
> >
> >Please see code dumps for i386, a target that implements atomic64_inc_return():
> >
> > 1a9:    8d 04 95 04 00 00 00     lea    0x4(,%edx,4),%eax
> > 1b0:    b9 00 00 00 00           mov    $0x0,%ecx
> >            1b1: R_386_32    .bss
> > 1b5:    89 43 0c                 mov    %eax,0xc(%ebx)
> > 1b8:    31 d2                    xor    %edx,%edx
> > 1ba:    b8 01 00 00 00           mov    $0x1,%eax
> > 1bf:    e8 fc ff ff ff           call   1c0 <ksys_ioperm+0xa8>
> >            1c0: R_386_PC32    atomic64_add_return_cx8
> > 1c4:    89 03                    mov    %eax,(%ebx)
> > 1c6:    89 53 04                 mov    %edx,0x4(%ebx)
> >
> >vs. improved:
> >
> > 1a9:    8d 04 95 04 00 00 00     lea    0x4(,%edx,4),%eax
> > 1b0:    be 00 00 00 00           mov    $0x0,%esi
> >            1b1: R_386_32    .bss
> > 1b5:    89 43 0c                 mov    %eax,0xc(%ebx)
> > 1b8:    e8 fc ff ff ff           call   1b9 <ksys_ioperm+0xa1>
> >            1b9: R_386_PC32    atomic64_inc_return_cx8
> > 1bd:    89 03                    mov    %eax,(%ebx)
> > 1bf:    89 53 04                 mov    %edx,0x4(%ebx)
> >
> >There is no need to initialize %eax/%edx register pair before the
> >"call" to atomic64_inc_return() function. The "call" is not an ABI
> >function call, but an asm volatile (which BTW lacks
> >ASM_CALL_CONSTRAINT), so there is no ABI guarantees which register is
> >call-preserved and which call-clobbered.
> >
> >Oh, this is the "return" variant -  the function indeed returns the
> >new value in %eax/%edx pair, so the difference is only in the
> >redundant register initialization. I can reword the commit message for
> >this case to mention that an initialization of register pair is spared
> >before the call.
> >
> >Uros.
> >
>
> What does ASM_CALL_CONSTRAINT actually do *in the kernel*, *for x86*? There isn't a redzone in the kernel, and there *can't* be, because asynchronous events can clobber data below the stack pointer at any time.

The reason for ASM_CALL_CONSTRAINT is explained in arch/x86/include/asm/asm.h:

--q--
/*
 * This output constraint should be used for any inline asm which has a "call"
 * instruction.  Otherwise the asm may be inserted before the frame pointer
 * gets set up by the containing function.  If you forget to do this, objtool
 * may print a "call without frame pointer save/setup" warning.
 */
register unsigned long current_stack_pointer asm(_ASM_SP);
#define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
--/q--

__alternative_atomic64() macro always uses CALL instruction and one of
alternatives in __arch_{,try_}cmpxchg64_emu() uses CALL as well, so
according to the above comment, they all qualify for
ASM_CALL_CONSTRAINT. This constraint is added to the mentioned macros
in the proposed series [1].

[1] https://lore.kernel.org/lkml/20241024180612.162045-1-ubizjak@gmail.com/

Uros.