dtrace: conditionally load the systrace_linux klds when loading dtrace.
ClosedPublic
Actions

Authored by gallatin on Jan 9 2023, 3:04 AM.

Details

Reviewers

markj
jhb
emaste
gnn

Commits

rGda81cc6035f8: dtrace: conditionally load the systrace_linux klds when loading dtrace.

Summary

When dtrace starts, it tries to detect if the dtrace klds are loaded, and if not, it loads them by loading the dtraceall kld. This module depends on most dtrace modules, including systrace for the native freebsd and freebsd32 ABIs. However, it does not depend on the systrace_linux klds, as they in turn depend on the linux ABI klds, and we don't want to load an ABI module that the user has not explicitly requested. This can leave a naive user in a state where they think all syscall providers have been loaded, yet linux ABI syscalls are "invisible" to dtrace.

To fix this, check to see if the linux ABI modules are loaded. If they are, then load their systrace klds.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

gallatin created this revision.Jan 9 2023, 3:04 AM

Herald added a reviewer: gnn. · View Herald TranscriptJan 9 2023, 3:04 AM

Herald added a subscriber: imp. · View Herald Transcript

gallatin requested review of this revision.Jan 9 2023, 3:04 AM

emaste added inline comments.Jan 9 2023, 6:55 PM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	Is this correct for 32-bit cases? (i.e., do we use linuxelf and systrace_linux there?)

gallatin added inline comments.Jan 9 2023, 11:40 PM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	I'm not sure, and I have no way to test this, as I have not run a 32-bit FreeBSD install in over a decade. This should not make 32-bit platforms any worse off, as errors loading the modules are ignored. At worst, 32-bit platforms will maintain the status quo where systrace_linux is not loaded.

emaste added inline comments.Jan 9 2023, 11:43 PM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	Fair point, it's not worse than what's there now. I was thinking perhaps of something like `if (modfind("linux64elf") >= 0 \|\| modfind("linuxelf") >= 0))` try loading both modules.

gallatin added inline comments.Jan 10 2023, 12:51 AM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	In a case where somebody intended to load just 64b linux ABI, your suggestion would result in dtrace loading the 32-bit linux systrace which would then load the 32-bit linux ABI as a dependency. It looks like linuxelf is the name of the 32-bit linux ABI on at least x86 (checked on ref14-i386) I think the right thing the is: if (modfind("linuxelf") >= 0) { if (sizeof(void *) == 8) kldload("systrace_linux32"); else kldload("systrace_linux"); } I'll test that (on amd64) and build-test with make universe..

Updated patch to load the proper systrace module when running on a 32-bit system.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	That works on 64b, and builds everywhere. And I think it should work on 32b.

emaste accepted this revision.Jan 10 2023, 2:02 PM

emaste added inline comments.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1121	There's also a `__LP64__` macro you could test (but the unused case here should be optimized out anyway, of course).

This revision is now accepted and ready to land.Jan 10 2023, 2:02 PM

markj added inline comments.Jan 13 2023, 3:45 PM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	I think it'd be a bit better to check `sizeof(void ) == 4)` for the benefit of CHERI, where `sizeof(void ) == 16`.

markj added inline comments.Jan 13 2023, 3:46 PM

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
1118–1119	Or perhaps test sizeof(long) == 8 like you do in D37707 (albeit at compile time).

I agree with @markj on using #ifdef's instead of sizeof(), so something like:

#if __SIZEOF_LONG__ == 8
    if (modfind("linux64elf") >= 0)
        kldload("systrace_linux");
    if (modfind("linuxelf") >= 0)
        kldload("systrace_linux32");
#else
    if (modfind("linuxelf") >= 0)
        kldload("sys trace_linux");
#endif

BTW, I really wish we would make "linuxelf" be the "native" Linux ABI and had linux32.ko instead of linux.ko on amd64 being i386. (*sigh*) That ship has probably sailed, but it would make this simpler had we done it correctly from the start.

I really wish we would make "linuxelf" be the "native" Linux ABI and had linux32.ko instead of linux.ko on amd64 being i386. (*sigh*) That ship has probably sailed

I don't think it's too late as long as someone wants to hand-hold the transition.

In D37986#866186, @emaste wrote:

I really wish we would make "linuxelf" be the "native" Linux ABI and had linux32.ko instead of linux.ko on amd64 being i386. (*sigh*) That ship has probably sailed

I don't think it's too late as long as someone wants to hand-hold the transition.

You have to also fix at least truss if you rename things I think. Plus renaming linux64.ko to linux.ko is probably POLA by now. It may be doable, but it's fairly tedious probably, and certainly orthogonal to this change.

Update diff to use ifdefs and SIZEOF_LONG as suggested by reviewers.

This revision now requires review to proceed.Jan 21 2023, 7:34 PM

markj accepted this revision.Jan 23 2023, 2:01 PM

This revision is now accepted and ready to land.Jan 23 2023, 2:01 PM

Closed by commit rGda81cc6035f8: dtrace: conditionally load the systrace_linux klds when loading dtrace. (authored by gallatin). · Explain WhyJan 24 2023, 1:38 AM

This revision was automatically updated to reflect the committed changes.

gallatin added a commit: rGda81cc6035f8: dtrace: conditionally load the systrace_linux klds when loading dtrace..

trond.endrestol_ximalas.info added a subscriber: trond.endrestol_ximalas.info.Jan 24 2023, 5:28 AM

trond.endrestol_ximalas.info added inline comments.