Re: [PATCH bpf-next v2 05/15] bpf: Allow storing percpu kptr in map

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Sun, Mar 20, 2022 at 02:56:20AM IST, Alexei Starovoitov wrote:
> On Sun, Mar 20, 2022 at 12:34:09AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sun, Mar 20, 2022 at 12:00:28AM IST, Alexei Starovoitov wrote:
> > > On Thu, Mar 17, 2022 at 05:29:47PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > Make adjustments to the code to allow storing percpu PTR_TO_BTF_ID in a
> > > > map. Similar to 'kptr_ref' tag, a new 'kptr_percpu' allows tagging types
> > > > of pointers accepting stores of such register types. On load, verifier
> > > > marks destination register as having type PTR_TO_BTF_ID | MEM_PERCPU |
> > > > PTR_MAYBE_NULL.
> > > >
> > > > Cc: Hao Luo <haoluo@google.com>
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  include/linux/bpf.h   |  3 ++-
> > > >  kernel/bpf/btf.c      | 13 ++++++++++---
> > > >  kernel/bpf/verifier.c | 26 +++++++++++++++++++++-----
> > > >  3 files changed, 33 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 702aa882e4a3..433f5cb161cf 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -161,7 +161,8 @@ enum {
> > > >  };
> > > >
> > > >  enum {
> > > > -	BPF_MAP_VALUE_OFF_F_REF = (1U << 0),
> > > > +	BPF_MAP_VALUE_OFF_F_REF    = (1U << 0),
> > > > +	BPF_MAP_VALUE_OFF_F_PERCPU = (1U << 1),
> > >
> > > What is the use case for storing __percpu pointer into a map?
> >
> > No specific use case for me, just thought it would be useful, especially now
> > that __percpu tag is understood by verifier for kernel BTF, so it may also refer
> > to dynamically allocated per-CPU memory, not just global percpu variables. But
> > fine with dropping both this and user kptr if you don't feel like keeping them.
>
> I prefer to drop it for now.
> The patch is trivial but kptr_percpu tag would stay forever.

Ok, I'll drop both this and user kptr for now.

> Maybe we can allow storing percpu pointers in a map with just kptr tag.
> The verifier should be able to understand from btf whether that pointer
> is percpu or not.

This won't work (unless I missed something), it is possible to see the type when
a store is being done, but we cannot know whether the pointer was percpu or not
when doing a load (which is needed to decide whether it will be marked with
MEM_PERCPU, so that user has to call bpf_this_cpu_ptr or bpf_per_cpu_ptr to
obtain actual pointer). So some extra tagging is needed.

--
Kartikeya