On Fri, Oct 30, 2020 at 12:12 AM Andrii Nakryiko
<andrii.nakry...@gmail.com> wrote:
>
> On Wed, Oct 28, 2020 at 9:17 AM KP Singh <kpsi...@chromium.org> wrote:
> >
> > From: KP Singh <kpsi...@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 <kpsi...@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);
> > +}
> > +
>
> [...]
