From: Eduard Zingerman <eddyz87@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
daniel@iogearbox.net, kernel-team@fb.com, yhs@fb.com
Subject: Re: [PATCH bpf-next v2 0/2] bpf: Fix to preserve reg parent/live fields when copying range info
Date: Fri, 13 Jan 2023 22:02:04 +0200 [thread overview]
Message-ID: <e64e8dbea359c1e02b7c38724be72f354257c2f6.camel@gmail.com> (raw)
In-Reply-To: <CAEf4BzYoB8Ut7UM62dw6TquHfBMAzjbKR=aG_c74XaCgYYyikg@mail.gmail.com>
On Wed, 2023-01-11 at 16:24 -0800, Andrii Nakryiko wrote:
[...]
>
> I'm wondering if we should consider allowing uninitialized
> (STACK_INVALID) reads from stack, in general. It feels like it's
> causing more issues than is actually helpful in practice. Common code
> pattern is to __builtin_memset() some struct first, and only then
> initialize it, basically doing unnecessary work of zeroing out. All
> just to avoid verifier to complain about some irrelevant padding not
> being initialized. I haven't thought about this much, but it feels
> that STACK_MISC (initialized, but unknown scalar value) is basically
> equivalent to STACK_INVALID for all intents and purposes. Thoughts?
Do you have an example of the __builtin_memset() usage?
I tried passing partially initialized stack allocated structure to
bpf_map_update_elem() and bpf_probe_write_user() and verifier did not
complain.
Regarding STACK_MISC vs STACK_INVALID, I think it's ok to replace
STACK_INVALID with STACK_MISC if we are talking about STX/LDX/ALU
instructions because after LDX you would get a full range register and
you can't do much with a full range value. However, if a structure
containing un-initialized fields (e.g. not just padding) is passed to
a helper or kfunc is it an error?
> Obviously, this is a completely separate change and issue from what
> you are addressing in this patch set.
>
> Awesome job on tracking this down and fixing it! For the patch set:
Thank you for reviewing this issue with me.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>
[...]
next prev parent reply other threads:[~2023-01-13 20:02 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-06 14:22 [PATCH bpf-next v2 0/2] bpf: Fix to preserve reg parent/live fields when copying range info Eduard Zingerman
2023-01-06 14:22 ` [PATCH bpf-next v2 1/2] " Eduard Zingerman
2023-01-06 14:22 ` [PATCH bpf-next v2 2/2] selftests/bpf: Verify copy_register_state() preserves parent/live fields Eduard Zingerman
2023-01-12 0:24 ` [PATCH bpf-next v2 0/2] bpf: Fix to preserve reg parent/live fields when copying range info Andrii Nakryiko
2023-01-13 20:02 ` Eduard Zingerman [this message]
2023-01-13 22:22 ` Andrii Nakryiko
2023-01-14 0:10 ` Eduard Zingerman
2023-01-14 1:17 ` Andrii Nakryiko
2023-01-14 1:30 ` Andrii Nakryiko
2023-01-19 23:52 ` Alexei Starovoitov
2023-01-20 0:16 ` Alexei Starovoitov
2023-01-30 15:33 ` Eduard Zingerman
2023-01-31 1:17 ` Andrii Nakryiko
2023-01-31 2:42 ` Alexei Starovoitov
2023-01-31 8:29 ` Eduard Zingerman
2023-01-31 18:55 ` Andrii Nakryiko
2023-01-20 13:39 ` Eduard Zingerman
2023-01-19 23:30 ` patchwork-bot+netdevbpf
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e64e8dbea359c1e02b7c38724be72f354257c2f6.camel@gmail.com \
--to=eddyz87@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox