Page MenuHomeFreeBSD

Shift some of the EKCD funcs around to keep them grouped together.
ClosedPublic

Authored by markj on Jul 15 2017, 12:43 AM.

Details

Summary

Now that all of the kerneldumpcrypto functions are called only in
kern_shutdown.c, this is more reasonable. This also lets us reduce the
number of ifdefs.

Also Remove the keysize parameter from mkdumpheader() and instead use
the configured dumper to get the key size. For textdumps we already
"hide" the configured crypto parameters by temporarily unsetting
dumper.kdc.

Test Plan

minidumps, encrypted minidumps, textdumps, 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 15 2017, 12:43 AM
markj edited the test plan for this revision. (Show Details)Jul 15 2017, 12:45 AM
markj added reviewers: cem, asomers, def, rgrimes.
cem accepted this revision.Jul 15 2017, 1:08 AM
cem added inline comments.
sys/kern/kern_shutdown.c
1217 ↗(On Diff #30805)

Maybe size_t or uint64_t (to match dumpsize) instead?

1280 ↗(On Diff #30805)

(Implicit zero-initialization of dumpkeysize in the non-EKCD case via bzero above.)

This revision is now accepted and ready to land.Jul 15 2017, 1:08 AM
def added inline comments.Aug 2 2017, 3:56 PM
sys/kern/kern_shutdown.c
1269 ↗(On Diff #30805)

dumpkeysize was passed to mkdumpheader() not to use the global dumper variable indirectly from MD code. I liked that we had only two functions where the dumper variable was used set_dumper() and doadump() which require some global state. The second one passes the dumper to MD code which calls dump API passing it as an argument.

What do you think about introducing dump_init_header() function which would have di as an argument so we could extract a kernel dump key size and a block size from it? Note that it doesn't make sense to create a kernel dump header and write it to a dump device which has a different block size from the one included in the header. Currently mkdumpheader() allows to do so and savecore(8) gets a dump device block size using ioctl(2), not from a kernel dump header. This issue existed before but we could fix it too meanwhile we're here.

markj added a comment.Aug 2 2017, 10:50 PM

(Oops, I forgot to submit my earlier comments in response to Conrad.)

sys/kern/kern_shutdown.c
1217 ↗(On Diff #30805)

I chose this to match the return type of kerneldumpcrypto_dumpkeysize(). I don't see any problem with keeping it a uint32_t?

1269 ↗(On Diff #30805)

Yup, that sounds reasonable to me. To be clear, you're talking about renaming mkdumpheader() and having it take di as an argument, rather than having both dump_init_header() and mkdumpheader()?

1280 ↗(On Diff #30805)

Yeah, I'll add an explicit initialization instead.

cem added inline comments.Aug 2 2017, 10:55 PM
sys/kern/kern_shutdown.c
1217 ↗(On Diff #30805)

There's no problem with it staying uint32_t.

def added inline comments.Aug 3 2017, 9:26 AM
sys/kern/kern_shutdown.c
1269 ↗(On Diff #30805)

Yes, I'd like to rename mkdumpheader() to dump_init_header(), provide a const di pointer as the first argument and remove blksz from the arguments list.

markj updated this revision to Diff 31603.Aug 4 2017, 11:46 PM
markj edited edge metadata.
markj marked 2 inline comments as done.
  • Rename mkdumpheader() to dump_init_header(), and have it take the dumperinfo as a parameter.
This revision now requires review to proceed.Aug 4 2017, 11:46 PM
def accepted this revision.Aug 6 2017, 3:51 PM
This revision is now accepted and ready to land.Aug 6 2017, 3:51 PM
This revision was automatically updated to reflect the committed changes.