Re: [PATCH] x86/ioperm: Use atomic64_inc_return() in ksys_ioperm()

From: H. Peter Anvin
Date: Sat Oct 26 2024 - 19:29:08 EST


On 10/25/24 14:01, Uros Bizjak wrote:

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@xxxxxxxxx/


Ugh. I am not criticizing the usage here, but the construct is bleacherous, because it converts what is properly an input constraint into an inout constraint, which wouldn't be a big deal except for the slight fact that older compilers don't allow asm goto to have output constraints.

By any sane definition, the constraint should actually be an input constraint on the frame pointer itself; something like:

#define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))

... except that "r" really should be a %rbp constraint, but %rbp doesn't seem to have a constraint letter. At least gcc 14.2 seems to do the right thing anyway, though: __builtin_frame_address(0) seems to force a frame pointer to have been created (even with -fomit-frame-pointer specified, and in a leaf function), and the value is always passed in %rbp (because why on Earth would it do it differently, when it is sitting right there?)

-hpa