From: Eduard Zingerman <eddyz87@gmail.com>
To: KP Singh <kpsingh@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Matt Bobrowski <mattbobrowski@google.com>,
bpf@vger.kernel.org, andrii@kernel.org, acme@redhat.com,
dwarves@vger.kernel.org
Subject: Re: bpf: Question about odd BPF verifier behaviour
Date: Tue, 28 Feb 2023 20:08:44 +0200 [thread overview]
Message-ID: <9f843ed8635b8d0f7bb61d0c2ee3f91970863413.camel@gmail.com> (raw)
In-Reply-To: <CACYkzJ4yR0n+kP9-G025uP2fwhyBh9DF2feA+H5mw+Re6GdkFQ@mail.gmail.com>
On Tue, 2023-02-28 at 03:55 +0100, KP Singh wrote:
> On Mon, Feb 27, 2023 at 9:48 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > On Mon, 2023-02-27 at 11:31 -0800, Andrii Nakryiko wrote:
> > [...]
> > > > > I'd start with understanding what BTF and DWARF differences are
> > > > > causing the issue before trying to come up with the fix. For that we
> > > > > don't even need config or repro steps, it should be enough to share
> > > > > vmlinux with BTF and DWARF, and start from there.
> > > > >
> > > >
> > > > Yes, I suspect that there is some kind of unanticipated
> > > > anomaly for some DWARF encoding for some kind of objects,
> > > > just need to find the root for the diverging type hierarchies.
> > > >
> > > > > But I'm sure Eduard is on top of this already (especially that he can
> > > > > repro the issue now).
> > > >
> > > > I'm working on it, nothing to report yet, but I'm working on it.
> > > >
> > >
> > > Thanks, please keep us posted!
> >
> > It is interesting how everything is interconnected. The patch for
> > pahole below happens to help. I prepared it last week while working on
> > new DWARF encoding scheme for btf_type_tag.
> >
> > I still need to track down which "unspecified_type" entries caused the
> > issue in this particular case. Will post an update tomorrow.
> >
> > Meanwhile, Matt, KP, could you please verify the patch on your side?
> > It is for the "next" branch of pahole.
> >
> > ---
> >
> > From 09fac63ca08e25aea499f827283b07cc87a7daab Mon Sep 17 00:00:00 2001
> > From: Eduard Zingerman <eddyz87@gmail.com>
> > Date: Tue, 21 Feb 2023 19:23:00 +0200
> > Subject: [PATCH] dwarf_loader: Fix for BTF id drift caused by adding
> > unspecified types
> >
> > Recent changes to handle unspecified types (see [1]) cause BTF ID drift.
> >
> > Specifically, the intent of commits [2], [3] and [4] is to render
> > references to unspecified types as void type.
> > However, as a consequence:
> > - in `die__process_unit()` call to `cu__add_tag()` allocates `small_id`
> > for unspecified type tags and adds these tags to `cu->types_table`;
> > - `btf_encoder__encode_tag()` skips generation of BTF entries for
> > `DW_TAG_unspecified_type` tags.
> >
> > Such logic causes ID drift if unspecified type is not the last type
> > processed for compilation unit. `small_id` of each type following
> > unspecified type in the `cu->types_table` would have its BTF id off by -1.
> > Thus renders references established on recode phase invalid.
> >
> > This commit reverts `unspecified_type` id/tag tracking, instead:
> > - `small_id` for unspecified type tags is set to 0, thus reference to
> > unspecified type tag would render BTF id of a `void` on recode phase;
> > - unspecified type tags are not added to `cu->types_table`.
> >
> > [1] https://lore.kernel.org/all/Y0R7uu3s%2FimnvPzM@kernel.org/
> > [2] bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void")
> > [3] cffe5e1f75e1 ("core: Record if a CU has a DW_TAG_unspecified_type")
> > [4] 75e0fe28bb02 ("core: Add DW_TAG_unspecified_type to tag__is_tag_type() set")
> >
> > Fixes: bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void")
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>
> Tested-by: KP Singh <kpsingh@kernel.org>
> Reported-by: Matt Bobrowski <mattbobrowski@google.com>
>
> Thank you so much Eduard, this worked:
>
> * No duplicate BTF ID warnings
> * No 15 minute BTF ID generation
> * Matt's reproducer loads successfully.
>
> I had a sneaky suspicion that it was these unspecified types, which is
> why my hacky patch which got unspecified types out of the way got
> things to *mostly* work.
Hi KP,
Thanks a lot for testing!
I found the root cause for the bug (took me longer than I would like
to admit...). Using the patch below the reproducer from Matt works as
expected and warnings are gone.
Still, I think that my patch from yesterday is a more general
approach, as it correctly handles unspecified types that occur in
non-tail position, so I'll post that one.
Thanks,
Eduard
---
From daa53248e8a5087edbceaffe1fad51f9eb06e922 Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@gmail.com>
Date: Tue, 28 Feb 2023 19:44:22 +0200
Subject: [PATCH] btf_encoder: reset encoder->unspecified_type for each CU
The field `encoder->unspecified_type` is set but not reset by function
`btf_encoder__encode_cu()` when processed `cu` has unspecified type.
The following sequence of events might occur when BTF encoding is
requested:
- CU with unspecified type is processed:
- unspecified type id is 42
- encoder->unspecified_type is set to 42
- CU without unspecified type is processed next using the same
`encoder` object:
- some `struct foo` has id 42 in this CU
- the references to `struct foo` are set 0 by function
`btf_encoder__tag_type()`.
This commit sets `encoder->unspecified_type` to 0 when CU does not
have unspecified type.
This issue was reported in thread [1].
See also [2].
[1] https://lore.kernel.org/bpf/Y%2FP1yxAuV6Wj3A0K@google.com/
[2] https://lore.kernel.org/all/Y0R7uu3s%2FimnvPzM@kernel.org/
Fixes: 52b25808e44a ("btf_encoder: Store type_id_off, unspecified type in encoder")
Reported-by: Matt Bobrowski <mattbobrowski@google.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
btf_encoder.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/btf_encoder.c b/btf_encoder.c
index da776f4..24f4c65 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1748,6 +1748,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
encoder->type_id_off = btf__type_cnt(encoder->btf) - 1;
if (encoder->cu->unspecified_type.tag)
encoder->unspecified_type = encoder->cu->unspecified_type.type;
+ else
+ encoder->unspecified_type = 0;
if (!encoder->has_index_type) {
/* cu__find_base_type_by_name() takes "type_id_t *id" */
--
2.39.1
next prev parent reply other threads:[~2023-02-28 18:08 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <Y/P1yxAuV6Wj3A0K@google.com>
[not found] ` <e9f7c86ff50b556e08362ebc0b6ce6729a2db7e7.camel@gmail.com>
[not found] ` <Y/czygarUnMnDF9m@google.com>
[not found] ` <17833347f8cec0e44d856aeafbb1bbe203526237.camel@gmail.com>
[not found] ` <Y/hLsgSO3B+2g9iF@google.com>
[not found] ` <8f8f9d43d2f3f6d19c477c28d05527250cc6213d.camel@gmail.com>
[not found] ` <Y/p0ryf5PcKIs7uj@google.com>
[not found] ` <c4cda35711804ec26ebaeedc07d10d5d51901495.camel@gmail.com>
[not found] ` <693dffd9571073a47820653fd2de863010491454.camel@gmail.com>
[not found] ` <CAEf4BzYaiD27y=Y85xhrj+VOvJY_5q1oVtg-4vYmFZFEpmW+nQ@mail.gmail.com>
[not found] ` <CACYkzJ7tgbJqwUVxfGd4UKDUcOQjK8zsbEKUUjV79=xHQn1fFg@mail.gmail.com>
[not found] ` <CAEf4BzZauF7V3pY1hgWgnJRN1F6eSkbTOTG3kM0c85uAX-vOfQ@mail.gmail.com>
[not found] ` <9ea9b52fca1300265ce5639a2da809813edb774f.camel@gmail.com>
[not found] ` <CAEf4BzYO+Rgcfbr+QzJ-8BdQg-x-mC6c4bOhA+Z4cvu_1ObX+g@mail.gmail.com>
2023-02-27 20:48 ` bpf: Question about odd BPF verifier behaviour Eduard Zingerman
2023-02-28 2:55 ` KP Singh
2023-02-28 18:08 ` Eduard Zingerman [this message]
2023-02-28 18:56 ` Andrii Nakryiko
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=9f843ed8635b8d0f7bb61d0c2ee3f91970863413.camel@gmail.com \
--to=eddyz87@gmail.com \
--cc=acme@redhat.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=dwarves@vger.kernel.org \
--cc=kpsingh@kernel.org \
--cc=mattbobrowski@google.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;
as well as URLs for NNTP newsgroup(s).