Re: [PATCH v2 01/13] perf: Ensure perf_guest_cbs aren't reloaded between !NULL check and deref

[Peter Zijlstra <peterz@xxxxxxxxxxxxx>]

On Fri, Aug 27, 2021 at 05:35:46PM -0700, Sean Christopherson wrote:

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 2d510ad750ed..6b0405e578c1 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1237,6 +1237,14 @@ extern void perf_event_bpf_event(struct bpf_prog *prog,
>                                u16 flags);
>  
>  extern struct perf_guest_info_callbacks *perf_guest_cbs;
> +static inline struct perf_guest_info_callbacks *perf_get_guest_cbs(void)
> +{
> +     /* Reg/unreg perf_guest_cbs waits for readers via synchronize_rcu(). */
> +     lockdep_assert_preemption_disabled();
> +
> +     /* Prevent reloading between a !NULL check and dereferences. */
> +     return READ_ONCE(perf_guest_cbs);
> +}

Nice..

>  extern int perf_register_guest_info_callbacks(struct 
> perf_guest_info_callbacks *callbacks);
>  extern int perf_unregister_guest_info_callbacks(struct 
> perf_guest_info_callbacks *callbacks);
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 464917096e73..2126f6327321 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6491,14 +6491,19 @@ struct perf_guest_info_callbacks *perf_guest_cbs;
>  
>  int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
>  {
> -     perf_guest_cbs = cbs;
> +     if (WARN_ON_ONCE(perf_guest_cbs))
> +             return -EBUSY;
> +
> +     WRITE_ONCE(perf_guest_cbs, cbs);
> +     synchronize_rcu();

You're waiting for all NULL users to go away? :-) IOW, we can do without
this synchronize_rcu() call.

>       return 0;
>  }
>  EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
>  
>  int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks 
> *cbs)
>  {
> -     perf_guest_cbs = NULL;

        if (WARN_ON_ONCE(perf_guest_cbs != cbs))
                return -EBUSY;

?

> +     WRITE_ONCE(perf_guest_cbs, NULL);
> +     synchronize_rcu();
>       return 0;
>  }
>  EXPORT_SYMBOL_GPL(perf_unregister_guest_info_callbacks);

Yes, this ought to work fine.