Page MenuHomeFreeBSD

Factor out common kerneldump code into dump_start()/dump_finish().
ClosedPublic

Authored by markj on Jul 13 2017, 8:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 11:58 PM
Unknown Object (File)
Nov 7 2024, 12:07 PM
Unknown Object (File)
Oct 23 2024, 1:12 PM
Unknown Object (File)
Oct 18 2024, 12:35 AM
Unknown Object (File)
Oct 1 2024, 9:44 PM
Unknown Object (File)
Sep 30 2024, 12:45 AM
Unknown Object (File)
Sep 29 2024, 9:10 PM
Unknown Object (File)
Sep 21 2024, 4:15 AM
Subscribers

Details

Summary

This is a step towards generic support for kernel dump compression.
dump_start() and dump_finish() are responsible for writing kernel dump
headers, optionally writing the crypto key, and initializing "dumplo",
i.e. the offset into the dump device at which we start writing the
dump's contents.

Also remove the unused dump_write_pad(), and make some functions
static now that they're no longer used outside of kern_shutdown.c.

No functional change intended.

Test Plan

minidumps, full dumps, encrypted minidumps, all on amd64

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10822
Build 11214: arc lint + arc unit

Event Timeline

markj added reviewers: cem, def, asomers, rgrimes.
This revision is now accepted and ready to land.Jul 14 2017, 9:36 PM
sys/kern/kern_shutdown.c
1238

dump_start() returns ENOSPC now while in the past we set error to E2BIG in case of a lack of space on amd64 and arm64. On these architectures in the ENOSPC case we try to create a minidump again.

We should remove E2BIG cases as it's dead code and check in case of ENOSPC if the first kernel dump header was successfully written. I think it doesn't make sense to try to write a crash dump again if we fail at the beginning.

sys/kern/kern_shutdown.c
1238

Oops, thanks for catching that.

Note that dump_check_bounds() returns ENOSPC. I think it would be simpler to change this to return E2BIG instead?

sys/kern/kern_shutdown.c
1218

Can we move it to sys/sys/kerneldump.h and rename to KERNELDUMP_METADATA_SIZE so we can use it also in sys/ddb/ddb/db_textdump.c?

1238

Do you mean to change dump_start() to return E2BIG if this check fails? This would be a good idea. Note that this also requires changes in MD code.

1240

Currently dump_start() modifies dumplop even if it fails. I would introduce a temporary dumplo variable to update *dumplop if dump_start() is successful. It would be less bug prone if we'll want to reuse *dumplop in case of a failure in the future. It would be also consistent with dumplo being updated after each dump_write() call in MD code.

1260

For consistency with dump_start() I would pass a pointer to dumplo to dump_finish() and update it after a kernel dump header is successfully written.

markj added inline comments.
sys/kern/kern_shutdown.c
1238

Indeed.

1260

I'm not sure why it's important to be consistent in this way. A dumplop argument would point into a stack frame that's about to be freed anyway. Further, I'll note that the dumplo/dumplo arguments are removed in a later revision.

markj edited edge metadata.
markj marked an inline comment as done.
  • Address review feedback.
This revision now requires review to proceed.Aug 2 2017, 10:39 PM
sys/arm/arm/minidump_machdep.c
345

I was thinking about

else if (error == E2BIG || error == ENOSPC)

here and in other architectures as dump_write() can still return ENOSPC which might happen when a number of pages being dumped has increased during dumping.

sys/kern/kern_shutdown.c
1260

OK.

  • Combine error handling for E2BIG and ENOSPC.
def added inline comments.
sys/amd64/amd64/minidump_machdep.c
444

ENOSPC case is handled before. :)

sys/arm64/arm64/minidump_machdep.c
420

ENOSPC case is handled before.

This revision is now accepted and ready to land.Aug 6 2017, 3:50 PM
This revision was automatically updated to reflect the committed changes.