On Fri, 2024-08-09 at 12:15 +0200, Uros Bizjak wrote:
> On Fri, Aug 9, 2024 at 10:28 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx/
> [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.
[...]
