linux-kernel - Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

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: <Yz7B1991xDY9ZtfL@xhacker>
Date:   Thu, 6 Oct 2022 19:53:59 +0800
From:   Jisheng Zhang <jszhang@...nel.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to
 satisfy asm constraints

On Thu, Oct 06, 2022 at 11:14:42AM +0100, Mark Rutland wrote:
> On Thu, Oct 06, 2022 at 03:55:41PM +0800, Jisheng Zhang wrote:
> > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > arch_static_branch_jump()'s arguments as const to satisfy asm
> > constraints. And Steven in [1] also pointed out that "The "i"
> > constraint needs to be a constant."
> 
> It needs to be a *compile-time constant*, but `const` on a function argument
> only ensures that the function can't modify the argument, not that it's a

compile-time constant is a subset of `const`.

> constant in the caller.
> 
> I think this is a quirk of the optimizer rather than anything else.

I dunno compiler internals, just tried as commit 864b435514b2 suggested
the issue did disappear. 

PS: I agree with you about this is a quirk or workaround.

> 
> > Tested with building a simple external kernel module with "O0".
> 
> Is building with `-O0` supported? I thought we required using `-O2` or above
> for a bunch of code that requires constant propagation, etc.

Per the information of Jason's reply in [1]: the reason tring O0/O1 is "to
play around with GCC's new static analyzer options".

While the reason I constify the arguments is that: in riscv world, even the
"-Os" can also reproduce the warnings and errors[2]. Grepping source found
arm64 also shares the same style, so these two patches.

[2]https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel@sholland.org/

> 
> I don't really have a problem with making this const, but I don't particularly
> want to try to "fix" all the other code that depends on constant propagation to
> assemble, and I'm worried this is the canary in the coal mine.

IMHO, it's a good idea to constify if the arguments can't be modified.

> 
> Thanks,
> Mark.
> 
> > 
> > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> > ---
> >  arch/arm64/include/asm/jump_label.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > index cea441b6aa5d..48ddc0f45d22 100644
> > --- a/arch/arm64/include/asm/jump_label.h
> > +++ b/arch/arm64/include/asm/jump_label.h
> > @@ -15,8 +15,8 @@
> >  
> >  #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
> >  
> > -static __always_inline bool arch_static_branch(struct static_key *key,
> > -					       bool branch)
> > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > +					       const bool branch)
> >  {
> >  	asm_volatile_goto(
> >  		"1:	nop					\n\t"
> > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> >  	return true;
> >  }
> >  
> > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > -						    bool branch)
> > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > +						    const bool branch)
> >  {
> >  	asm_volatile_goto(
> >  		"1:	b		%l[l_yes]		\n\t"
> > -- 
> > 2.37.2
> >