Re: [PATCH bpf-next] bpf: Handle MEM_RCU type properly

[Yonghong Song <yhs@meta.com>]

On 11/28/22 6:37 PM, Yonghong Song wrote:
> Commit 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
> introduced MEM_RCU and bpf_rcu_read_lock/unlock() support. In that
> commit, a rcu pointer is tagged with both MEM_RCU and PTR_TRUSTED
> so that it can be passed into kfuncs or helpers as an argument.
> 
> Martin raised a good question in [1] such that the rcu pointer,
> although being able to accessing the object, might have reference
> count of 0. This might cause a problem if the rcu pointer is passed
> to a kfunc which expects trusted arguments where ref count should
> be greater than 0.
> 
> So this patch tries to fix this problem by tagging rcu pointer with
> MEM_RCU only. Special acquire functions are needed to try to
> acquire a reference with the consideration that the original rcu
> pointer ref count could be 0. This special acquire function's
> argument needs to be KF_RCU, a new introduced kfunc flag. In
> verifier, KF_RCU will require the actual argument register type
> to be MEM_RCU.
> 
>   [1] https://lore.kernel.org/bpf/ac70f574-4023-664e-b711-e0d3b18117fd@linux.dev/
> 
> Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>   include/linux/bpf_verifier.h                  |  2 +-
>   include/linux/btf.h                           |  1 +
>   kernel/bpf/helpers.c                          | 14 ++++++++
>   kernel/bpf/verifier.c                         | 36 +++++++++++++------
>   .../selftests/bpf/prog_tests/cgrp_kfunc.c     |  4 +--
>   .../selftests/bpf/prog_tests/task_kfunc.c     |  4 +--
>   .../selftests/bpf/progs/rcu_read_lock.c       |  7 ++--
>   7 files changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index c05aa6e1f6f5..6f192dd9025e 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -683,7 +683,7 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
>   	}
>   }
>   
> -#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | MEM_RCU | PTR_TRUSTED)
> +#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | PTR_TRUSTED)
>   
>   static inline bool bpf_type_has_unsafe_modifiers(u32 type)
>   {
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 9ed00077db6e..cbd6e4096f8c 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -70,6 +70,7 @@
>   #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
>   #define KF_SLEEPABLE    (1 << 5) /* kfunc may sleep */
>   #define KF_DESTRUCTIVE  (1 << 6) /* kfunc performs destructive actions */
> +#define KF_RCU          (1 << 7) /* kfunc only takes rcu pointer arguments */
>   
>   /*
>    * Return the name of the passed struct, if exists, or halt the build if for
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a5a511430f2a..46fbe027f3b6 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1837,6 +1837,19 @@ struct task_struct *bpf_task_acquire(struct task_struct *p)
>   	return p;
>   }
>   
> +/**
> + * bpf_task_acquire_rcu - Acquire a reference to a rcu task object. A task
> + * acquired by this kfunc which is not stored in a map as a kptr, must be
> + * released by calling bpf_task_release().
> + * @p: The task on which a reference is being acquired or NULL.

A typo. The argument @p cannot be NULL. Will wait for more feedbacks
before sending next version with the fix.

> + */
> +struct task_struct *bpf_task_acquire_rcu(struct task_struct *p)
> +{
> +	if (!refcount_inc_not_zero(&p->rcu_users))
> +		return NULL;
> +	return p;
> +}
> +
>   /**
[...]