Re: [PATCH] bpf, x86: Sign-extend narrow signed kfunc returns

[Jérémy Jean]

Hello Alexei,

On 2026-08-26 03:22, Alexei Starovoitov wrote:
> On Sun Aug 23, 2026 at 1:39 PM PDT, Jérémy Jean wrote:
> > bpf_res_spin_lock() returns a 32-bit int. On failure, the verifier
> > models R0 as a signed 64-bit value in [-MAX_ERRNO, -1].
> >
> > On x86-64, returning -EDEADLK writes 0xffffffdd to EAX and clears the
> > upper half of RAX. Since the JIT leaves the native return value as-is,
> > BPF sees 0x00000000ffffffdd instead of the sign-extended
> > 0xffffffffffffffdd. A 64-bit signed comparison therefore treats the
> > value as positive, while the verifier treats it as -35.
> >
> > As a result, a signed comparison against zero can take one path during
> > verification and another at run time. With rqspinlock aliases, this 
> > can
> > lead to unmatched bpf_res_spin_unlock() calls, corrupting the per-CPU
> > rqspinlock state and unbalancing the preemption count.
>
> Is that a theory? I'm pretty sure we have selftests for negative
> error codes from res_spin_lock.

> I'm pretty sure we have selftests for negative
> error codes from res_spin_lock.

As far as I could checked, this is not entirely covered: when the
returned 32-bit signed int is a negative integer in %eax
(0xffffffdd), it is later mapped to 64-bit register r0, which clears
the 32 MSB as its value is taken from %rax, and this makes the
negative error code a large 32-bit value stored in r0
(0x00000000ffffffdd). When writing raw BPF, one could check the
return code using r0 instead of w0, and this would introduce a
mismatch with the verifier that sees a real negative value.

In my understanding, the current selftests do not exactly verify this?

> Pls craft a selftest if the issue is real.

Below is an attempt for a selftest. I got help from an LLM to write
the BPF instructions, and then try my best to shrink it but it is a
bit long; hope that's okay. The point is to craft the check on r0
manually after the call to bpf_res_spin_lock(), and forces the branch
taken to be different between the verifier and the jitted code.

The need for raw BPF instructions is required as the compilation from
C correctly uses w0 for the check. I added comments to emphasize this.

SEC("?tc")
__arch_x86_64
__load_if_JITed()
__success
__retval(0)
__naked int res_spin_lock_return_sign_extension(void)
{
  asm volatile ("                                                       
\
  *(u32 *)(r10 - 4) = 0;                                                
\
  r2 = r10;                                                             
\
  r2 += -4;                                                             
\
  r9 = r2;                                                              
\
  r1 = %[arrmap] ll;                                                    
\
  call %[bpf_map_lookup_elem];                                          
\
  if r0 == 0 goto out_%=;                                               
\
  r6 = r0;                                                              
\
  /* Adversarial setup: give one map value two verifier pointer IDs. */ 
\
  r2 = r9;                                                              
\
  r1 = %[arrmap] ll;                                                    
\
  call %[bpf_map_lookup_elem];                                          
\
  if r0 == 0 goto out_%=;                                               
\
  r7 = r0;                                                              
\
  r8 = 0;                                                               
\
  r1 = r6;                                                              
\
  call %[bpf_res_spin_lock];                                            
\
  if w0 != 0 goto out_%=;                                               
\
  r1 = r7;                                                              
\
  call %[bpf_res_spin_lock];                                            
\
  /* Adversarial check: treat the negative int return as 64-bit R0. */  
\
  if r0 s< 0 goto unlock_%=;                                            
\
  r8 = 1;                                                               
\
  if r0 != 0 goto unlock_%=;                                            
\
  r1 = r7;                                                              
\
  call %[bpf_res_spin_unlock];                                          
\
unlock_%=:                                                              
\
  r1 = r6;                                                              
\
  call %[bpf_res_spin_unlock];                                          
\
  r0 = r8;                                                              
\
  exit;                                                                 
\
out_%=:                                                                 
\
  exit;                                                                 
\
"  :
 : __imm(bpf_map_lookup_elem),
   __imm(bpf_res_spin_lock),
   __imm(bpf_res_spin_unlock),
   __imm_addr(arrmap)
 : __clobber_all);
}

When I run the selftest (I used 9328b3b03bdc), I get:

#1/1     res_spin_lock_failure/res_spin_lock_arg:OK
#1/2     res_spin_lock_failure/res_spin_lock_AA:OK
#1/3     res_spin_lock_failure/res_spin_lock_cond_AA:OK
#1/4     res_spin_lock_failure/res_spin_lock_mismatch_1:OK
#1/5     res_spin_lock_failure/res_spin_lock_mismatch_2:OK
#1/6     res_spin_lock_failure/res_spin_lock_irq_mismatch_1:OK
#1/7     res_spin_lock_failure/res_spin_lock_irq_mismatch_2:OK
#1/8     res_spin_lock_failure/res_spin_lock_ooo:OK
#1/9     res_spin_lock_failure/res_spin_lock_ooo_irq:OK
(..snip..)
#1/10    res_spin_lock_failure/pin_lock_return_sign_extension:FAIL
#1/11    res_spin_lock_failure/res_spin_lock_ooo_unlock:OK
#1/12    res_spin_lock_failure/res_spin_lock_bad_off:OK
#1/13    res_spin_lock_failure/res_spin_lock_var_off:OK
#1/14    res_spin_lock_failure/res_spin_lock_no_lock_map:OK
#1/15    res_spin_lock_failure/res_spin_lock_no_lock_kptr:OK

If you confirm this is not yet covered in the selftests, I can send a
two-commit patch (one with the previous patch, one with the selftest).

Regards,
Jérémy