Re: [PATCH 2/2] rawfbt: prvname is not properly set

On Fri, Jan 16, 2026 at 12:18:51AM -0500, Eugene Loh wrote:
> On 1/15/26 17:18, Kris Van Hees wrote:
> 
> > On Tue, Jan 13, 2026 at 04:42:05PM -0500, [email protected] wrote:
> > > From: Eugene Loh <[email protected]>
> > > 
> > > The char array prvname[] is set for each provider.  It is used in the
> > > file that implements the provider.  It might also be passed to
> > > dt_sdt_populate() via a function argument.
> > > 
> > > However, dt_provider_tp.h also defines the macro GROUP_DATA in terms of
> > > prvname.  In turn, GROUP_DATA is used not only in dt_prov_dtrace.c but
> > > then again in dt_prov_fbt.c to define FBT_GROUP_DATA.
> > > 
> > > In commit 0b7c5a632 ("fbt, rawfbt: consolidate code to avoid duplication"),
> > > the fbt and rawfbt providers are combined into a single file.  Thus, two
> > > uses of prvname collide.  The in-file collisions get resolved, but the
> > > cascade of macro definitions does not:  FBT_GROUP_DATA ends up using
> > > "fbt" for both fbt and rawfbt providers.  As a result, it is possible
> > > for rawfbt probes not to be seen.
> > > 
> > > Notice that GROUP_DATA is always paired with prp->desc->prb.  Therefore,
> > > simply replace:
> > >      -#define GROUP_DATA     getpid(), prvname
> > >      +#define PROBE_DATA     getpid(), prp->desc->prv, prp->desc->prb
> > > This makes the code more compact and relieves the macro definitions from
> > > needing prvname.  It also makes the FBT_GROUP_DATA macro unnecessary.
> > > 
> > > The format (FMT) macro is similarly renamed and redefined to include the
> > > probe info.
> > > 
> > > Add a test to check that a rawfbt probe can be found behind an fbt probe
> > > when the probe description has a wildcard provider.
> > > 
> > > Orabug: 38842114
> > > Signed-off-by: Eugene Loh <[email protected]>
> > > ---
> > >   libdtrace/dt_prov_dtrace.c                    | 14 ++++++-------
> > >   libdtrace/dt_prov_fbt.c                       | 13 ++++++------
> > >   libdtrace/dt_prov_rawtp.c                     |  4 ++--
> > >   libdtrace/dt_prov_sdt.c                       |  4 ++--
> > >   libdtrace/dt_provider_tp.h                    | 12 +++++------
> > >   .../providers/rawfbt/tst.wildcard-provider.d  | 20 +++++++++++++++++++
> > >   .../providers/rawfbt/tst.wildcard-provider.r  |  2 ++
> > >   7 files changed, 45 insertions(+), 24 deletions(-)
> > >   create mode 100644 test/unittest/providers/rawfbt/tst.wildcard-provider.d
> > >   create mode 100644 test/unittest/providers/rawfbt/tst.wildcard-provider.r
> > > 
> > > diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> > > index 34b5d8e24..102afd84e 100644
> > > --- a/libdtrace/dt_prov_dtrace.c
> > > +++ b/libdtrace/dt_prov_dtrace.c
> > > @@ -243,8 +243,8 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> > >   		/* add a uprobe */
> > >   		fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
> > >   		if (fd != -1) {
> > > -			rc = dprintf(fd, "p:" GROUP_FMT "/%s %s\n",
> > > -				     GROUP_DATA, prp->desc->prb, spec);
> > > +			rc = dprintf(fd, "p:" PROBE_FMT " %s\n",
> > > +				     PROBE_DATA, spec);
> > >   			close(fd);
> > >   		}
> > >   		free(spec);
> > > @@ -252,14 +252,14 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> > >   			return -ENOENT;
> > >   		/* open format file */
> > > -		len = snprintf(NULL, 0, "%s" GROUP_FMT "/%s/format",
> > > -			       EVENTSFS, GROUP_DATA, prp->desc->prb) + 1;
> > > +		len = snprintf(NULL, 0, "%s" PROBE_FMT "/format",
> > > +			       EVENTSFS, PROBE_DATA) + 1;
> > >   		fn = dt_alloc(dtp, len);
> > >   		if (fn == NULL)
> > >   			return -ENOENT;
> > > -		snprintf(fn, len, "%s" GROUP_FMT "/%s/format",
> > > -			 EVENTSFS, GROUP_DATA, prp->desc->prb);
> > > +		snprintf(fn, len, "%s" PROBE_FMT "/format",
> > > +			 EVENTSFS, PROBE_DATA);
> > How about also changing this to use asprintf() rather than doing the douoble
> > snprintf (one to get the langth, the alloc, and then actually populate).
> 
> Sure, sounds good.  On the other hand, this is in dt_prov_dtrace, while the
> main point of the patch is an fbt problem.  Yeah, I know, not a big deal. 
> But since we've been chasing these asprintf() sites opportunistically over a
> couple of patches and there are several (4?) more sites to go, how about I
> do all these asprintf() conversions in a separate patch?

Sounds good (and thank you for already posting those).

> > >   		f = fopen(fn, "r");
> > >   		dt_free(dtp, fn);
> > >   		if (f == NULL)
> > > @@ -300,7 +300,7 @@ static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> > >   	if (fd == -1)
> > >   		return;
> > > -	dprintf(fd, "-:" GROUP_FMT "/%s\n", GROUP_DATA, prp->desc->prb);
> > > +	dprintf(fd, "-:" PROBE_FMT "\n", PROBE_DATA);
> > >   	close(fd);
> > >   }
> > > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > > index 3feac56ea..8dbf9740a 100644
> > > --- a/libdtrace/dt_prov_fbt.c
> > > +++ b/libdtrace/dt_prov_fbt.c
> > > @@ -53,8 +53,7 @@ static const char		prvname[] = "fbt";
> > >   #define KPROBE_EVENTS		TRACEFS "kprobe_events"
> > > -#define FBT_GROUP_FMT		GROUP_FMT "_%s"
> > > -#define FBT_GROUP_DATA		GROUP_DATA, prp->desc->prb
> > > +#define FBT_PROBE_FMT		"dt_%d_%s_%s"
> > >   static const dtrace_pattr_t	pattr = {
> > >   { DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
> > > @@ -508,16 +507,16 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> > >   		if (fd == -1)
> > >   			goto out;
> > > -		rc = dprintf(fd, "%c:" FBT_GROUP_FMT "/%s %s\n",
> > > +		rc = dprintf(fd, "%c:" FBT_PROBE_FMT "/%s %s\n",
> > >   			     prp->desc->prb[0] == 'e' ? 'p' : 'r',
> > > -			     FBT_GROUP_DATA, tpn, fun);
> > > +			     PROBE_DATA, tpn, fun);
> > >   		close(fd);
> > >   		if (rc == -1)
> > >   			goto out;
> > >   		/* create format file name */
> > > -		if (asprintf(&fn, "%s" FBT_GROUP_FMT "/%s/format", EVENTSFS,
> > > -			     FBT_GROUP_DATA, tpn) == -1)
> > > +		if (asprintf(&fn, "%s" FBT_PROBE_FMT "/%s/format", EVENTSFS,
> > > +			     PROBE_DATA, tpn) == -1)
> > >   			goto out;
> > >   		/* open format file */
> > > @@ -583,7 +582,7 @@ static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> > >   		}
> > >   	}
> > > -	dprintf(fd, "-:" FBT_GROUP_FMT "/%s\n", FBT_GROUP_DATA, tpn);
> > > +	dprintf(fd, "-:" FBT_PROBE_FMT "/%s\n", PROBE_DATA, tpn);
> > >   	close(fd);
> > >   	if (tpn != prp->desc->fun)
> > > diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
> > > index 709f7040f..e3269f4b3 100644
> > > --- a/libdtrace/dt_prov_rawtp.c
> > > +++ b/libdtrace/dt_prov_rawtp.c
> > > @@ -56,7 +56,7 @@ static const dtrace_pattr_t	pattr = {
> > >   /*
> > >    * The PROBE_LIST file lists all tracepoints in a <group>:<name> format.
> > >    * We need to ignore these groups:
> > > - *   - GROUP_FMT (created by DTrace processes)
> > > + *   - PROBE_SFMT
> > >    *   - kprobes and uprobes
> > >    *   - syscalls (handled by a different provider)
> > >    *   - pid and usdt probes (ditto)
> > > @@ -89,7 +89,7 @@ static int populate(dtrace_hdl_t *dtp)
> > >   			*p++ = '\0';
> > > -			if (sscanf(buf, GROUP_SFMT, &dummy, &str) == 2) {
> > > +			if (sscanf(buf, PROBE_SFMT, &dummy, &str) == 2) {
> > >   				free(str);
> > >   				continue;
> > >   			}
> > > diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
> > > index 74555f5be..ee2cc3c27 100644
> > > --- a/libdtrace/dt_prov_sdt.c
> > > +++ b/libdtrace/dt_prov_sdt.c
> > > @@ -54,7 +54,7 @@ static const dtrace_pattr_t	pattr = {
> > >   /*
> > >    * The PROBE_LIST file lists all tracepoints in a <group>:<name> format.
> > >    * We need to ignore these groups:
> > > - *   - GROUP_FMT (created by DTrace processes)
> > > + *   - PROBE_SFMT
> > >    *   - kprobes and uprobes
> > >    *   - syscalls (handled by a different provider)
> > >    *   - pid and usdt probes (ditto)
> > > @@ -87,7 +87,7 @@ static int populate(dtrace_hdl_t *dtp)
> > >   			*p++ = '\0';
> > > -			if (sscanf(buf, GROUP_SFMT, &dummy, &str) == 2) {
> > > +			if (sscanf(buf, PROBE_SFMT, &dummy, &str) == 2) {
> > >   				free(str);
> > >   				continue;
> > >   			}
> > > diff --git a/libdtrace/dt_provider_tp.h b/libdtrace/dt_provider_tp.h
> > > index f131b1e02..406609d1b 100644
> > > --- a/libdtrace/dt_provider_tp.h
> > > +++ b/libdtrace/dt_provider_tp.h
> > > @@ -18,13 +18,13 @@ extern "C" {
> > >    * Tracepoint group naming format for DTrace providers.  Providers may append
> > >    * to this format string as needed.
> > >    *
> > > - * GROUP_DATA provides the necessary data items to populate the format string
> > > - * (PID of the dtrace process and the provider name).  GROUP_SFMT is like
> > > - * GROUP_FMT, but for sscanf().
> > > + * PROBE_DATA provides the necessary data items to populate the format string.
> > > + * PROBE_FMT formats that data.
> > > + * PROBE_SFMT is a format string for recognizing PROBE_FMT data in sscanf().
> > >    */
> > > -#define GROUP_FMT	"dt_%d_%s"
> > > -#define GROUP_SFMT	"dt_%d_%ms"
> > > -#define GROUP_DATA	getpid(), prvname
> > > +#define PROBE_DATA	getpid(), prp->desc->prv, prp->desc->prb
> > > +#define PROBE_FMT	"dt_%d_%s/%s"
> > > +#define PROBE_SFMT	"dt_%d_%ms"
> > >   typedef struct tp_probe tp_probe_t;
> > > diff --git a/test/unittest/providers/rawfbt/tst.wildcard-provider.d b/test/unittest/providers/rawfbt/tst.wildcard-provider.d
> > > new file mode 100644
> > > index 000000000..ad9f4c779
> > > --- /dev/null
> > > +++ b/test/unittest/providers/rawfbt/tst.wildcard-provider.d
> > > @@ -0,0 +1,20 @@
> > > +/*
> > > + * Oracle Linux DTrace.
> > > + * Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved.
> > > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > > + * http://oss.oracle.com/licenses/upl.
> > > + */
> > > +
> > > +/*
> > > + * ASSERTION: rawfbt probes can be found even when a wildcard provider
> > > + * description also allows fbt probes.
> > > + */
> > > +
> > > +#pragma D option quiet
> > > +
> > > +BEGIN,
> > > +*fbt:vmlinux:do_sys_open*:entry
> > > +{
> > > +	printf("success\n");
> > > +	exit(0);
> > > +}
> > I am confused how this test accomplishes verification of the assertion.  If
> > a regular FBT probe satisfies the probe description, wouldn't this pass also?
> > Wouldn't a -l based test be better here, where you can test that the output
> > does indeed report both the fbt probe and the rawfbt probe that match?
> 
> It appears that -l is not an issue;  a -l test does not show the problem. 
> The test in the patch is derived from the user bug report that motivated
> this patch.
> 
> The whole prvname problem is that the bad prvname value makes GROUP_DATA
> bad, which makes FBT_GROUP_DATA bad, which is an issue only in kprobe_attach
> (and detach).  So, we need kprobe_attach() to see the problem.

Ok, so yes, -l is not sufficient.  But still, even if the test is derived from
the original reported problem, it still looks wrong to me.  Or rather, the test
can pass when

	fbt:vmlinux:do_sys+open*:entry exists
	rawfbt:vmlinux:do_sys+open*:entry does NOT exist

because as long as 1 probe matches the probe specification, compilation will
succeed (and at runtime, the probe firing will complete the success criterium).

But that is not a valid PASS result for the assertion that is posed for this
test.  If you are specifically looking for a rawfbt probe using a wildcard, you
might want to use r*fbt:vmlinux:do_sys+open*:entry instead perhaps?

> > > diff --git a/test/unittest/providers/rawfbt/tst.wildcard-provider.r b/test/unittest/providers/rawfbt/tst.wildcard-provider.r
> > > new file mode 100644
> > > index 000000000..b34a52d2f
> > > --- /dev/null
> > > +++ b/test/unittest/providers/rawfbt/tst.wildcard-provider.r
> > > @@ -0,0 +1,2 @@
> > > +success
> > > +
> > > -- 
> > > 2.47.3
> > >