Re: [PATCH bpf-next v2] Add notrace to queued_spin_lock_slowpath

[Jiri Olsa <olsajiri@gmail.com>]

On Sun, Apr 21, 2024 at 07:43:36PM -0400, Siddharth Chintamaneni wrote:
> This patch is to prevent deadlocks when multiple bpf
> programs are attached to queued_spin_locks functions. This issue is similar
> to what is already discussed[1] before with the spin_lock helpers.
> 
> The addition of notrace macro to the queued_spin_locks
> has been discussed[2] when bpf_spin_locks are introduced.
> 
> [1] https://lore.kernel.org/bpf/CAE5sdEigPnoGrzN8WU7Tx-h-iFuMZgW06qp0KHWtpvoXxf1OAQ@mail.gmail.com/#r
> [2] https://lore.kernel.org/all/20190117011629.efxp7abj4bpf5yco@ast-mbp/t/#maf05c4d71f935f3123013b7ed410e4f50e9da82c
> 
> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@vt.edu>
> ---
>  kernel/locking/qspinlock.c                    |  2 +-
>  .../bpf/prog_tests/tracing_failure.c          | 24 +++++++++++++++++++
>  .../selftests/bpf/progs/tracing_failure.c     |  6 +++++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index ebe6b8ec7cb3..4d46538d8399 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -313,7 +313,7 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
>   * contended             :    (*,x,y) +--> (*,0,0) ---> (*,0,1) -'  :
>   *   queue               :         ^--'                             :
>   */
> -void __lockfunc queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> +notrace void __lockfunc queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)

we did the same for bpf spin lock helpers, which is fine, but I wonder
removing queued_spin_lock_slowpath from traceable functions could break
some scripts (even though many probably use contention tracepoints..)

maybe we could have a list of helpers/kfuncs that could call spin lock
and deny bpf program to load/attach to queued_spin_lock_slowpath
if it calls anything from that list

>  {
>  	struct mcs_spinlock *prev, *next, *node;
>  	u32 old, tail;
> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_failure.c b/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
> index a222df765bc3..822ee6c559bc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
> @@ -28,10 +28,34 @@ static void test_bpf_spin_lock(bool is_spin_lock)
>  	tracing_failure__destroy(skel);
>  }
>  
> +static void test_queued_spin_lock(void)
> +{
> +	struct tracing_failure *skel;
> +	int err;
> +
> +	skel = tracing_failure__open();
> +	if (!ASSERT_OK_PTR(skel, "tracing_failure__open"))
> +		return;
> +
> +	bpf_program__set_autoload(skel->progs.test_queued_spin_lock, true);
> +
> +	err = tracing_failure__load(skel);
> +	if (!ASSERT_OK(err, "tracing_failure__load"))
> +		goto out;
> +
> +	err = tracing_failure__attach(skel);
> +	ASSERT_ERR(err, "tracing_failure__attach");

the test is broken, fentry program won't load with notrace function

[root@qemu bpf]# ./test_progs -n 391/3
test_queued_spin_lock:PASS:tracing_failure__open 0 nsec
libbpf: prog 'test_queued_spin_lock': failed to find kernel BTF type ID of 'queued_spin_lock_slowpath': -3
libbpf: prog 'test_queued_spin_lock': failed to prepare load attributes: -3
libbpf: prog 'test_queued_spin_lock': failed to load: -3
libbpf: failed to load object 'tracing_failure'
libbpf: failed to load BPF skeleton 'tracing_failure': -3
test_queued_spin_lock:FAIL:tracing_failure__load unexpected error: -3 (errno 3)
#391/3   tracing_failure/queued_spin_lock_slowpath:FAIL
#391     tracing_failure:FAIL

jirka

> +
> +out:
> +	tracing_failure__destroy(skel);
> +}
> +
>  void test_tracing_failure(void)
>  {
>  	if (test__start_subtest("bpf_spin_lock"))
>  		test_bpf_spin_lock(true);
>  	if (test__start_subtest("bpf_spin_unlock"))
>  		test_bpf_spin_lock(false);
> +	if (test__start_subtest("queued_spin_lock_slowpath"))
> +		test_queued_spin_lock();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/tracing_failure.c b/tools/testing/selftests/bpf/progs/tracing_failure.c
> index d41665d2ec8c..2d2e7fc9d4f0 100644
> --- a/tools/testing/selftests/bpf/progs/tracing_failure.c
> +++ b/tools/testing/selftests/bpf/progs/tracing_failure.c
> @@ -18,3 +18,9 @@ int BPF_PROG(test_spin_unlock, struct bpf_spin_lock *lock)
>  {
>  	return 0;
>  }
> +
> +SEC("?fentry/queued_spin_lock_slowpath")
> +int BPF_PROG(test_queued_spin_lock, struct qspinlock *lock, u32 val)
> +{
> +	return 0;
> +}
> -- 
> 2.43.0
>