Re: [PATCH bpf] bpf: Fix deadlock when freeing cgroup storage

[Yonghong Song]

On 12/18/24 1:21 AM, Abel Wu wrote:
> The following commit
> bc235cdb423a ("bpf: Prevent deadlock from recursive bpf_task_storage_[get|delete]")
> first introduced deadlock prevention for fentry/fexit programs attaching
> on bpf_task_storage helpers. That commit also employed the logic in map
> free path in its v6 version.
>
> Later bpf_cgrp_storage was first introduced in
> c4bcfb38a95e ("bpf: Implement cgroup storage available to non-cgroup-attached bpf progs")
> which faces the same issue as bpf_task_storage, instead of its busy
> counter, NULL was passed to bpf_local_storage_map_free() which opened
> a window to cause deadlock:
>
> 	<TASK>
> 	_raw_spin_lock_irqsave+0x3d/0x50
> 	bpf_local_storage_update+0xd1/0x460
> 	bpf_cgrp_storage_get+0x109/0x130
> 	bpf_prog_72026450ec387477_cgrp_ptr+0x38/0x5e
> 	bpf_trace_run1+0x84/0x100
> 	cgroup_storage_ptr+0x4c/0x60
> 	bpf_selem_unlink_storage_nolock.constprop.0+0x135/0x160
> 	bpf_selem_unlink_storage+0x6f/0x110
> 	bpf_local_storage_map_free+0xa2/0x110
> 	bpf_map_free_deferred+0x5b/0x90
> 	process_one_work+0x17c/0x390
> 	worker_thread+0x251/0x360
> 	kthread+0xd2/0x100
> 	ret_from_fork+0x34/0x50
> 	ret_from_fork_asm+0x1a/0x30
> 	</TASK>
>
> 	[ Since the verifier treats 'void *' as scalar which
> 	  prevents me from getting a pointer to 'struct cgroup *',
> 	  I added a raw tracepoint in cgroup_storage_ptr() to
> 	  help reproducing this issue. ]
>
> Although it is tricky to reproduce, the risk of deadlock exists and
> worthy of a fix, by passing its busy counter to the free procedure so
> it can be properly incremented before storage/smap locking.

The above stack trace and explanation does not show that we will have
a potential dead lock here. You mentioned that it is tricky to reproduce,
does it mean that you have done some analysis or coding to reproduce it?
Could you share the details on why you think we may have deadlock here?

> Fixes: c4bcfb38a95e ("bpf: Implement cgroup storage available to non-cgroup-attached bpf progs")
> Signed-off-by: Abel Wu <wuyun.abel@xxxxxxxxxxxxx>
> ---
>   kernel/bpf/bpf_cgrp_storage.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> index 20f05de92e9c..7996fcea3755 100644
> --- a/kernel/bpf/bpf_cgrp_storage.c
> +++ b/kernel/bpf/bpf_cgrp_storage.c
> @@ -154,7 +154,7 @@ static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr)
>   
>   static void cgroup_storage_map_free(struct bpf_map *map)
>   {
> -	bpf_local_storage_map_free(map, &cgroup_cache, NULL);
> +	bpf_local_storage_map_free(map, &cgroup_cache, &bpf_cgrp_storage_busy);
>   }
>   
>   /* *gfp_flags* is a hidden argument provided by the verifier */