On Wed, 7 Sept 2022 at 21:00, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sun, Sep 04, 2022 at 10:41:18PM +0200, Kumar Kartikeya Dwivedi wrote:
> > Enable support for kptrs in local storage maps by wiring up the freeing
> > of these kptrs from map value.
> >
> > Cc: Martin KaFai Lau <kafai@xxxxxx>
> > Cc: KP Singh <kpsingh@xxxxxxxxxx>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > include/linux/bpf_local_storage.h | 2 +-
> > kernel/bpf/bpf_local_storage.c | 33 +++++++++++++++++++++++++++----
> > kernel/bpf/syscall.c | 5 ++++-
> > kernel/bpf/verifier.c | 9 ++++++---
> > 4 files changed, 40 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> > index 7ea18d4da84b..6786d00f004e 100644
> > --- a/include/linux/bpf_local_storage.h
> > +++ b/include/linux/bpf_local_storage.h
> > @@ -74,7 +74,7 @@ struct bpf_local_storage_elem {
> > struct hlist_node snode; /* Linked to bpf_local_storage */
> > struct bpf_local_storage __rcu *local_storage;
> > struct rcu_head rcu;
> > - /* 8 bytes hole */
> > + struct bpf_map *map; /* Only set for bpf_selem_free_rcu */
> > /* The data is stored in another cacheline to minimize
> > * the number of cachelines access during a cache hit.
> > */
> > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > index 802fc15b0d73..4a725379d761 100644
> > --- a/kernel/bpf/bpf_local_storage.c
> > +++ b/kernel/bpf/bpf_local_storage.c
> > @@ -74,7 +74,8 @@ bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
> > gfp_flags | __GFP_NOWARN);
> > if (selem) {
> > if (value)
> > - memcpy(SDATA(selem)->data, value, smap->map.value_size);
> > + copy_map_value(&smap->map, SDATA(selem)->data, value);
> > + /* No call to check_and_init_map_value as memory is zero init */
> > return selem;
> > }
> >
> > @@ -92,12 +93,27 @@ void bpf_local_storage_free_rcu(struct rcu_head *rcu)
> > kfree_rcu(local_storage, rcu);
> > }
> >
> > +static void check_and_free_fields(struct bpf_local_storage_elem *selem)
> > +{
> > + if (map_value_has_kptrs(selem->map))
> > + bpf_map_free_kptrs(selem->map, SDATA(selem));
> > +}
> > +
> > static void bpf_selem_free_rcu(struct rcu_head *rcu)
> > {
> > struct bpf_local_storage_elem *selem;
> >
> > selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
> > - kfree_rcu(selem, rcu);
> > + check_and_free_fields(selem);
> > + kfree(selem);
> > +}
> > +
> > +static void bpf_selem_free_tasks_trace_rcu(struct rcu_head *rcu)
> > +{
> > + struct bpf_local_storage_elem *selem;
> > +
> > + selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
> > + call_rcu(&selem->rcu, bpf_selem_free_rcu);
> > }
> >
> > /* local_storage->lock must be held and selem->local_storage == local_storage.
> > @@ -150,10 +166,11 @@ bool bpf_selem_unlink_storage_nolock(struct bpf_local_storage *local_storage,
> > SDATA(selem))
> > RCU_INIT_POINTER(local_storage->cache[smap->cache_idx], NULL);
> >
> > + selem->map = &smap->map;
> > if (use_trace_rcu)
> > - call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_rcu);
> > + call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_tasks_trace_rcu);
> > else
> > - kfree_rcu(selem, rcu);
> > + call_rcu(&selem->rcu, bpf_selem_free_rcu);
> >
> > return free_local_storage;
> > }
> > @@ -581,6 +598,14 @@ void bpf_local_storage_map_free(struct bpf_local_storage_map *smap,
> > */
> > synchronize_rcu();
> >
> > + /* When local storage map has kptrs, the call_rcu callback accesses
> > + * kptr_off_tab, hence we need the bpf_selem_free_rcu callbacks to
> > + * finish before we free it.
> > + */
> > + if (map_value_has_kptrs(&smap->map)) {
> > + rcu_barrier();
> > + bpf_map_free_kptr_off_tab(&smap->map);
>
> probably needs conditional rcu_barrier_tasks_trace before rcu_barrier?
> With or without it will be a significant delay in map freeing.
> Maybe we should generalize the destroy_mem_alloc trick?
>
Yes, let me take a closer look tomorrow and ask questions if any.
Otherwise I will rework it. Thanks for catching this.
> Patch 4 needs rebase. Applied patches 1-3.
> The first 5 look great to me.
> Pls follow up with kptr specific tests.
Thanks, I will split those out into another series with its own test.
