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