Page MenuHomeFreeBSD
Differential D19996 Diff 56897 sys/kern/kern_shutdown.c

Changeset View

Standalone View

View Options

sys/kern/kern_shutdown.c

Show All 37 Lines
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include "opt_ddb.h" #include "opt_ddb.h"
#include "opt_ekcd.h" #include "opt_ekcd.h"
#include "opt_kdb.h" #include "opt_kdb.h"
#include "opt_panic.h" #include "opt_panic.h"
#include "opt_printf.h"
#include "opt_sched.h" #include "opt_sched.h"
#include "opt_watchdog.h" #include "opt_watchdog.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/systm.h> #include <sys/systm.h>
#include <sys/bio.h> #include <sys/bio.h>
#include <sys/buf.h> #include <sys/buf.h>
#include <sys/conf.h> #include <sys/conf.h>
#include <sys/compressor.h> #include <sys/compressor.h>
#include <sys/cons.h> #include <sys/cons.h>
#include <sys/disk.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.h>
#include <sys/filedesc.h> #include <sys/filedesc.h>
#include <sys/jail.h> #include <sys/jail.h>
#include <sys/kdb.h> #include <sys/kdb.h>
#include <sys/kernel.h> #include <sys/kernel.h>
#include <sys/kerneldump.h> #include <sys/kerneldump.h>
#include <sys/kthread.h> #include <sys/kthread.h>
#include <sys/ktr.h> #include <sys/ktr.h>
#include <sys/malloc.h> #include <sys/malloc.h>
#include <sys/mbuf.h> #include <sys/mbuf.h>
#include <sys/mount.h> #include <sys/mount.h>
#include <sys/priv.h> #include <sys/priv.h>
#include <sys/proc.h> #include <sys/proc.h>
#include <sys/reboot.h> #include <sys/reboot.h>
#include <sys/resourcevar.h> #include <sys/resourcevar.h>
#include <sys/rwlock.h> #include <sys/rwlock.h>
#include <sys/sbuf.h>
#include <sys/sched.h> #include <sys/sched.h>
#include <sys/smp.h> #include <sys/smp.h>
#include <sys/sysctl.h> #include <sys/sysctl.h>
#include <sys/sysproto.h> #include <sys/sysproto.h>
#include <sys/taskqueue.h> #include <sys/taskqueue.h>
#include <sys/vnode.h> #include <sys/vnode.h>
#include <sys/watchdog.h> #include <sys/watchdog.h>
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines
/* /*
* Variable panicstr contains argument to first call to panic; used as flag * Variable panicstr contains argument to first call to panic; used as flag
* to indicate that the kernel has already called panic. * to indicate that the kernel has already called panic.
*/ */
const char *panicstr; const char *panicstr;
int dumping; /* system is dumping */ int dumping; /* system is dumping */
int rebooting; /* system is rebooting */ int rebooting; /* system is rebooting */
static struct dumperinfo dumper; /* our selected dumper */ /*
* Used to serialize between sysctl kern.shutdown.dumpdevname and list
* modifications via ioctl.
markjUnsubmitted
Done
Inline Actions

I'm not sure what "unprotected" means here.

markj: I'm not sure what "unprotected" means here.
cemAuthorUnsubmitted
Done
Inline Actions

More correct would be “unprivileged.” We shouldn’t crash if root manipulates a kernel data structure concurrently with access either, but it’s slightly better. I will probably remove the adjective and just mention concurrent manipulation and the sysctl.

cem: More correct would be “unprivileged.” We shouldn’t crash if root manipulates a kernel data…
*/
static struct mtx dumpconf_list_lk;
MTX_SYSINIT(dumper_configs, &dumpconf_list_lk, "dumper config list", MTX_DEF);
/* Our selected dumper(s). */
markjUnsubmitted
Done
Inline Actions

Convert to a full sentence?

markj: Convert to a full sentence?
cemAuthorUnsubmitted
Done
Inline Actions

Does “These are...” make the comment easier to understand? I doubt it, though it improves style(9) conformance. I inherited this one, just added “(s)”. Will fix for style sake.

cem: Does “These are...” make the comment easier to understand? I doubt it, though it improves style…
markjUnsubmitted
Done
Inline Actions

I just meant to capitalize and add a period since the comment is on its own line now.

markj: I just meant to capitalize and add a period since the comment is on its own line now.
static TAILQ_HEAD(dumpconflist, dumperinfo) dumper_configs =
TAILQ_HEAD_INITIALIZER(dumper_configs);
/* Context information for dump-debuggers. */ /* Context information for dump-debuggers. */
static struct pcb dumppcb; /* Registers. */ static struct pcb dumppcb; /* Registers. */
lwpid_t dumptid; /* Thread ID. */ lwpid_t dumptid; /* Thread ID. */
static struct cdevsw reroot_cdevsw = { static struct cdevsw reroot_cdevsw = {
.d_version = D_VERSION, .d_version = D_VERSION,
.d_name = "reroot", .d_name = "reroot",
}; };
▲ Show 20 Lines • Show All 137 Lines • ▼ Show 20 Lines
doadump(boolean_t textdump) doadump(boolean_t textdump)
{ {
boolean_t coredump; boolean_t coredump;
int error; int error;
error = 0; error = 0;
if (dumping) if (dumping)
return (EBUSY); return (EBUSY);
if (dumper.dumper == NULL) if (TAILQ_EMPTY(&dumper_configs))
return (ENXIO); return (ENXIO);
savectx(&dumppcb); savectx(&dumppcb);
dumptid = curthread->td_tid; dumptid = curthread->td_tid;
dumping++; dumping++;
coredump = TRUE; coredump = TRUE;
#ifdef DDB #ifdef DDB
if (textdump && textdump_pending) { if (textdump && textdump_pending) {
coredump = FALSE; coredump = FALSE;
textdump_dumpsys(&dumper); textdump_dumpsys(TAILQ_FIRST(&dumper_configs));
} }
#endif #endif
if (coredump) if (coredump) {
error = dumpsys(&dumper); struct dumperinfo *di;
TAILQ_FOREACH(di, &dumper_configs, di_next) {
error = dumpsys(di);
if (error == 0)
break;
}
}
dumping--; dumping--;
return (error); return (error);
} }
/* /*
* Shutdown the system cleanly to prepare for reboot, halt, or power off. * Shutdown the system cleanly to prepare for reboot, halt, or power off.
*/ */
void void
▲ Show 20 Lines • Show All 555 Lines • ▼ Show 20 Lineskthread_shutdown(void *arg, int howto)
error = kthread_suspend(td, kproc_shutdown_wait * hz); error = kthread_suspend(td, kproc_shutdown_wait * hz);
if (error == EWOULDBLOCK) if (error == EWOULDBLOCK)
printf("timed out\n"); printf("timed out\n");
else else
printf("done\n"); printf("done\n");
} }
static char dumpdevname[sizeof(((struct cdev*)NULL)->si_name)]; static int
SYSCTL_STRING(_kern_shutdown, OID_AUTO, dumpdevname, CTLFLAG_RD, dumpdevname_sysctl_handler(SYSCTL_HANDLER_ARGS)
dumpdevname, 0, "Device for kernel dumps"); {
char buf[256];
cemAuthorUnsubmitted
Done
Inline Actions

Tinderbox — not all kernel configurations specify PRINTF_BUFR_SIZE :-(.

cem: Tinderbox — not all kernel configurations specify `PRINTF_BUFR_SIZE` :-(.
struct dumperinfo *di;
struct sbuf sb;
int error;
error = sysctl_wire_old_buffer(req, 0);
if (error != 0)
return (error);
scottlUnsubmitted
Done
Inline Actions

Consider using sbuf_new_for_sysctl() here.

scottl: Consider using sbuf_new_for_sysctl() here.
scottlUnsubmitted
Done
Inline Actions

You'll also need to add sbuf_wire_old_buffer()

scottl: You'll also need to add sbuf_wire_old_buffer()
cemAuthorUnsubmitted
Done
Inline Actions

Yeah, I explicitly did not use sbuf_new_for_sysctl to avoid sleeping in drain with the mtx held. sysctl_wire_old_buffer sounds like it might be a solution to that, modulo it allows unprivileged users to wire memory.

The tradeoff seems to be: potentially truncated result due to limited stack buffer vs allow wiring arbitrarily large, unprivileged user buffers (DoS vector).

It wouldn't be the first sysctl allowing that, though, so it's not much of a downside. I'll plan to make the change.

cem: Yeah, I explicitly did not use sbuf_new_for_sysctl to avoid sleeping in drain with the mtx held.
markjUnsubmitted
Done
Inline Actions

There's a lot of other sysctl handlers which happily wire user memory without such a check, and in general unprivileged users are permitted to wire a small amount (more than a page) of physical memory anyway. Rather than having ad-hoc policies implemented in sysctl handlers, I'd prefer to handle the issue globally in vslock() (which currently gives you a very loose seatbelt).

markj: There's a lot of other sysctl handlers which happily wire user memory without such a check, and…
cemAuthorUnsubmitted
Done
Inline Actions

On the one hand, sure. On the other hand, to exceed a 4k page, even with ~255 devices (theoretical max based on uint8_t kda_index), would take just over 16 bytes per device name on average (which is much longer than typical for network devices or even disk partitions). It's just a relatively huge amount of memory relative to what is needed and also the minimum unit we can wire, so I figured it was pretty reasonable.

I'm happy to remove it, though. Will plan to do that.

cem: On the one hand, sure. On the other hand, to exceed a 4k page, even with ~255 devices…
sbuf_new_for_sysctl(&sb, buf, sizeof(buf), req);
mtx_lock(&dumpconf_list_lk);
TAILQ_FOREACH(di, &dumper_configs, di_next) {
if (di != TAILQ_FIRST(&dumper_configs))
sbuf_putc(&sb, ',');
sbuf_cat(&sb, di->di_devname);
}
mtx_unlock(&dumpconf_list_lk);
error = sbuf_finish(&sb);
markjUnsubmitted
Done
Inline Actions

You can replace the flag variable with if (di == TAILQ_FIRST(&dumper_configs)).

markj: You can replace the flag variable with `if (di == TAILQ_FIRST(&dumper_configs))`.
cemAuthorUnsubmitted
Done
Inline Actions

Ok

cem: Ok
sbuf_delete(&sb);
return (error);
}
SYSCTL_PROC(_kern_shutdown, OID_AUTO, dumpdevname, CTLTYPE_STRING | CTLFLAG_RD,
&dumper_configs, 0, dumpdevname_sysctl_handler, "A",
"Device(s) for kernel dumps");
static int _dump_append(struct dumperinfo *di, void *virtual, static int _dump_append(struct dumperinfo *di, void *virtual,
vm_offset_t physical, size_t length); vm_offset_t physical, size_t length);
#ifdef EKCD #ifdef EKCD
static struct kerneldumpcrypto * static struct kerneldumpcrypto *
kerneldumpcrypto_create(size_t blocksize, uint8_t encryption, kerneldumpcrypto_create(size_t blocksize, uint8_t encryption,
const uint8_t *key, uint32_t encryptedkeysize, const uint8_t *encryptedkey) const uint8_t *key, uint32_t encryptedkeysize, const uint8_t *encryptedkey)
{ {
▲ Show 20 Lines • Show All 120 Lines • ▼ Show 20 Lineskerneldumpcomp_destroy(struct dumperinfo *di)
if (kdcomp == NULL) if (kdcomp == NULL)
return; return;
compressor_fini(kdcomp->kdc_stream); compressor_fini(kdcomp->kdc_stream);
explicit_bzero(kdcomp->kdc_buf, di->maxiosize); explicit_bzero(kdcomp->kdc_buf, di->maxiosize);
free(kdcomp->kdc_buf, M_DUMPER); free(kdcomp->kdc_buf, M_DUMPER);
free(kdcomp, M_DUMPER); free(kdcomp, M_DUMPER);
} }
/*
* Must not be present on global list.
*/
static void
free_single_dumper(struct dumperinfo *di)
{
if (di == NULL)
return;
if (di->blockbuf != NULL) {
explicit_bzero(di->blockbuf, di->blocksize);
free(di->blockbuf, M_DUMPER);
}
kerneldumpcomp_destroy(di);
#ifdef EKCD
if (di->kdcrypto != NULL) {
explicit_bzero(di->kdcrypto, sizeof(*di->kdcrypto) +
di->kdcrypto->kdc_dumpkeysize);
free(di->kdcrypto, M_EKCD);
}
#endif
explicit_bzero(di, sizeof(*di));
free(di, M_DUMPER);
}
/* Registration of dumpers */ /* Registration of dumpers */
int int
set_dumper(struct dumperinfo *di, const char *devname, struct thread *td, dumper_insert(const struct dumperinfo *di_template, const char *devname,
uint8_t compression, uint8_t encryption, const uint8_t *key, const struct diocskerneldump_arg *kda)
uint32_t encryptedkeysize, const uint8_t *encryptedkey)
{ {
size_t wantcopy; struct dumperinfo *newdi, *listdi;
bool inserted;
uint8_t index;
int error; int error;
error = priv_check(td, PRIV_SETDUMPER); index = kda->kda_index;
MPASS(index != KDA_REMOVE && index != KDA_REMOVE_DEV &&
index != KDA_REMOVE_ALL);
error = priv_check(curthread, PRIV_SETDUMPER);
if (error != 0) if (error != 0)
return (error); return (error);
if (dumper.dumper != NULL) newdi = malloc(sizeof(*newdi) + strlen(devname) + 1, M_DUMPER, M_WAITOK
return (EBUSY); | M_ZERO);
dumper = *di; memcpy(newdi, di_template, sizeof(*newdi));
markjUnsubmitted
Done
Inline Actions

I know the old code did this, but an explict memcpy() might be nicer. I've gotten confused here before because I'm not used to seeing structures being copied this way.

markj: I know the old code did this, but an explict memcpy() might be nicer. I've gotten confused here…
cemAuthorUnsubmitted
Done
Inline Actions

I don’t have a strong preference for either. I’d guess there are more people confused about this than harmed by memcpy, so I’m happy to change it.

cem: I don’t have a strong preference for either. I’d guess there are more people confused about…
dumper.blockbuf = NULL; newdi->blockbuf = NULL;
dumper.kdcrypto = NULL; newdi->kdcrypto = NULL;
dumper.kdcomp = NULL; newdi->kdcomp = NULL;
strcpy(newdi->di_devname, devname);
if (encryption != KERNELDUMP_ENC_NONE) { if (kda->kda_encryption != KERNELDUMP_ENC_NONE) {
#ifdef EKCD #ifdef EKCD
dumper.kdcrypto = kerneldumpcrypto_create(di->blocksize, newdi->kdcrypto = kerneldumpcrypto_create(di_template->blocksize,
encryption, key, encryptedkeysize, encryptedkey); kda->kda_encryption, kda->kda_key,
if (dumper.kdcrypto == NULL) { kda->kda_encryptedkeysize, kda->kda_encryptedkey);
if (newdi->kdcrypto == NULL) {
error = EINVAL; error = EINVAL;
goto cleanup; goto cleanup;
} }
#else #else
error = EOPNOTSUPP; error = EOPNOTSUPP;
goto cleanup; goto cleanup;
#endif #endif
} }
if (kda->kda_compression != KERNELDUMP_COMP_NONE) {
wantcopy = strlcpy(dumpdevname, devname, sizeof(dumpdevname));
if (wantcopy >= sizeof(dumpdevname)) {
printf("set_dumper: device name truncated from '%s' -> '%s'\n",
devname, dumpdevname);
}
if (compression != KERNELDUMP_COMP_NONE) {
/* /*
* We currently can't support simultaneous encryption and * We currently can't support simultaneous encryption and
* compression. * compression because our only encryption mode is an unpadded
* block cipher, go figure. This is low hanging fruit to fix.
*/ */
if (encryption != KERNELDUMP_ENC_NONE) { if (kda->kda_encryption != KERNELDUMP_ENC_NONE) {
error = EOPNOTSUPP; error = EOPNOTSUPP;
goto cleanup; goto cleanup;
} }
dumper.kdcomp = kerneldumpcomp_create(&dumper, compression); newdi->kdcomp = kerneldumpcomp_create(newdi,
if (dumper.kdcomp == NULL) { kda->kda_compression);
if (newdi->kdcomp == NULL) {
error = EINVAL; error = EINVAL;
goto cleanup; goto cleanup;
} }
} }
dumper.blockbuf = malloc(di->blocksize, M_DUMPER, M_WAITOK | M_ZERO); newdi->blockbuf = malloc(newdi->blocksize, M_DUMPER, M_WAITOK | M_ZERO);
/* Add the new configuration to the queue */
mtx_lock(&dumpconf_list_lk);
inserted = false;
TAILQ_FOREACH(listdi, &dumper_configs, di_next) {
if (index == 0) {
TAILQ_INSERT_BEFORE(listdi, newdi, di_next);
inserted = true;
break;
}
index--;
}
if (!inserted)
TAILQ_INSERT_TAIL(&dumper_configs, newdi, di_next);
mtx_unlock(&dumpconf_list_lk);
return (0); return (0);
cleanup: cleanup:
(void)clear_dumper(td); free_single_dumper(newdi);
return (error); return (error);
} }
int static bool
clear_dumper(struct thread *td) dumper_config_match(const struct dumperinfo *di, const char *devname,
const struct diocskerneldump_arg *kda)
{ {
int error; if (kda->kda_index == KDA_REMOVE_ALL)
return (true);
error = priv_check(td, PRIV_SETDUMPER); if (strcmp(di->di_devname, devname) != 0)
if (error != 0) return (false);
return (error);
#ifdef NETDUMP /*
netdump_mbuf_drain(); * Allow wildcard removal of configs matching a device on g_dev_orphan.
#endif */
if (kda->kda_index == KDA_REMOVE_DEV)
return (true);
if (di->kdcomp != NULL) {
if (di->kdcomp->kdc_format != kda->kda_compression)
return (false);
} else if (kda->kda_compression != KERNELDUMP_COMP_NONE)
markjUnsubmitted
Done
Inline Actions

I know encryption and compression can't be configured simultaneously right now, but that could easily change in the future. Is there any other reason to use an else if here rather than plain if?

markj: I know encryption and compression can't be configured simultaneously right now, but that could…
cemAuthorUnsubmitted
Done
Inline Actions

This branch isn't related to our current compression+encryption limitation. It's just asserting that a non-NONE kda_compression mode doesn't match kdcomp == NULL. Plain if would require restating kdcomp == NULL && which we inherit via else if.

cem: This branch isn't related to our current compression+encryption limitation. It's just…
return (false);
#ifdef EKCD #ifdef EKCD
if (dumper.kdcrypto != NULL) { if (di->kdcrypto != NULL) {
explicit_bzero(dumper.kdcrypto, sizeof(*dumper.kdcrypto) + if (di->kdcrypto->kdc_encryption != kda->kda_encryption)
dumper.kdcrypto->kdc_dumpkeysize); return (false);
free(dumper.kdcrypto, M_EKCD); /*
} * Do we care to verify keys match to delete? It seems weird
* to expect multiple fallback dump configurations on the same
* device that only differ in crypto key.
*/
} else
#endif #endif
if (kda->kda_encryption != KERNELDUMP_ENC_NONE)
return (false);
kerneldumpcomp_destroy(&dumper); return (true);
}
if (dumper.blockbuf != NULL) { int
explicit_bzero(dumper.blockbuf, dumper.blocksize); dumper_remove(const char *devname, const struct diocskerneldump_arg *kda)
free(dumper.blockbuf, M_DUMPER); {
struct dumperinfo *di, *sdi;
bool found;
int error;
error = priv_check(curthread, PRIV_SETDUMPER);
if (error != 0)
return (error);
/*
* Try to find a matching configuration, and kill it.
*
* NULL 'kda' indicates remove any configuration matching 'devname',
* which may remove multiple configurations in atypical configurations.
*/
found = false;
mtx_lock(&dumpconf_list_lk);
TAILQ_FOREACH_SAFE(di, &dumper_configs, di_next, sdi) {
if (dumper_config_match(di, devname, kda)) {
found = true;
TAILQ_REMOVE(&dumper_configs, di, di_next);
free_single_dumper(di);
markjUnsubmitted
Not Done
Inline Actions

Is it possible to match multiple devices?

markj: Is it possible to match multiple devices?
cemAuthorUnsubmitted
Done
Inline Actions

Yes - this allows things like “dumpon off” to continue to disable everything without iterating all dump configurations explicitly. Are you thinking the sys/disk.h comment could specify that more clearly? For that matter, it’s probably more appropriate in the .c implementation anyway.

cem: Yes - this allows things like “dumpon off” to continue to disable everything without iterating…
markjUnsubmitted
Done
Inline Actions

It might be worth noting that wildcard matches are permitted.

markj: It might be worth noting that wildcard matches are permitted.
} }
explicit_bzero(&dumper, sizeof(dumper)); }
dumpdevname[0] = '\0'; mtx_unlock(&dumpconf_list_lk);
/* Only produce ENOENT if a more targetted match didn't match. */
if (!found && kda->kda_index == KDA_REMOVE)
return (ENOENT);
cemAuthorUnsubmitted
Done
Inline Actions

At least REMOVE_ALL shouldn't error; it breaks the 12 ABI to do so (unless we explicitly squash it in 12-compat callers) because dumpon(8) in 12 always does a clear first, even if no dump device is set, before setting a dump device.

cem: At least REMOVE_ALL shouldn't error; it breaks the 12 ABI to do so (unless we explicitly squash…
return (0); return (0);
} }
static int static int
dump_check_bounds(struct dumperinfo *di, off_t offset, size_t length) dump_check_bounds(struct dumperinfo *di, off_t offset, size_t length)
{ {
if (di->mediasize > 0 && length != 0 && (offset < di->mediaoffset || if (di->mediasize > 0 && length != 0 && (offset < di->mediaoffset ||
▲ Show 20 Lines • Show All 390 Lines • Show Last 20 Lines