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

[KP Singh]

On Fri, Oct 30, 2020 at 12:12 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Oct 28, 2020 at 9:17 AM KP Singh <kpsingh@xxxxxxxxxxxx> wrote:
> >
> > From: KP Singh <kpsingh@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
>
> 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);
> > +}
> > +
>
> [...]