Re: [PATCH bpf-next 1/5] bpf: Implement task local storage

[Martin KaFai Lau]

On Tue, Oct 27, 2020 at 06:03:13PM +0100, KP Singh wrote:
[ ... ]

> diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> new file mode 100644
> index 000000000000..774140c458cc
> --- /dev/null
> +++ b/kernel/bpf/bpf_task_storage.c
> @@ -0,0 +1,327 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Facebook
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include "linux/pid.h"
> +#include "linux/sched.h"
> +#include <linux/rculist.h>
> +#include <linux/list.h>
> +#include <linux/hash.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_local_storage.h>
> +#include <net/sock.h>
Is this required?

> +#include <uapi/linux/sock_diag.h>
> +#include <uapi/linux/btf.h>
> +#include <linux/bpf_lsm.h>
> +#include <linux/btf_ids.h>
> +#include <linux/fdtable.h>
> +
> +DEFINE_BPF_STORAGE_CACHE(task_cache);
> +
> +static struct bpf_local_storage __rcu **task_storage_ptr(void *owner)
> +{
> +	struct task_struct *task = owner;
> +	struct bpf_storage_blob *bsb;
> +
> +	bsb = bpf_task(task);
> +	if (!bsb)
> +		return NULL;
> +	return &bsb->storage;
> +}
> +
> +static struct bpf_local_storage_data *
> +task_storage_lookup(struct task_struct *task, struct bpf_map *map,
> +		    bool cacheit_lockit)
> +{
> +	struct bpf_local_storage *task_storage;
> +	struct bpf_local_storage_map *smap;
> +	struct bpf_storage_blob *bsb;
> +
> +	bsb = bpf_task(task);
> +	if (!bsb)
> +		return NULL;
> +
> +	task_storage = rcu_dereference(bsb->storage);
> +	if (!task_storage)
> +		return NULL;
> +
> +	smap = (struct bpf_local_storage_map *)map;
> +	return bpf_local_storage_lookup(task_storage, smap, cacheit_lockit);
> +}
> +

[ ... ]

> +static void *bpf_pid_task_storage_lookup_elem(struct bpf_map *map, void *key)
> +{
> +	struct bpf_local_storage_data *sdata;
> +	struct task_struct *task;
> +	struct pid *pid;
> +	struct file *f;
> +	int fd, err;
> +
> +	fd = *(int *)key;
> +	f = fget_raw(fd);
> +	if (!f)
> +		return ERR_PTR(-EBADF);
> +
> +	if (f->f_op != &pidfd_fops) {
> +		err = -EBADF;
> +		goto out_fput;
> +	}
> +
> +	pid = get_pid(f->private_data);
n00b question. Is get_pid(f->private_data) required?
f->private_data could be freed while holding f->f_count?

> +	task = get_pid_task(pid, PIDTYPE_PID);
Should put_task_struct() be called before returning?

> +	if (!task || !task_storage_ptr(task)) {
"!task_storage_ptr(task)" is unnecessary, task_storage_lookup() should
have taken care of it.


> +		err = -ENOENT;
> +		goto out;
> +	}
> +
> +	sdata = task_storage_lookup(task, map, true);
> +	put_pid(pid);
> +	return sdata ? sdata->data : NULL;
> +out:
> +	put_pid(pid);
> +out_fput:
> +	fput(f);
> +	return ERR_PTR(err);
> +}
> +
[ ... ]

> +static int task_storage_map_btf_id;
> +const struct bpf_map_ops task_storage_map_ops = {
> +	.map_meta_equal = bpf_map_meta_equal,
> +	.map_alloc_check = bpf_local_storage_map_alloc_check,
> +	.map_alloc = task_storage_map_alloc,
> +	.map_free = task_storage_map_free,
> +	.map_get_next_key = notsupp_get_next_key,
> +	.map_lookup_elem = bpf_pid_task_storage_lookup_elem,
> +	.map_update_elem = bpf_pid_task_storage_update_elem,
> +	.map_delete_elem = bpf_pid_task_storage_delete_elem,
Please exercise the syscall use cases also in the selftest.

> +	.map_check_btf = bpf_local_storage_map_check_btf,
> +	.map_btf_name = "bpf_local_storage_map",
> +	.map_btf_id = &task_storage_map_btf_id,
> +	.map_owner_storage_ptr = task_storage_ptr,
> +};
> +