Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201027170317.2011119-1-kpsingh@chromium.org>
 <20201027170317.2011119-2-kpsingh@chromium.org> <20201028011321.4yu62347lfzisxwy@kafai-mbp>
In-Reply-To: <20201028011321.4yu62347lfzisxwy@kafai-mbp>
From:   KP Singh <kpsingh@chromium.org>
Date:   Fri, 30 Oct 2020 11:53:00 +0100
Message-ID: <CACYkzJ5VU2Pd2ZiY7AKJM0yZ2NsDbQOu1Y_FYwkBv6M6NFvkcw@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Implement task local storage
To:     Martin KaFai Lau <kafai@fb.com>
Cc:     open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
        Hao Luo <haoluo@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks for taking a look!

On Wed, Oct 28, 2020 at 2:13 AM Martin KaFai Lau <kafai@fb.com> 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,
> > +};
> > +