Re: [PATCH] bpf: Fix percpu address space issues

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-08-09 at 12:15 +0200, Uros Bizjak wrote:
> On Fri, Aug 9, 2024 at 10:28 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Sun, 2024-08-04 at 20:55 +0200, Uros Bizjak wrote:
> > 
> > [...]
> > 
> > > Found by GCC's named address space checks.
> > 
> > Please provide some additional details.
> > I assume that the definition of __percpu was changed from
> > __attribute__((btf_type_tag(percpu))) to
> > __attribute__((address_space(??)), is that correct?
> 
> This is correct. The fixes in the patch are based on the patch series
> [1] that enable strict percpu check via GCC's x86 named address space
> qualifiers, and in its RFC state hacks __seg_gs into the __percpu
> qualifier (as can be seen in the 3/3 patch). The compiler will detect
> pointer address space mismatches for e.g.:
> 
> --cut here--
> int __seg_gs m;
> 
> int *foo (void) { return &m; }
> --cut here--
> 
> v.c: In function ‘foo’:
> v.c:5:26: error: return from pointer to non-enclosed address space
>    5 | int *foo (void) { return &m; }
>      |                          ^~
> v.c:5:26: note: expected ‘int *’ but pointer is of type ‘__seg_gs int *’
> 
> and expects explicit casts via uintptr_t when these casts are really
> intended ([2], please also see [3] for similar sparse requirement):
> 
> int *foo (void) { return (int *)(uintptr_t)&m; }
> 
> [1] https://lore.kernel.org/lkml/20240805184012.358023-1-ubizjak@gmail.com/
> [2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces
> [3] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name

Understood, thank you for the details.
Interestingly, clang does not require (uintptr_t) intermediate cast, e.g.:

    $ cat test.c
    #define __as(N) __attribute__((address_space(N)))

    void *foo(void __as(1)* x) { return x; }         // error
    void *bar(void __as(1)* x) { return (void *)x; } // fine

    $ clang -o /dev/null -c test.c
    test.c:3:37: error: returning '__as(1) void *' from a function with result type 'void *' changes address space of pointer
        3 | void *foo(void __as(1)* x) { return x; }         // error
          |                                     ^
    1 error generated.
    

[...]

> > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> > > index 188e3c2effb2..544ca433275e 100644
> > > --- a/kernel/bpf/arraymap.c
> > > +++ b/kernel/bpf/arraymap.c
> > > @@ -600,7 +600,7 @@ static void *bpf_array_map_seq_start(struct seq_file *seq, loff_t *pos)
> > >       array = container_of(map, struct bpf_array, map);
> > >       index = info->index & array->index_mask;
> > >       if (info->percpu_value_buf)
> > > -            return array->pptrs[index];
> > > +            return array->ptrs[index];
> > 
> > I disagree with this change.
> > One might say that indeed the address space is cast away here,
> > however, value returned by this function is only used in functions
> > bpf_array_map_seq_{next,show,stop}(), where it is guarded by the same
> > 'if (info->percpu_value_buf)' condition to identify if per_cpu_ptr()
> > is necessary.
> 
> If this is the case, you have to inform the compiler that address
> space is cast away with explicit (void *)(uintptr_t) cast, placed
> before return. But looking at the union with ptrs and pptrs members,
> it looked to me that it is just the case of wrong union member
> accessed.

I'd say it's better to use pptr and add a cast in this case.

[...]

> > > @@ -632,7 +632,7 @@ static int __bpf_array_map_seq_show(struct seq_file *seq, void *v)
> > >       struct bpf_iter_meta meta;
> > >       struct bpf_prog *prog;
> > >       int off = 0, cpu = 0;
> > > -     void __percpu **pptr;
> > > +     void * __percpu *pptr;
> > 
> > Should this be 'void __percpu *pptr;?
> > The value comes from array->pptrs[*] field,
> > which has the above type for elements.
> 
> I didn't want to introduce semantic changes, so I have just changed
> the base type fo __percpu one, due to:
> 
> per_cpu_ptr(pptr, cpu));
> 
> later in the code.

There would be no semantic changes if type of pptr is changed to 'void __percpu *'.

[...]

> > > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > > index be1f64c20125..a49212bbda09 100644
> > > --- a/kernel/bpf/hashtab.c
> > > +++ b/kernel/bpf/hashtab.c
> > > @@ -1049,14 +1049,14 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
> > >                       pptr = htab_elem_get_ptr(l_new, key_size);
> > >               } else {
> > >                       /* alloc_percpu zero-fills */
> > > -                     pptr = bpf_mem_cache_alloc(&htab->pcpu_ma);
> > > -                     if (!pptr) {
> > > +                     void *ptr = bpf_mem_cache_alloc(&htab->pcpu_ma);
> > > +                     if (!ptr) {
> > 
> > Why adding an intermediate variable here?
> 
> Mainly to avoid several inter-as casts, because l_new->ptr_to_pptr
> also expects assignment from generic address space.

Ok, makes sense.

[...]

> > > diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> > > index dec892ded031..b3858a76e0b3 100644
> > > --- a/kernel/bpf/memalloc.c
> > > +++ b/kernel/bpf/memalloc.c
> > > @@ -138,8 +138,8 @@ static struct llist_node notrace *__llist_del_first(struct llist_head *head)
> > >  static void *__alloc(struct bpf_mem_cache *c, int node, gfp_t flags)
> > >  {
> > >       if (c->percpu_size) {
> > > -             void **obj = kmalloc_node(c->percpu_size, flags, node);
> > > -             void *pptr = __alloc_percpu_gfp(c->unit_size, 8, flags);
> > > +             void __percpu **obj = kmalloc_node(c->percpu_size, flags, node);
> > 
> > Why __percpu is needed for obj?
> 
> The new declaration declares "void pointer to percpu pointer", it is
> needed because some lines below we have:
> 
>         obj[1] = pptr;

Oh, right.

[...]