Re: DTrace, kernel loader, unknown probes, enable on load?

On Fri, Feb 11, 2022 at 03:52:37PM +0000, Bjoern A. Zeeb wrote:
> On Fri, 11 Feb 2022, Mark Johnston wrote:
> 
> > On Fri, Feb 11, 2022 at 09:23:47AM -0500, Mark Johnston wrote:
> >> On Fri, Feb 11, 2022 at 02:10:30PM +0000, Bjoern A. Zeeb wrote:
> >>> Hi,
> >>>
> >>> one of the drawbacks of Dtrace (and other tracing frameworks out there
> >>> on various OSes) is that they do need a list of probes upfront before
> >>> they can enable.
> >>
> >> The probes don't have to exist, add -Z to the dtrace(1) parameters.
> >>
> >>> Say I want to trace a kernel module from the moment it is loaded, that
> >>> is currently not possible.
> >>
> >> So something like:
> >>
> >> # dtrace -n 'fbt::coretemp_identify:entry {stack();}' -Z
> >> # kldload coretemp
> >>
> >> ought to work, I think, but it doesn't.  It's been a while since I
> >> looked at this code but I think it might be related to the unimplemented
> >> (on FreeBSD) portion of dtrace_probe_provide().  IIRC that's due to a
> >> lock order reversal...
> >
> > I see now: the problem is that FBT registers probes after KLD SYSINITs
> > and module hooks are invoked.  See dtrace_module_loaded(), which
> > (asynchronously) calls dtrace_enabling_matchall() to see if any newly
> > registered probes match pending "retained enablings", in this case,
> > enablings created by dtrace -Z.  We could perhaps add a new eventhandler
> > that gets called before anything in the KLD is executed, and register
> > probes at that point.  Or, if the code you're interested runs after
> > SYSINITs are finished, then -Z might be sufficient today.
> 
> -Z didn't do it last I tried.  I wasn't expecting it to actually
> enable the probes at a later time based on the man page.
> 
> Having something which will do the job before SYSINTs are run, that
> would be awesome.

It appears to be sufficient to simply move the kld_load hook to before
module registration, patch below.  In the case of a subsequent error,
the unload hook is called so DTrace gets a chance to clean up.  I can't
see any reasons not to move it, though there's at least one non-dtrace
consumer that needs a look.

diff --git a/sys/kern/kern_linker.c b/sys/kern/kern_linker.c
index 2e4c95f16c8f..55661b9f9aa2 100644
--- a/sys/kern/kern_linker.c
+++ b/sys/kern/kern_linker.c
@@ -452,6 +452,7 @@ linker_load_file(const char *filename, linker_file_t *result)
 		if (error != ENOENT)
 			foundfile = 1;
 		if (lf) {
+			EVENTHANDLER_INVOKE(kld_load, lf);
 			error = linker_file_register_modules(lf);
 			if (error == EEXIST) {
 				linker_file_unload(lf, LINKER_UNLOAD_FORCE);
@@ -472,7 +473,6 @@ linker_load_file(const char *filename, linker_file_t *result)
 				return (ENOEXEC);
 			}
 			linker_file_enable_sysctls(lf);
-			EVENTHANDLER_INVOKE(kld_load, lf);
 			*result = lf;
 			return (0);
 		}