Re: [PATCH bpf v1 1/2] bpf: Fix grace period wait for tracepoint bpf_link

[Puranjay Mohan <puranjay@kernel.org>]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

> Recently, tracepoints were switched from using disabled preemption
> (which acts as RCU read section) to SRCU-fast when they are not
> faultable. This means that to do a proper grace period wait for programs
> running in such tracepoints, we must use SRCU's grace period wait.
> This is only for non-faultable tracepoints, faultable ones continue
> using RCU Tasks Trace.
>
> However, bpf_link_free() currently does call_rcu() for all cases when
> the link is non-sleepable (hence, for tracepoints, non-faultable). Fix
> this by doing a call_srcu() grace period wait.
>
> As far RCU Tasks Trace gp -> RCU gp chaining is concerned, it is deemed
> unnecessary for tracepoint programs. The link and program are either
> accessed under RCU Tasks Trace protection, or SRCU-fast protection now.
>
> The earlier logic of chaining both RCU Tasks Trace and RCU gp waits was
> to generalize the logic, even if it conceded an extra RCU gp wait,
> however that is unnecessary for tracepoints even before this change.
> In practice no cost was paid since rcu_trace_implies_rcu_gp() was always
> true.
>
> Hence we need not chain any SRCU gp waits after RCU Tasks Trace.
>
> Fixes: a46023d5616e ("tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

With a nit below which is mostly informational.

Reviewed-by: Puranjay Mohan <puranjay@kernel.org>

> ---
>  include/linux/tracepoint.h |  8 ++++++++
>  kernel/bpf/syscall.c       | 22 ++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 22ca1c8b54f3..8227102a771f 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -113,6 +113,10 @@ void for_each_tracepoint_in_module(struct module *mod,
>   */
>  #ifdef CONFIG_TRACEPOINTS
>  extern struct srcu_struct tracepoint_srcu;
> +static inline struct srcu_struct *tracepoint_srcu_ptr(void)
> +{
> +	return &tracepoint_srcu;
> +}
>  static inline void tracepoint_synchronize_unregister(void)
>  {
>  	synchronize_rcu_tasks_trace();
> @@ -123,6 +127,10 @@ static inline bool tracepoint_is_faultable(struct tracepoint *tp)
>  	return tp->ext && tp->ext->faultable;
>  }
>  #else
> +static inline struct srcu_struct *tracepoint_srcu_ptr(void)
> +{
> +	return NULL;
> +}
>  static inline void tracepoint_synchronize_unregister(void)
>  { }
>  static inline bool tracepoint_is_faultable(struct tracepoint *tp)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 274039e36465..ab61a5ce35af 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3261,6 +3261,13 @@ static void bpf_link_defer_dealloc_rcu_gp(struct rcu_head *rcu)
>  	bpf_link_dealloc(link);
>  }
>  
> +static bool bpf_link_is_tracepoint(struct bpf_link *link)
> +{
> +	/* Only these combinations support a tracepoint bpf_link. */
> +	return link->type == BPF_LINK_TYPE_RAW_TRACEPOINT ||
> +	       (link->type == BPF_LINK_TYPE_TRACING && link->attach_type == BPF_TRACE_RAW_TP);

nit: this second check is never true here, because BPF_LINK_TYPE_TRACING
uses bpf_tracing_link_lops, which has .dealloc (not .dealloc_deferred)
and this function is only called from dealloc_deferred() path.

> +}
> +
>  static void bpf_link_defer_dealloc_mult_rcu_gp(struct rcu_head *rcu)
>  {
>  	if (rcu_trace_implies_rcu_gp())
> @@ -3279,16 +3286,27 @@ static void bpf_link_free(struct bpf_link *link)
>  	if (link->prog)
>  		ops->release(link);
>  	if (ops->dealloc_deferred) {
> -		/* Schedule BPF link deallocation, which will only then
> +		struct srcu_struct *tp_srcu = tracepoint_srcu_ptr();
> +
> +		/*
> +		 * Schedule BPF link deallocation, which will only then
>  		 * trigger putting BPF program refcount.
>  		 * If underlying BPF program is sleepable or BPF link's target
>  		 * attach hookpoint is sleepable or otherwise requires RCU GPs
>  		 * to ensure link and its underlying BPF program is not
>  		 * reachable anymore, we need to first wait for RCU tasks
> -		 * trace sync, and then go through "classic" RCU grace period
> +		 * trace sync, and then go through "classic" RCU grace period.
> +		 *
> +		 * For tracepoint BPF links, we need to go through SRCU grace
> +		 * period wait instead when non-faultable tracepoint is used. We
> +		 * don't need to chain SRCU grace period waits, however, for the
> +		 * faultable case, since it exclusively uses RCU Tasks Trace.
>  		 */
>  		if (link->sleepable || (link->prog && link->prog->sleepable))
>  			call_rcu_tasks_trace(&link->rcu, bpf_link_defer_dealloc_mult_rcu_gp);
> +		/* We need to do a SRCU grace period wait for tracepoint-based BPF links. */
> +		else if (bpf_link_is_tracepoint(link) && tp_srcu)
> +			call_srcu(tp_srcu, &link->rcu, bpf_link_defer_dealloc_rcu_gp);
>  		else
>  			call_rcu(&link->rcu, bpf_link_defer_dealloc_rcu_gp);
>  	} else if (ops->dealloc) {
> -- 
> 2.52.0