On Fri, Aug 9, 2024 at 10:29 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
This is probably a deficiency in clang, sparse nicely explains the
reason for intermediate cast:
-q-
Sparse treats pointers with different address spaces as distinct types
and will warn on casts (implicit or explicit) mixing the address
spaces. An exception to this is when the destination type is uintptr_t
or unsigned long since the resulting integer value is independent of
the address space and can’t be dereferenced without first casting it
back to a pointer type.
-/q-
>
>
> [...]
>
> > > > 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.
OK, will do in a v2 patch, together with your other suggestion.
Thanks,
Uros.
