On Feb 22, 2021, at 10:21 PM, Andrii Nakryiko [off-list ref] wrote:

On Mon, Feb 22, 2021 at 5:23 PM Song Liu [off-list ref] wrote:

quoted

BPF helpers bpf_task_storage_[get|delete] could hold two locks:
bpf_local_storage_map_bucket->lock and bpf_local_storage->lock. Calling
these helpers from fentry/fexit programs on functions in bpf_*_storage.c
may cause deadlock on either locks.

Prevent such deadlock with a per cpu counter, bpf_task_storage_busy, which
is similar to bpf_prog_active. We need this counter to be global, because
the two locks here belong to two different objects: bpf_local_storage_map
and bpf_local_storage. If we pick one of them as the owner of the counter,
it is still possible to trigger deadlock on the other lock. For example,
if bpf_local_storage_map owns the counters, it cannot prevent deadlock
on bpf_local_storage->lock when two maps are used.

Signed-off-by: Song Liu <redacted>
---
kernel/bpf/bpf_task_storage.c | 57 ++++++++++++++++++++++++++++++-----
1 file changed, 50 insertions(+), 7 deletions(-)

[...]

quoted

@@ -109,7 +136,9 @@ static void *bpf_pid_task_storage_lookup_elem(struct bpf_map *map, void *key)
               goto out;
       }

+       bpf_task_storage_lock();
       sdata = task_storage_lookup(task, map, true);
+       bpf_task_storage_unlock();
       put_pid(pid);
       return sdata ? sdata->data : NULL;

out:

@@ -141,8 +170,10 @@ static int bpf_pid_task_storage_update_elem(struct bpf_map *map, void *key,
               goto out;
       }

+       bpf_task_storage_lock();
       sdata = bpf_local_storage_update(
               task, (struct bpf_local_storage_map *)map, value, map_flags);

this should probably be container_of() instead of casting

bpf_task_storage.c uses casting in multiple places. How about we fix it in a 
separate patch?

Thanks,
Song

quoted

+       bpf_task_storage_unlock();

       err = PTR_ERR_OR_ZERO(sdata);
out:

@@ -185,7 +216,9 @@ static int bpf_pid_task_storage_delete_elem(struct bpf_map *map, void *key)
               goto out;
       }

+       bpf_task_storage_lock();
       err = task_storage_delete(task, map);
+       bpf_task_storage_unlock();

out:
       put_pid(pid);
       return err;

[...]