Page MenuHomeFreeBSD
Differential D2698 Diff 8687 sys/kern/kern_shutdown.c

Changeset View

Standalone View

View Options

sys/kern/kern_shutdown.c

Show First 20 Lines • Show All 44 Lines • ▼ Show 20 Lines
#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/cons.h> #include <sys/cons.h>
#include <sys/eventhandler.h> #include <sys/eventhandler.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/mount.h> #include <sys/mount.h>
▲ Show 20 Lines • Show All 84 Lines • ▼ Show 20 Lines
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 */ static struct dumperinfo dumper; /* our selected dumper */
/* 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 = {
.d_version = D_VERSION,
.d_name = "reroot",
};
static void poweroff_wait(void *, int); static void poweroff_wait(void *, int);
static void shutdown_halt(void *junk, int howto); static void shutdown_halt(void *junk, int howto);
static void shutdown_panic(void *junk, int howto); static void shutdown_panic(void *junk, int howto);
static void shutdown_reset(void *junk, int howto); static void shutdown_reset(void *junk, int howto);
static int kern_reroot(void);
/* register various local shutdown events */ /* register various local shutdown events */
static void static void
shutdown_conf(void *unused) shutdown_conf(void *unused)
{ {
EVENTHANDLER_REGISTER(shutdown_final, poweroff_wait, NULL, EVENTHANDLER_REGISTER(shutdown_final, poweroff_wait, NULL,
SHUTDOWN_PRI_FIRST); SHUTDOWN_PRI_FIRST);
EVENTHANDLER_REGISTER(shutdown_final, shutdown_halt, NULL, EVENTHANDLER_REGISTER(shutdown_final, shutdown_halt, NULL,
SHUTDOWN_PRI_LAST + 100); SHUTDOWN_PRI_LAST + 100);
EVENTHANDLER_REGISTER(shutdown_final, shutdown_panic, NULL, EVENTHANDLER_REGISTER(shutdown_final, shutdown_panic, NULL,
SHUTDOWN_PRI_LAST + 100); SHUTDOWN_PRI_LAST + 100);
EVENTHANDLER_REGISTER(shutdown_final, shutdown_reset, NULL, EVENTHANDLER_REGISTER(shutdown_final, shutdown_reset, NULL,
SHUTDOWN_PRI_LAST + 200); SHUTDOWN_PRI_LAST + 200);
} }
SYSINIT(shutdown_conf, SI_SUB_INTRINSIC, SI_ORDER_ANY, shutdown_conf, NULL); SYSINIT(shutdown_conf, SI_SUB_INTRINSIC, SI_ORDER_ANY, shutdown_conf, NULL);
/* /*
* The only reason this exists is to create the /dev/reroot/ directory,
* used by reroot code in init(8) as a mountpoint for tmpfs.
*/
static void
reroot_conf(void *unused)
{
int error;
struct cdev *cdev;
error = make_dev_p(MAKEDEV_CHECKNAME, &cdev,
kibUnsubmitted
Done
Inline Actions

I recommend to add MAKEDEV_WAITOK explicitely.

kib: I recommend to add MAKEDEV_WAITOK explicitely.
&reroot_cdevsw, NULL, UID_ROOT, GID_WHEEL, 0600, "reroot/reroot");
if (error != 0) {
kibUnsubmitted
Not Done
Inline Actions

You must not signal the system processes at all.

kib: You must not signal the system processes at all.
printf("%s: failed to create device node, error %d",
kibUnsubmitted
Not Done
Inline Actions

Er ?

kib: Er ?
__func__, error);
}
}
SYSINIT(reroot_conf, SI_SUB_DEVFS, SI_ORDER_ANY, reroot_conf, NULL);
/*
* The system call that results in a reboot. * The system call that results in a reboot.
*/ */
/* ARGSUSED */ /* ARGSUSED */
int int
sys_reboot(struct thread *td, struct reboot_args *uap) sys_reboot(struct thread *td, struct reboot_args *uap)
kibUnsubmitted
Done
Inline Actions

What is the purpose of this ?

kib: What is the purpose of this ?
{ {
int error; int error;
error = 0; error = 0;
#ifdef MAC #ifdef MAC
error = mac_system_check_reboot(td->td_ucred, uap->opt); error = mac_system_check_reboot(td->td_ucred, uap->opt);
#endif #endif
if (error == 0) if (error == 0)
error = priv_check(td, PRIV_REBOOT); error = priv_check(td, PRIV_REBOOT);
if (error == 0) { if (error == 0) {
if (uap->opt & RB_REROOT) {
error = kern_reroot();
kibUnsubmitted
Done
Inline Actions

Why do you need Giant ?

kib: Why do you need Giant ?
traszAuthorUnsubmitted
Done
Inline Actions

To be extra safe - vfs_unmountall() was previously always called with Giant held; calling it without might uncover some bugs.

trasz: To be extra safe - vfs_unmountall() was previously always called with Giant held; calling it…
kibUnsubmitted
Done
Inline Actions

There is no gains from locking Giant there. It only confuses the readers.

kib: There is no gains from locking Giant there. It only confuses the readers.
traszAuthorUnsubmitted
Done
Inline Actions

Ok, if it's safe to call vfs_unmountall() and vfs_mountroot() without it, I'll just remove it.

trasz: Ok, if it's safe to call vfs_unmountall() and vfs_mountroot() without it, I'll just remove it.
} else {
mtx_lock(&Giant); mtx_lock(&Giant);
kern_reboot(uap->opt); kern_reboot(uap->opt);
swapoff_all();
kibUnsubmitted
Done
Inline Actions

I did not noticed this and the next line before. Do they belong to the changeset ?

kib: I did not noticed this and the next line before. Do they belong to the changeset ?
DELAY(100000); /* wait for console output to finish */
mtx_unlock(&Giant); mtx_unlock(&Giant);
} }
}
return (error); return (error);
} }
/* /*
* Called by events that want to shut down.. e.g <CTL><ALT><DEL> on a PC * Called by events that want to shut down.. e.g <CTL><ALT><DEL> on a PC
*/ */
void void
shutdown_nice(int howto) shutdown_nice(int howto)
▲ Show 20 Lines • Show All 125 Lines • ▼ Show 20 Lines#endif
if ((howto & (RB_HALT|RB_DUMP)) == RB_DUMP && !cold && !dumping) if ((howto & (RB_HALT|RB_DUMP)) == RB_DUMP && !cold && !dumping)
doadump(TRUE); doadump(TRUE);
/* Now that we're going to really halt the system... */ /* Now that we're going to really halt the system... */
EVENTHANDLER_INVOKE(shutdown_final, howto); EVENTHANDLER_INVOKE(shutdown_final, howto);
for(;;) ; /* safety against shutdown_reset not working */ for(;;) ; /* safety against shutdown_reset not working */
/* NOTREACHED */ /* NOTREACHED */
}
/*
* The system call that results in changing the rootfs.
*/
static int
kern_reroot(void)
{
struct vnode *oldrootvnode;
struct mount *mp, *devmp;
if (curproc != initproc)
kibUnsubmitted
Done
Inline Actions

this is better expressed as curproc != initproc

kib: this is better expressed as curproc != initproc
return (EPERM);
/*
* Remove the filesystem containing currently-running executable
* from the mount list, to prevent it from being unmounted
* by vfs_unmountall(), and to avoid confusing vfs_mountroot().
*
* Also preserve /dev - forcibly unmounting it could cause driver
wblockUnsubmitted
Done
Inline Actions

s/preserviing/preserve/

wblock: s/preserviing/preserve/
* reinitialization.
wblockUnsubmitted
Done
Inline Actions

Should be "currently-running".

wblock: Should be "currently-running".
*/
kibUnsubmitted
Done
Inline Actions

This is weird order of things. I would only cache v_mount after vn_lock succeeded.

kib: This is weird order of things. I would only cache v_mount after vn_lock succeeded.
mp = curproc->p_textvp->v_mount;
kibUnsubmitted
Done
Inline Actions

BTW, p_textvp could be already reclaimed, for many reasons. Then v_mount is NULL.

kib: BTW, p_textvp could be already reclaimed, for many reasons. Then v_mount is NULL.
devmp = rootdevmp;
rootdevmp = NULL;
mtx_lock(&mountlist_mtx);
TAILQ_REMOVE(&mountlist, mp, mnt_list);
TAILQ_REMOVE(&mountlist, devmp, mnt_list);
mtx_unlock(&mountlist_mtx);
kibUnsubmitted
Done
Inline Actions

I am looking at the code for vfs_mountroot() in the tree at r287124, and I do not understand why it would not mount an extra instance of the devfs on /dev.

Also, what makes vfs_mountroot() to mount the _new_ root ? I am missing the bits which pass the new configuration to the kernel.

kib: I am looking at the code for vfs_mountroot() in the tree at r287124, and I do not understand…
traszAuthorUnsubmitted
Done
Inline Actions

Well, since devfs forced unmount now works, it will mount the only instance of devfs on /dev.

I'd like to add this after this patch here goes in - it works correctly without it, and I'd like to avoid changes to vfs_mountroot.c, simply because I don't have a good way to test it properly.

As for the new root - again, it's described in reboot.8 change above.

trasz: Well, since devfs forced unmount now works, it will mount the only instance of devfs on /dev.
kibUnsubmitted
Done
Inline Actions

Ok, I missed that devfs is unmounted. Now I dislike this even more. My opinion is that inital devfs mount must be left as is and reused for new root.

kib: Ok, I missed that devfs is unmounted. Now I dislike this even more. My opinion is that inital…
traszAuthorUnsubmitted
Done
Inline Actions

To be honest, I'd prefer it the way it is now. I'm a huge fan of simplicity, and I just don't see the point of complicating the whole thing by treating devfs special in this particular case. Unless it would fix some actual bug. Would it?

trasz: To be honest, I'd prefer it the way it is now. I'm a huge fan of simplicity, and I just don't…
kibUnsubmitted
Done
Inline Actions

This is a false simplicity. When the devfs is unmounted, all devfs vnodes are reclaimed. This could cause unneccessary driver reinitialization, drop of the cached pages (since the vnode' vm_object is destroyed), and probably more consequences which I just do not bother to enumerate.

As an example of breakage, I believe that your process would loose the console if the filesystem is unmounted.

IMO not unmounting the initial devfs mount is simpler, if you look not at your code modifications, but at the whole system operation.

kib: This is a false simplicity. When the devfs is unmounted, all devfs vnodes are reclaimed. This…
oldrootvnode = rootvnode;
kibUnsubmitted
Done
Inline Actions

? Is there {} missed ?

kib: ? Is there {} missed ?
/*
* Unmount everything except for the two filesystems preserved above.
*/
vfs_unmountall();
/*
* Add /dev back; vfs_mountroot() will move it into its new place.
*/
mtx_lock(&mountlist_mtx);
kibUnsubmitted
Done
Inline Actions

Isn't this leaves the transient filesystem (the mp, which carries the init binary between remounts) not attached at any point in the current filesystem hierarchy ? I do not see anything which would make the filesystem accessible by path to the new namespace. In particular, I do not understand how unmount in the init(8) reroot code worked for you.

kib: Isn't this leaves the transient filesystem (the mp, which carries the init binary between…
traszAuthorUnsubmitted
Done
Inline Actions

Looks like sys_unmount() just iterates over the mountlist comparing f_mntonname. Should I try to reattach it (ie lookup the vnode for /mnt, set mnt_vnodecovered and v_mountedhere, etc) or just leave it as it is? The dounmount() contains code to handle mnt_vnodecovered being NULL, for some reason, so perhaps this doesn't work just by luck?

trasz: Looks like sys_unmount() just iterates over the mountlist comparing f_mntonname. Should I try…
kibUnsubmitted
Done
Inline Actions

I suggest to use some directory on the initial devfs mount for the safe harbor to keep the transient filesystem around. Also, you could consider adding yet another MNTK flag to mark both initial devfs and the transient filesystems as unmountable during the reroot (after reroot the flag must be cleared).

kib: I suggest to use some directory on the initial devfs mount for the safe harbor to keep the…
traszAuthorUnsubmitted
Done
Inline Actions

Again, is it required for fixing an actual bug in the code, or just a matter of personal preference?

trasz: Again, is it required for fixing an actual bug in the code, or just a matter of personal…
kibUnsubmitted
Done
Inline Actions

Consider it a personal taste based on some experience with our VFS I have. Having a filesystem not reachable by the usual means, even for the short time, is against too many pre-supposed assumptions in the VFS.

kib: Consider it a personal taste based on some experience with our VFS I have. Having a filesystem…
TAILQ_INSERT_HEAD(&mountlist, devmp, mnt_list);
mtx_unlock(&mountlist_mtx);
rootdevmp = devmp;
/*
* Mount the new rootfs.
*/
vfs_mountroot();
/*
* Update all references to the old rootvnode.
*/
mountcheckdirs(oldrootvnode, rootvnode);
/*
* Add the temporary filesystem back.
*/
mtx_lock(&mountlist_mtx);
TAILQ_INSERT_TAIL(&mountlist, mp, mnt_list);
mtx_unlock(&mountlist_mtx);
return (0);
} }
/* /*
* If the shutdown was a clean halt, behave accordingly. * If the shutdown was a clean halt, behave accordingly.
*/ */
static void static void
shutdown_halt(void *junk, int howto) shutdown_halt(void *junk, int howto)
{ {
▲ Show 20 Lines • Show All 427 Lines • Show Last 20 Lines