On Mon, Jul 29, 2024 at 8:48 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
>
> On Thu, Jun 27, 2024 at 7:08 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > The need to get ELF build ID reliably is an important aspect when
> > dealing with profiling and stack trace symbolization, and
> > /proc/<pid>/maps textual representation doesn't help with this.
> [...]
> > @@ -539,6 +543,21 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
> > }
> > }
> >
> > + if (karg.build_id_size) {
> > + __u32 build_id_sz;
> > +
> > + err = build_id_parse(vma, build_id_buf, &build_id_sz);
> > + if (err) {
> > + karg.build_id_size = 0;
> > + } else {
> > + if (karg.build_id_size < build_id_sz) {
> > + err = -ENAMETOOLONG;
> > + goto out;
> > + }
> > + karg.build_id_size = build_id_sz;
> > + }
> > + }
>
> The diff doesn't have enough context lines to see it here, but the two
> closing curly braces above are another copy of exactly the same code
> block from the preceding patch. The current state in mainline looks
> like this, with two repetitions of exactly the same block:
Yeah, you are right, thanks for the heads up! Seems like a rebase
screw up which duplicated build_id logic. It doesn't have any negative
effects besides doing the same work twice (if build ID parsing is
requested), but I'll definitely will send a fix to drop the
duplication.
>
> [...]
> karg.dev_minor = 0;
> karg.inode = 0;
> }
>
> if (karg.build_id_size) {
> __u32 build_id_sz;
>
> err = build_id_parse(vma, build_id_buf, &build_id_sz);
> if (err) {
> karg.build_id_size = 0;
> } else {
> if (karg.build_id_size < build_id_sz) {
> err = -ENAMETOOLONG;
> goto out;
> }
> karg.build_id_size = build_id_sz;
> }
> }
>
> if (karg.build_id_size) {
> __u32 build_id_sz;
>
> err = build_id_parse(vma, build_id_buf, &build_id_sz);
> if (err) {
> karg.build_id_size = 0;
> } else {
> if (karg.build_id_size < build_id_sz) {
> err = -ENAMETOOLONG;
> goto out;
> }
> karg.build_id_size = build_id_sz;
> }
> }
>
> if (karg.vma_name_size) {
> [...]
