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> <CAEf4BzatLFGpht-CiSmOfSjBY_nATZsgnWhLnUFuDgvMi4yXLw@mail.gmail.com>
In-Reply-To: <CAEf4BzatLFGpht-CiSmOfSjBY_nATZsgnWhLnUFuDgvMi4yXLw@mail.gmail.com>
From:   KP Singh <kpsingh@chromium.org>
Date:   Fri, 30 Oct 2020 12:02:11 +0100
Message-ID: <CACYkzJ6iEVQ1GNexBH58jtPrtwzrYWi87sj_jsEaGuWypxbA4w@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Implement task local storage
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
        Hao Luo <haoluo@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 30, 2020 at 12:12 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 28, 2020 at 9:17 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>
> > ---
>
> Please also double-check all three of get_pid_task() uses, you need to
> put_task_struct() in all cases.

Done, Martin also pointed it out.

>
> >  include/linux/bpf_lsm.h                       |  23 ++
> >  include/linux/bpf_types.h                     |   1 +
> >  include/uapi/linux/bpf.h                      |  39 +++
> >  kernel/bpf/Makefile                           |   1 +

[...]

>
> > + *
> > + * int bpf_task_storage_delete(struct bpf_map *map, void *task)
>
> please use long for return type, as all other helpers (except
> bpf_inode_storage_delete, which would be nice to fix as well) do.

Done. Will also fix the return value of bpf_inode_storage_delete in a
separate patch.

>
> > + *     Description
> > + *             Delete a bpf_local_storage from a *task*.
> > + *     Return
> > + *             0 on success.

[...]

> > +               return;
> > +       }
> > +
> > +       /* Netiher the bpf_prog nor the bpf-map's syscall
>
> typo: Neither

Thanks. Fixed.

>
> > +        * could be modifying the local_storage->list now.
> > +        * Thus, no elem can be added-to or deleted-from the
> > +        * local_storage->list by the bpf_prog or by the bpf-map's syscall.
> > +        *
> > +        * It is racing with bpf_local_storage_map_free() alone
> > +        * when unlinking elem from the local_storage->list and
> > +        * the map's bucket->list.
> > +        */
> > +       raw_spin_lock_bh(&local_storage->lock);
> > +       hlist_for_each_entry_safe(selem, n, &local_storage->list, snode) {
> > +               /* Always unlink from map before unlinking from
> > +                * local_storage.
> > +                */
> > +               bpf_selem_unlink_map(selem);
> > +               free_task_storage = bpf_selem_unlink_storage_nolock(
> > +                       local_storage, selem, false);
>
> this will override the previous value of free_task_storage. Did you
> intend to do || here?

in bpf_selem_unlink_storage_nolock:

  free_local_storage = hlist_is_singular_node(&selem->snode,
  &local_storage->list);

free_local_storage is only true when the linked list has one element, so it does
not really matter. I guess we could use the "||" here for correctness, and if
we do that, we should also update the other local storages.

>
> > +       }
> > +       raw_spin_unlock_bh(&local_storage->lock);
> > +       rcu_read_unlock();
> > +
> > +       /* free_task_storage should always be true as long as
> > +        * local_storage->list was non-empty.
> > +        */
> > +       if (free_task_storage)
> > +               kfree_rcu(local_storage, rcu);
> > +}
> > +
>
> [...]