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

[KP Singh]

Thanks for taking a look!

On Wed, Oct 28, 2020 at 2:13 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> 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?

Nope. Removed.

>
> > +#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)

[...]

> > +             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?

I would assume that holding a reference to the file should also
keep the private_data alive but I was not sure so I grabbed the
extra reference.

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

If we keep using get_pid_task then, yes, I see it grabs a reference to the task.
We could also call pid_task under rcu locks but it might be cleaner to
just get_pid_task
and put_task_struct().

>
> > +     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);

[...]

> > +     .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.

Will do. Thanks for the nudge :)

>
> > +     .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,
> > +};
> > +