MIME-Version: 1.0
References: <20230204183241.never.481-kees@kernel.org> <CAKH8qBvqLeR3Wsbpb-v=EUY=Bw0jCP2OAaBn4tOqGmA1AqBZbA@mail.gmail.com>
 <63e14abb.170a0220.ca425.b7bc@mx.google.com> <CAKH8qBtX0HU4_YtnZ3hU4NhGHSQ9VU70niXFFoqf3k67a1+6aA@mail.gmail.com>
 <63e5210b.630a0220.c17be.27ff@mx.google.com>
In-Reply-To: <63e5210b.630a0220.c17be.27ff@mx.google.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Thu, 9 Feb 2023 08:55:23 -0800
Message-ID: <CAADnVQK-sW51SsC7FYDqLaO8c5xj=MAgcu_6p8L-JN9kAsrzeA@mail.gmail.com>
Subject: Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with
 flexible array
To:     Kees Cook <keescook@chromium.org>
Cc:     Stanislav Fomichev <sdf@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Hao Luo <haoluo@google.com>,
        Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>,
        Shuah Khan <shuah@kernel.org>,
        Haowen Bai <baihaowen@meizu.com>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 9, 2023 at 8:36 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Feb 06, 2023 at 11:17:06AM -0800, Stanislav Fomichev wrote:
> > It's my understanding that it's the intended use-case. Users are
> > expected to use this struct as a header; at least we've been using it
> > that way :-)
> >
> > For me, both return the same:
> > sizeof(struct { __u32 prefix; __u8 data[0]; })
> > sizeof(struct { __u32 prefix; __u8 data[]; })
> >
> > So let's do s/data[0]/data[]/ in the UAPI only? What's wrong with
> > using this struct as a header?
>
> For the whole struct, yup, the above sizeof()s are correct. However:
>
> sizeof(foo->data) == 0             // when data[0]
> sizeof(foo->data) == compile error // when data[]
>
> The [0]-array GNU extension doesn't have consistent behavior, so it's
> being removed from the kernel in favor of the proper C99 [] flexible
> arrays, so we can enable -fstrict-flex-arrays=3 to remove all the
> ambiguities with array bounds:
> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> https://people.kernel.org/kees/bounded-flexible-arrays-in-c
>
> As a header, this kind of overlap isn't well supported. Clang already
> warns, and GCC is going to be removing support for overlapping composite
> structs with a flex array in the middle (and also warns under -pedantic):
> https://godbolt.org/z/vWzqs41h6
>
> I talk about dealing with these specific cases in my recent write-up
> on array bounds checking -- see "Overlapping composite structure members"
> in the people.kernel.org post above.
>
> > > Perhaps better might be:
> > >
> > > struct bpf_lpm_trie_key {
> > >     __u32   prefixlen;      /* up to 32 for AF_INET, 128 for AF_INET6 */
> > > };
> > >
> > > struct bpf_lpm_trie_key_raw {
> > >     struct bpf_lpm_trie_key_prefix prefix;
> > >     u8 data[];
> > > };
> > >
> > > struct my_key {
> > >     struct bpf_lpm_trie_key_prefix prefix;
> > >     int a, b, c;
> > > };
>
> This approach is, perhaps, the best way to go? Besides the selftest,
> what things in userspace consumes struct bpf_lpm_trie_key?

Plenty of bpf progs use it:
https://github.com/cilium/cilium/blob/master/bpf/lib/common.h#L352