Re: [PATCH v2 bpf-next] bpf: fix uninitialized values in BPF_{CORE,PROBE}_READ

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/04/30 09:34AM, Alexei Starovoitov wrote:
> On Wed, Apr 30, 2025 at 9:29 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > On 25/04/30 09:00AM, Andrii Nakryiko wrote:
> > > On Tue, Apr 29, 2025 at 7:19 AM Anton Protopopov
> > > <a.s.protopopov@gmail.com> wrote:
> > > >
> > > > With the latest LLVM bpf selftests build will fail with
> > > > the following error message:
> > > >
> > > >     progs/profiler.inc.h:710:31: error: default initialization of an object of type 'typeof ((parent_task)->real_cred->uid.val)' (aka 'const unsigned int') leaves the object uninitialized and is incompatible with C++ [-Werror,-Wdefault-const-init-unsafe]
> > >
> > > this is BPF-side code, what does C++ have to do with this, I'm confused...
> >
> > This I am not sure about why exactly, but clang (wihout ++) emits this warning
> > now (try smth like `clang -c -x c - <<<'void foo(void) {const int x;}'`).
> > When sending patch, I though that CORE* macros also can be used by ++ progs.
> > For C, maybe, this is a problem with clang that it enables -Wdefault-const-init-unsafe?
> >
> > >
> > > Also, why using __u8[] is suddenly ok, and using the actual type
> > > isn't? Eventually it all is initialized by bpf_probe_read_kernel(), so
> > > compiler is wrong or I am misunderstanding something... Can you please
> > > help me understand this?
> >
> > So, when a const sneaks in, one have BPF_CORE_READ expanded into
> > say smth like this:
> >
> >     ({
> >     typeof(((parent_task)->real_cred->uid.val)) __r;
> >     BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__);
> >     __r;
> >     })
> >
> > It happens that real_cred is a pointer to const, so __r becomes const,
> > and thus the warning (if enabled) is legit.
> >
> > With __u8 this turns into (let T = typeof(((parent_task)->real_cred->uid.val)))
> >
> >     ({
> >     __u8 __r[sizeof(T)];
> >     BPF_CORE_READ_INTO(&__r, (src), a, ##__VA_ARGS__);
> >     * (T *) __r;
> >     })
> >
> > So here we do not care if T is const or not, as __r is not in any case.
> 
> The problem with __u8 approach is that it's losing alignment.
> Have you tried typeof_unqual instead ?
> Modern gcc and clang support it directly.

Yes, thanks, typeof_unqual should work.

> For older compilers we have __unqual_typeof() in bpf_atomic.h

Older compilers aren't a problem, as build only fails on modern
(LLVM added -Wdefault-const-init-unsafe for C just a few days ago).