BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	"Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] bpf: queue_stack_maps: Replace zero-length array with flexible-array member
Date: Tue, 18 Feb 2020 15:53:14 +0100
Message-ID: <8c212e06-dc1f-3a47-ba3e-8f408ba7a972@iogearbox.net> (raw)
In-Reply-To: <CAEf4Bzae=Afp_4FGgeFy+=kk64nm1vhRso2zF3j7Qdst66RFZw@mail.gmail.com>

On 2/13/20 6:47 PM, Andrii Nakryiko wrote:
> On Thu, Feb 13, 2020 at 7:22 AM Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
>> The current codebase makes use of the zero-length array language
>> extension to the C90 standard, but the preferred mechanism to declare
>> variable-length types such as these ones is a flexible array member[1][2],
>> introduced in C99:
>> struct foo {
>>          int stuff;
>>          struct boo array[];
>> };
>> By making use of the mechanism above, we will get a compiler warning
>> in case the flexible array does not occur last in the structure, which
>> will help us prevent some kind of undefined behavior bugs from being
>> inadvertently introduced[3] to the codebase from now on.
>> Also, notice that, dynamic memory allocations won't be affected by
>> this change:
>> "Flexible array members have incomplete type, and so the sizeof operator
>> may not be applied. As a quirk of the original implementation of
>> zero-length arrays, sizeof evaluates to zero."[1]
>> This issue was found with the help of Coccinelle.
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>> [2] https://github.com/KSPP/linux/issues/21
>> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Sure, why not, though I don't think that's the only one (e.g.,
> bpf_storage_buffer's data is zero-length as well).
> Acked-by: Andrii Nakryiko <andriin@fb.com>

+1, Gustavo, there are several such instances in the whole BPF subsystem. Please combine
them all into a single patch, including https://patchwork.ozlabs.org/patch/1239563/, and


      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13 15:24 Gustavo A. R. Silva
2020-02-13 17:47 ` Andrii Nakryiko
2020-02-18 14:53   ` Daniel Borkmann [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8c212e06-dc1f-3a47-ba3e-8f408ba7a972@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=gustavo@embeddedor.com \
    --cc=kafai@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git