linux-kernel - Re: [PATCH bpf-next 3/4] libbpf: Add support to parse extra info in usdt note record

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aSa6lMkV3VkIM95g@krava>
Date: Wed, 26 Nov 2025 09:30:12 +0100
From: Jiri Olsa <olsajiri@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	linux-kernel@...r.kernel.org, Song Liu <songliubraving@...com>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next 3/4] libbpf: Add support to parse extra info in
 usdt note record

On Mon, Nov 24, 2025 at 09:29:09AM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 17, 2025 at 12:36 AM Jiri Olsa <jolsa@...nel.org> wrote:
> >
> > Adding support to parse extra info in usdt note record that
> > indicates there's nop,nop5 emitted for probe.
> >
> > We detect this by checking extra zero byte placed in between
> > args zero termination byte and desc data end. Please see [1]
> > for more details.
> >
> > Together with uprobe syscall feature detection we can decide
> > if we want to place the probe on top of nop or nop5.
> >
> > [1] https://github.com/libbpf/usdt
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  tools/lib/bpf/usdt.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> > index c174b4086673..5730295e69d3 100644
> > --- a/tools/lib/bpf/usdt.c
> > +++ b/tools/lib/bpf/usdt.c
> > @@ -241,6 +241,7 @@ struct usdt_note {
> >         long loc_addr;
> >         long base_addr;
> >         long sema_addr;
> > +       bool nop_combo;
> >  };
> >
> >  struct usdt_target {
> > @@ -262,6 +263,7 @@ struct usdt_manager {
> >         bool has_bpf_cookie;
> >         bool has_sema_refcnt;
> >         bool has_uprobe_multi;
> > +       bool has_uprobe_syscall;
> >  };
> >
> >  struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
> > @@ -301,6 +303,11 @@ struct usdt_manager *usdt_manager_new(struct bpf_object *obj)
> >          * usdt probes.
> >          */
> >         man->has_uprobe_multi = kernel_supports(obj, FEAT_UPROBE_MULTI_LINK);
> > +
> > +       /*
> > +        * Detect kernel support for uprobe syscall to be used to pick usdt attach point.
> > +        */
> 
> nit: single line comment
> 
> but I find the wording confusing, we don't really use uprobe() syscall
> to pick USDT attach point (which is what comment implies in my mind).
> Just say that we detect uprobe() syscall support. It's presence means
> we can take advantage of faster nop5 uprobe handling. Also, please add
> reference commit hash + message, just like for other feature detectors
> here.

ok

> 
> > +       man->has_uprobe_syscall = kernel_supports(obj, FEAT_UPROBE_SYSCALL);
> >         return man;
> >  }
> >
> > @@ -784,6 +791,15 @@ static int collect_usdt_targets(struct usdt_manager *man, Elf *elf, const char *
> >                 target = &targets[target_cnt];
> >                 memset(target, 0, sizeof(*target));
> >
> > +               /*
> > +                * We have usdt with nop,nop5 instruction and we detected uprobe syscall,
> > +                * so we can place the uprobe directly on nop5 (+1) to get it optimized.
> > +                */
> > +               if (note.nop_combo && man->has_uprobe_syscall) {
> > +                       usdt_abs_ip++;
> > +                       usdt_rel_ip++;
> > +               }
> 
> how hard would it be to check nop5 instruction in ELF file to be extra
> safe? I'm just not sure if I'm 100% comfortable just trusting that
> extra zero byte :)

good idea, and we do have the Elf object as argument, so it should be easy enough

> 
> > +
> >                 target->abs_ip = usdt_abs_ip;
> >                 target->rel_ip = usdt_rel_ip;
> >                 target->sema_off = usdt_sema_off;
> > @@ -1144,7 +1160,7 @@ struct bpf_link *usdt_manager_attach_usdt(struct usdt_manager *man, const struct
> >  static int parse_usdt_note(GElf_Nhdr *nhdr, const char *data, size_t name_off, size_t desc_off,
> >                            struct usdt_note *note)
> >  {
> > -       const char *provider, *name, *args;
> > +       const char *provider, *name, *args, *end, *extra;
> >         long addrs[3];
> >         size_t len;
> >
> > @@ -1182,6 +1198,15 @@ static int parse_usdt_note(GElf_Nhdr *nhdr, const char *data, size_t name_off, s
> >         if (args >= data + len) /* missing arguments spec */
> >                 return -EINVAL;
> >
> > +       extra = memchr(args, '\0', data + len - args);
> > +       if (!extra) /* non-zero-terminated args */
> > +               return -EINVAL;
> > +       ++extra;
> > +       end = data + len;
> 
> end variable just to use it once in the comparison below? Also, how
> about just this:
> 
> extra++;
> if (extra < data + len & *extra == '\0')
>     note->nop_combo = true;
> 
> ?
> 
> (why assuming extra is the very last byte, maybe we'll have more
> "extensions" in the future :) )

well, it is the very last byte for now ;-) but sure, will change

thanks,
jirka