linux(4): implement coredump support

[trasz]

Summary

Implement dumping core for Linux binaries on amd64, for both 32- and 64-bit executables.
Some bits are still missing.

This is based on a prototype by chuck@.

[dchagin]

sys/compat/linux/linux_elf.c

353

is __ELF_WORD_SIZE == 32 really should be here?

[trasz]

sys/compat/linux/linux_elf.c

353

I think so. Thing is, this code is built twice, but indirectly; see linux_elf64.c and linux_elf32.c.

[trasz]

Rebase.

[trasz]

There's still one change to imgact_elf.c, but this one is completely Linux-specific, so I'm not sure if there's a point in splitting it off.

[kib]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

I think this should be yet another sv_XXX method.

[dchagin]

sys/compat/linux/linux_elf.c

353

ok, I’m confused a bit)
For i386 we does not define compat_linux32

[trasz]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

Why? We're already calling this through this indirection (as sv_coredump); I don't see the point of adding more. Also, another ABI - if it ever happens - might need different tweaks here.

[kib]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

For me, your reply sounds as an additional strong argument pro change this to sv_XXX method. Exactly because this part is per-ABI.

The fact that it is called through sv_coredump does not matter, since you ended up in the shared code.

[trasz]

Fix tinderbox.

[trasz]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

This isn't per-ABI - it's ABI-specific. In other words, one can expect each ABI to require this kind of quirk, but not this particular one, so we would be adding more and more sv_whatever variables to control each of them.

[dchagin]

sys/modules/linux64/Makefile

21

what is the purpose to ignore i386?

[trasz]

sys/modules/linux64/Makefile

21

It doesn't build on i386, and I'm not particularly motivated to spending time on fixing a dead architecture, I have a live one that needs fixing ;-)

[trasz]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

I think my main problem here is that adding another sv_variable makes things harder to understand (more indirection) and more boilerplate'y for no good reason - it won't make the code cleaner, shorter, or more flexible.

[kib]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

Of course it makes the code cleaner, and we might argue about flexibility. But not having the FreeBSD ABI coredump code to know/care about other ABIs is the good design by definition.

Also, I do not get the argument that some ABIs would need to do more specializations there. If the differences are large, ABI would provide more then one sysent classes, if not, it would handle it internally. But even more then, I do not want to contaminate the base FreeBSD code with the irrelevant details to handle foreign ABIs.

[trasz]

Get rid of SVC_LINUX, prefill this field first instead.

[trasz]

Restore build fixes.

[trasz]

sys/kern/imgact_elf.c

1825 ↗

(On Diff #89897)

Okay, what do you think about this instead? It avoids having any Linux-specific code in the common path, is simple, flexible, and avoids introducing another sysent entry.

[kib]

sys/compat/linux/linux_elf.c

120

Now that there is no common code changed in your patch, I looked at the linux/ diff.

I would not call this code similar. This function is a literal copy of the FreeBSD native coredump, modulo the removal of compressed dump support.

221

And this function is again the exact copy of the FreeBSD' corehdr except the added assignment to e_ident.

I do not think this is a good approach. Returning to the previous suggestion of adding some sv_ methods, I think this is the way to go instead of copying 90% of coredumping code and making small modifications here and there.

If you do not like adding sv_ methods for ELF coredump, consider creating a table of virtual methods just for ELF coredump and passing it as an arg to coredump(). But this copying of ~300 LoC should be fixed IMO.

[trasz]

Rework the sysent to reduce code duplication.

[trasz]

sys/compat/linux/linux_elf.c

221

I thought there would be too many differences to do this properly, but turns out you were right - it's just two additional sv_ variables and one method, with pretty much no duplication left.

[kib]

Overall, I like the result.

Again, could you, please split this change into core part and then Linux bits? Perhaps for review it is better to keep them together, but I suggest to make (at least) two commits there.

sys/kern/imgact_elf.c

1885 ↗

(On Diff #90740)

Can you change the interface to add either struct sysentvec *sv or struct thread *td as the argument, to avoid referencing curproc there?

I know that coredump code have more dependencies on the context so it cannot dump remote process, but I do not want to introduce new deps.

1955 ↗

(On Diff #90740)

Same (please add td or sv arg)/

sys/sys/imgact_elf.h

105 ↗

(On Diff #90740)

I am not sure that this static definition has the effect you intended, formally it add the instance of FREEBSD_ABI_VENDOR to all files including this header.

sys/sys/sysent.h

117 ↗

(On Diff #90740)

I suggest to name the fields in style of sv_elf_core_XXX

[trasz]

Implement Konstantin's suggestions.

[trasz]

Will definitely split it into non-Linux specific part, and the Linux-specific one, but I think it's easier to review as a whole.

[kib]

sys/compat/linux/linux_elf.c

106

Extra ()

126

What are the differences between this code and FreeBSD native prpsinfo?

135

The cast is not needed.

197

Cast is not needed.
Perhaps you can fix these casts in FreeBSD code as the preliminary commit.

221

But this function is still the exact copy of the FreeBSD function?

246

Same code, again?

sys/sys/sysent.h

119 ↗

(On Diff #90801)

Is this line too long?

[trasz]

sys/compat/linux/linux_elf.c

126

I think none at the moment, but there's a large chance they will diverge. This code is in a "good enough" state, but it'll require spending some more time comparing its output with what Linux kernel generates. Thus, I'd rather not deduplicate them just yet.

[trasz]

Some more fixes.

[kib]

sys/compat/linux/linux_elf.c

106

You might want to fix this in imgact_elf.c as a preliminary commit.

126

I suspect that prpsinfo goes back to SysV, so the format of the note is engraved in stone.

276

Why 320?

And similar question for the native note_procstat_auxv, where it might become too short quite fast (128).

I mean, it should be based on AT_COUNT and equivalent Linux constant.

[trasz]

sys/compat/linux/linux_elf.c

106

Spun off as https://reviews.freebsd.org/D30841.

126

I'm not sure anything can be engraved in stone when we're talking about Linux :-) In any case: if it turns out these are same between FreeBSD and Linux, I'll deduplicate them later on.

[trasz]

Fix some more issues pointed out by Kib, and try to pacify tinderbox.

[kib]

sys/amd64/amd64/elf_machdep.c

64 ↗

(On Diff #91212)

BTW I think all entries can use elf64 instead of elfN(). This is for later, there are existing elfN() uses.

sys/compat/linux/linux_elf.c

126

In fact, I would be more happy about leaving this functions redefined if you used the names like l_prstatus_t instead of using exact same name as the FreeBSD native code.

[trasz]

Add prefixes to individual functions; should make backtraces
less confusing. While here also add a greppable comment.

[kib]

sys/compat/linux/linux_elf.c

138

If it is still elf_prpsinfo_t, you do not need a copy of the function. Both prpsinfo and prfpregset should be linux-named IMO.

[trasz]

sys/compat/linux/linux_elf.c

138

It is elf_prpsinfo_t _for now_; it will likely change later. I'd prefer to avoid a scenario where we add complexity, exporting the native function and using it here, only to find that we actually do need to do something differently here. Deduplicating stuff afterwards is easy.

[trasz]

This diff is somewhat huge, so I've spun off bits not specific to Linux into https://reviews.freebsd.org/D30921.

[trasz]

Regen after 435754a59ee.

[kib]

sys/compat/linux/linux_elf.c

123

Why did you left the generic name for this (and one below) types?

These should be e.g. linux_prpsinfo_t. Do not use elf_ prefix in linux code which you refuse to de-dup.

203

This cast is not needed.

[trasz]

Fix type names, remove unnecessary casts.