Re: bpf: fix key serial argument of bpf_lookup_user_key()

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Tue, 2025-06-17 at 10:57 -0400, James Bottomley wrote:
> The underlying lookup_user_key() function uses a signed 32 bit integer
> for key serial numbers because legitimate serial numbers are positive
> (and > 3) and keyrings are negative.  Using a u32 for the keyring in
> the bpf function doesn't currently cause any conversion problems but
> will start to trip the signed to unsigned conversion warnings when the
> kernel enables them, so convert the argument to signed (and update the
> tests accordingly) before it acquires more users.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Looks good from my side:

Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks

Roberto

> ---
>  kernel/trace/bpf_trace.c                                  | 2 +-
>  tools/testing/selftests/bpf/bpf_kfuncs.h                  | 2 +-
>  tools/testing/selftests/bpf/progs/rcu_read_lock.c         | 5 +++--
>  tools/testing/selftests/bpf/progs/test_lookup_key.c       | 4 ++--
>  tools/testing/selftests/bpf/progs/test_sig_in_xattr.c     | 2 +-
>  tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c | 2 +-
>  tools/testing/selftests/bpf/progs/verifier_ref_tracking.c | 2 +-
>  7 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 132c8be6f635..0a06ea6638fe 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1270,7 +1270,7 @@ __bpf_kfunc_start_defs();
>   * Return: a bpf_key pointer with a valid key pointer if the key is found, a
>   *         NULL pointer otherwise.
>   */
> -__bpf_kfunc struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)
> +__bpf_kfunc struct bpf_key *bpf_lookup_user_key(s32 serial, u64 flags)
>  {
>  	key_ref_t key_ref;
>  	struct bpf_key *bkey;
> diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
> index 8215c9b3115e..9386dfe8b884 100644
> --- a/tools/testing/selftests/bpf/bpf_kfuncs.h
> +++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
> @@ -69,7 +69,7 @@ extern int bpf_get_file_xattr(struct file *file, const char *name,
>  			      struct bpf_dynptr *value_ptr) __ksym;
>  extern int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr *digest_ptr) __ksym;
>  
> -extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
> +extern struct bpf_key *bpf_lookup_user_key(__s32 serial, __u64 flags) __ksym;
>  extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
>  extern void bpf_key_put(struct bpf_key *key) __ksym;
>  extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> diff --git a/tools/testing/selftests/bpf/progs/rcu_read_lock.c b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> index 43637ee2cdcd..3a868a199349 100644
> --- a/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> +++ b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> @@ -16,10 +16,11 @@ struct {
>  	__type(value, long);
>  } map_a SEC(".maps");
>  
> -__u32 user_data, key_serial, target_pid;
> +__u32 user_data, target_pid;
> +__s32 key_serial;
>  __u64 flags, task_storage_val, cgroup_id;
>  
> -struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
> +struct bpf_key *bpf_lookup_user_key(__s32 serial, __u64 flags) __ksym;
>  void bpf_key_put(struct bpf_key *key) __ksym;
>  void bpf_rcu_read_lock(void) __ksym;
>  void bpf_rcu_read_unlock(void) __ksym;
> diff --git a/tools/testing/selftests/bpf/progs/test_lookup_key.c b/tools/testing/selftests/bpf/progs/test_lookup_key.c
> index cdbbb12f1491..1f7e1e59b073 100644
> --- a/tools/testing/selftests/bpf/progs/test_lookup_key.c
> +++ b/tools/testing/selftests/bpf/progs/test_lookup_key.c
> @@ -14,11 +14,11 @@
>  char _license[] SEC("license") = "GPL";
>  
>  __u32 monitored_pid;
> -__u32 key_serial;
> +__s32 key_serial;
>  __u32 key_id;
>  __u64 flags;
>  
> -extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
> +extern struct bpf_key *bpf_lookup_user_key(__s32 serial, __u64 flags) __ksym;
>  extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
>  extern void bpf_key_put(struct bpf_key *key) __ksym;
>  
> diff --git a/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c b/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
> index 8ef6b39335b6..34b30e2603f0 100644
> --- a/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
> +++ b/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
> @@ -40,7 +40,7 @@ char digest[MAGIC_SIZE + SIZEOF_STRUCT_FSVERITY_DIGEST + SHA256_DIGEST_SIZE];
>  __u32 monitored_pid;
>  char sig[MAX_SIG_SIZE];
>  __u32 sig_size;
> -__u32 user_keyring_serial;
> +__s32 user_keyring_serial;
>  
>  SEC("lsm.s/file_open")
>  int BPF_PROG(test_file_open, struct file *f)
> diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> index e96d09e11115..ff8d755548b9 100644
> --- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> @@ -17,7 +17,7 @@
>  #define MAX_SIG_SIZE 1024
>  
>  __u32 monitored_pid;
> -__u32 user_keyring_serial;
> +__s32 user_keyring_serial;
>  __u64 system_keyring_id;
>  
>  struct data {
> diff --git a/tools/testing/selftests/bpf/progs/verifier_ref_tracking.c b/tools/testing/selftests/bpf/progs/verifier_ref_tracking.c
> index 683a882b3e6d..910365201f68 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_ref_tracking.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_ref_tracking.c
> @@ -27,7 +27,7 @@ struct bpf_key {} __attribute__((preserve_access_index));
>  
>  extern void bpf_key_put(struct bpf_key *key) __ksym;
>  extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
> -extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
> +extern struct bpf_key *bpf_lookup_user_key(__s32 serial, __u64 flags) __ksym;
>  
>  /* BTF FUNC records are not generated for kfuncs referenced
>   * from inline assembly. These records are necessary for