Page MenuHomeFreeBSD

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

Authored by markj on Jul 13 2017, 8:51 PM.

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Jul 13 2017, 8:51 PM
markj edited the test plan for this revision. (Show Details)Jul 13 2017, 8:54 PM
markj added reviewers: cem, def, asomers, rgrimes.
cem accepted this revision.Jul 14 2017, 9:36 PM
This revision is now accepted and ready to land.Jul 14 2017, 9:36 PM
def added inline comments.Jul 31 2017, 8:14 PM
sys/kern/kern_shutdown.c
1238 ↗(On Diff #30745)

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.

markj added inline comments.Jul 31 2017, 9:37 PM
sys/kern/kern_shutdown.c
1238 ↗(On Diff #30745)

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?

def added inline comments.Aug 2 2017, 3:06 PM
sys/kern/kern_shutdown.c
1218 ↗(On Diff #30745)

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 ↗(On Diff #30745)

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 ↗(On Diff #30745)

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 ↗(On Diff #30745)

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 marked 3 inline comments as done.Aug 2 2017, 10:33 PM
markj added inline comments.
sys/kern/kern_shutdown.c
1238 ↗(On Diff #30745)

Indeed.

1260 ↗(On Diff #30745)

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 updated this revision to Diff 31501.Aug 2 2017, 10:39 PM
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
def added inline comments.Aug 3 2017, 9:22 AM
sys/arm/arm/minidump_machdep.c
345 ↗(On Diff #31501)

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 ↗(On Diff #30745)

OK.

markj updated this revision to Diff 31602.Aug 4 2017, 11:32 PM
  • Combine error handling for E2BIG and ENOSPC.
markj marked an inline comment as done.Aug 4 2017, 11:32 PM
def accepted this revision.Aug 6 2017, 3:50 PM
def added inline comments.
sys/amd64/amd64/minidump_machdep.c
444 ↗(On Diff #31602)

ENOSPC case is handled before. :)

sys/arm64/arm64/minidump_machdep.c
420 ↗(On Diff #31602)

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.