[DTrace-devel] [PATCH] Allow arbitrary tracefs mount points

[Nick Alcock]

On 7 Nov 2024, Kris Van Hees uttered the following:

> On Wed, Nov 06, 2024 at 04:46:56PM +0000, Nick Alcock via DTrace-devel wrote:
>> Tested with both in-practice-observed locations of debugfs.
>
> Since you mention that tracefs can be mounted anywhere, there definitely should
> be a test that exercises this functionality when tracefs is *not* mounted in
> any of the typical locations.

That's hard to do without disrupting later tests, really. You can't
mount --move tracefs and if you unmount it remounting it in the same way
it was mounted is... interesting.

I'm locally testing with unusual mount points, and it's easy to tweak a
VM so it always has a mount in the "other usual place".

> And I bet you meant to write 'tracefs' here and not 'debugfs'?

Ugh yes!

>> -	{ EDT_PRINT, "Missing or corrupt print() record" }
>> +	{ EDT_PRINT, "Missing or corrupt print() record" },
>> +	{ EDT_TRACEFS_MNT, "Cannot find tracefs mount point" }
>
> I think you could make the identifier a little shorter (EDT_TARCEFS would do),
> and maybe the message can be "Cannot find tracefs"?  Especially since the code
> that throws this error goes through all the mountpoints, so the issue is not
> that the mount point cannot be found but rather that tracefs is not mounted.

It's that we can't find the mount point. It might well still be mounted
in ways we can't find (e.g. in some other namespace, in a disconnected
subtree etc). Unlikely, I suppose...

The MNT was to indicate that this is specifically about the user needing
to *mount* tracefs, rather than some other thing, but I guess I can
shorten it. I can't think of what that other thing might be anyway.

>>  };
>>  
>>  static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
>> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
>> index 68fb8ec53c06..9d246cef2e7f 100644
>> --- a/libdtrace/dt_impl.h
>> +++ b/libdtrace/dt_impl.h
>> @@ -354,6 +354,8 @@ struct dtrace_hdl {
>>  	char *dt_module_path;	/* pathname of kernel module root */
>>  	dt_version_t dt_kernver;/* kernel version, used in the libpath */
>>  	char *dt_dofstash_path;	/* Path to the DOF stash.  */
>> +	char *dt_tracefs_path;	/* Path to tracefs.  */
>> +	char *dt_tracefs_file;	/* See tracefs_file().  */
>
> I don't think you need this _file member (see below).

Yeah, if you want to have to free() it over and over again. I avoided
doing that because I thought it was unbearably noisy.

But the functions you suggest eliminate any need to do that!

>>  	uid_t dt_useruid;	/* lowest non-system uid: set via -xuseruid */
>>  	char *dt_sysslice;	/* the systemd system slice: set via -xsysslice */
>>  	uint_t dt_lazyload;	/* boolean:  set via -xlazyload */
>> @@ -643,6 +645,7 @@ enum {
>>  	EDT_TRACEMEM,		/* missing or corrupt tracemem() record */
>>  	EDT_PCAP,		/* missing or corrupt pcap() record */
>>  	EDT_PRINT,		/* missing or corrupt print() record */
>> +	EDT_TRACEFS_MNT,	/* cannot find tracefs mount point */
>
> EDT_TRACEFS, cannot find tracefs

ACK.

>>  };
>>  
>>  /*
>> @@ -713,6 +716,7 @@ extern void dt_conf_init(dtrace_hdl_t *);
>>  
>>  extern int dt_gmatch(const char *, const char *);
>>  extern char *dt_basename(char *);
>> +extern const char *tracefs_file(dtrace_hdl_t *, const char *);
>
> As mentioned below, I think it would make sense to change this to be:
>
> 	extern char *dt_tracefs_fn(dtrace_hdl_t *, const char *, ...);
>
> so it can operate as a *printf() style function.

Oh that's very nice! Why didn't I think of that etc.

This is why I love reviews :)

> As mentioned below, I think adding a function here would improve things:
>
> 	extern int dt_tracefs_open(dtrace_hdl_t *, const char *, mode_t);
>
> This will open the given file within tracefs and return the fd. 

Naah... go the whole hog:

extern int dt_tracefs_open(dtrace_hdl_t *, const char *, int flags, ...);

i.e. make it a printf-style function too.  With that, we don't even need
dt_tracefs_fn(), since literally nothing calls it.

(you do need the flags, and the mode is best defaulted I think, given
that no existing callers actually need it and it would uglify every call
and there is a reasonable default (0666). I agree that we must
unconditionally *pass* it in dt_tracefs_open() though, since we don't
know there if the callers passed flags that need it or not.)

>>  		snprintf(fn, len, "%s" GROUP_FMT "/%s/format",
>> -			 EVENTSFS, GROUP_DATA, prp->desc->prb);
>> +			 uprobe_events, GROUP_DATA, prp->desc->prb);
>
> ... to here with:
>
> 		fn = dt_tracefs_fn(dtp, "events/" GROUP_FMT "/%s/format",
> 				   GROUP_DATA, prp->desc->prb);
> 		if (fn == NULL)
> 			return -ENOENT;
>
> And apply the same changes to the other providers.

Ack! except this one can actually use dt_tracefd_open since it's used
to open a file... the same is true of all of them :) goodbye,
dt_tracefs_fn! Oh this is such a nice change, thank you!

>> --- a/libdtrace/dt_prov_fbt.c
>> +++ b/libdtrace/dt_prov_fbt.c
>> @@ -7,7 +7,7 @@
>>   * The Function Boundary Tracing (FBT) provider for DTrace.
>>   *
>>   * FBT probes are exposed by the kernel as kprobes.  They are listed in the
>> - * TRACEFS/available_filter_functions file.  Some kprobes are associated with
>> + * tracefs available_filter_functions file.  Some kprobes are associated with
>
> I would keep these comments as is because TRACEFS representing a static define
> or being a symbol to represent that this component is not pre-defined is quite
> equivalent.  CHanging it to "tracefs *" doesn't really make a difference.

I thought it was a reference to the #define in particular, which is gone.

Dropped.

>>  	FILE		*f;
>> -	char		fn[256];
>> +	const char	*syscallsfs;
>> +	char		fn[PATH_MAX];
>>  	int		rc;
>>  
>>  	/*
>>  	 * We know that the probe name is either "entry" or "return", so we can
>>  	 * just check the first character.
>>  	 */
>> -	strcpy(fn, SYSCALLSFS);
>> +	if ((syscallsfs = tracefs_file(dtp, SYSCALLSFS)) == NULL)
>> +		return -ENOENT;
>> +
>> +	strcpy(fn, syscallsfs);
>>  	if (prp->desc->prb[0] == 'e')
>>  		strcat(fn, "sys_enter_");
>
> This and the following code in that function could benefit from simply using
> dt_tracefs_fn() and doing a free on the returned string when done.

Absolutely! Actually we don't even need that, it's just doing an
open with it, we can just use dt_tracefs_open().

The result is wildly simpler and easier to read, despite using a ternary
conditional:

	fd = dt_tracefs_open(dtp, SYSCALLSFS "/sys_%s_%s/format", O_RDONLY,
			     (prp->desc->prb[0] == 'e') ? "enter" : "exit",
			     prp->desc->fun);

(caveat: only compile-tested so far, I'm sending this while make check
runs).

(I'm wondering if the general pattern of "open using this format string,
call dt_tp_probe_info() on it" should be encapsulated itself. A future
improvement perhaps.)

>> +{
>> +	free(dtp->dt_tracefs_file);
>> +
>> +	if (!dtp->dt_tracefs_path)
>> +		if (find_tracefs_path(dtp) < 0)
>> +			return NULL;		/* errno is set for us. */
>> +
>> +	if (asprintf(&dtp->dt_tracefs_file, "%s/%s", dtp->dt_tracefs_path, fn) < 0) {
>> +		dtp->dt_tracefs_file = NULL;
>> +		dt_set_errno(dtp, EDT_NOMEM);
>> +		return NULL;
>> +	}
>> +	return dtp->dt_tracefs_file;
>> +}
>
> Make this a function that takes a format string and optional arguments, and
> then uses vasprintf() to create the resulting string.  Callers can free the
> string (hardly an issue).
> 
> Add a function dt_tracefs_open(dtrace_hdl_t *dtp, const char *fn, mode_t mode)
> to open the given file under tracefs, returning the fd.  Returns -1 (after
> settinng errno) if the open fails for some reason.

I ruled this out because I didn't think it would be useful. This is
clearly ridiculous, it gets used multiple times and as you show it is
ever so much better than what I have now. Adjusted (with tiny change
noted above, they're both varargs now).

Another tiny improvement: we only accept tracefs paths that do not
contain percent characters, since they never should and if they do we
can't just slam them on the front of a format string without
modification like this.

>> diff --git a/test/unittest/funcs/tst.rw_.x b/test/unittest/funcs/tst.rw_.x
>> index 29c581116154..7e178778d900 100755
>> --- a/test/unittest/funcs/tst.rw_.x
>> +++ b/test/unittest/funcs/tst.rw_.x
>> @@ -1,6 +1,11 @@
>>  #!/bin/sh
>>  
>> -FUNCS=/sys/kernel/debug/tracing/available_filter_functions
>> +TRACEFS=/sys/kernel/tracing
>> +if [[ ! -e $TRACEFS/available_filter_functions ]]; then
>> +	TRACEFS=/sys/kernel/debug/tracing
>> +fi
>> +
>> +FUNCS=${TRACEFS}/available_filter_functions
>
> How about having runtest.sh determine the location (not just checking two
> possible places - look at the mount table), and export it as TRACEFS?  That
> way tests (incl. future tests) can depend on that.

Done! (exported as $tracefs because stuff runtest.sh exports is always
lowercase currently.)

It turns out to be literally a one-liner, i.e. shorter than the code it
replaces that does a worse job in the individual tests!

-- 
NULL && (void)