Re: [PATCH v7 3/3] kallsyms: Unroll 24-bit sequence reconstruction in get_symbol_seq()

[Kees Cook <kees@kernel.org>]

On Tue, Sep 29, 2026 at 12:07:32PM -0600, Jim Cromie via B4 Relay wrote:
> Mark get_symbol_seq() as static inline and unroll the 3-byte extraction
> into direct byte shifts: (p[0] << 16) | (p[1] << 8) | p[2]. This
> eliminates loop induction variable maintenance and allows the compiler
> to generate direct loads and constant shifts.

So... did you actually see any difference in the output binary? This is
such a small loop I'd expect every compiler to both inline and unroll
it already. *time passes* I've checked; I was mostly right. :)

It's already inlined and unrolled by the compilers, but GCC weirdly
didn't see through the array math:

  At -O2 GCC (and Clang, mostly the same) on x86_64:

  before                                  after
  lea    (%rax,%rax,2),%eax               lea    (%rax,%rax,2),%edx
  mov    %rax,%rdx                        movslq %edx,%rdx
  movzbl seqs(%rax),%eax                  movzbl seqs(%rdx),%eax
  lea    0x1(%rdx),%ecx                   movzbl seqs+2(%rdx),%ecx
  add    $0x2,%edx                        movzbl seqs+1(%rdx),%edx
  movzbl seqs(%rcx),%ecx                  shl    $0x10,%eax
  shl    $0x8,%eax                        shl    $0x8,%edx
  movzbl seqs(%rdx),%edx                  or     %ecx,%eax
  or     %ecx,%eax                        or     %edx,%eax
  shl    $0x8,%eax
  or     %edx,%eax

So it's actually the loss of the "+ i" part that does it.

> -static unsigned int get_symbol_seq(int index)
> +static inline unsigned int get_symbol_seq(int index)
>  {
> -	unsigned int i, seq = 0;
> +	const u8 *p = &kallsyms_seqs_of_names[3 * index];
>  
> -	for (i = 0; i < 3; i++)
> -		seq = (seq << 8) | kallsyms_seqs_of_names[3 * index + i];
> -
> -	return seq;
> +	return (p[0] << 16) | (p[1] << 8) | p[2];
>  }

I'm on the fence about readability, but I think it's improved.

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook