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

[Song Liu <songliubraving@fb.com>]

> On Nov 3, 2020, at 7:31 AM, KP Singh <kpsingh@chromium.org> wrote:
> 
> From: KP Singh <kpsingh@google.com>
> 
> Similar to bpf_local_storage for sockets and inodes add local storage
> for task_struct.
> 
> The life-cycle of storage is managed with the life-cycle of the
> task_struct.  i.e. the storage is destroyed along with the owning task
> with a callback to the bpf_task_storage_free from the task_free LSM
> hook.
> 
> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in
> the security blob which are now stackable and can co-exist with other
> LSMs.
> 
> The userspace map operations can be done by using a pid fd as a key
> passed to the lookup, update and delete operations.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>

Acked-by: Song Liu <songliubraving@fb.com>

with a few nits:

> ---
> include/linux/bpf_lsm.h        |  23 +++
> include/linux/bpf_types.h      |   1 +
> include/uapi/linux/bpf.h       |  39 ++++
> kernel/bpf/Makefile            |   1 +
> kernel/bpf/bpf_lsm.c           |   4 +
> kernel/bpf/bpf_task_storage.c  | 313 +++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c           |   3 +-
> kernel/bpf/verifier.c          |  10 ++
> security/bpf/hooks.c           |   2 +
> tools/include/uapi/linux/bpf.h |  39 ++++
> 10 files changed, 434 insertions(+), 1 deletion(-)
> create mode 100644 kernel/bpf/bpf_task_storage.c
> 
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index aaacb6aafc87..326cb68a3632 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -7,6 +7,7 @@
> #ifndef _LINUX_BPF_LSM_H
> #define _LINUX_BPF_LSM_H
> 
> +#include "linux/sched.h"

vscode?

> #include <linux/bpf.h>
> #include <linux/lsm_hooks.h>
> 
> @@ -35,9 +36,21 @@ static inline struct bpf_storage_blob *bpf_inode(
> 	return inode->i_security + bpf_lsm_blob_sizes.lbs_inode;
> }

[...]

> index 000000000000..f5ed5eedc532
> --- /dev/null
> +++ b/kernel/bpf/bpf_task_storage.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Facebook

nit: I guess we shouldn't say 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>

[...]

> +}
> +
> +BPF_CALL_2(bpf_task_storage_delete, struct bpf_map *, map, struct task_struct *,
> +	   task)
> +{
> +	/* This helper must only called from where the task is guaranteed
> +	 * to have a refcount and cannot be freed.
> +	 */
> +	return task_storage_delete(task, map);
> +}
> +
> +static int notsupp_get_next_key(struct bpf_map *map, void *key, void *next_key)
> +{
> +	return -ENOTSUPP;
> +}

This is the third copy of notsupp_get_next_key(). We can probably move it to bpf.h. 

[...]