Messages in this thread | | | From | (Eric W. Biederman) | Date | Thu, 19 Jun 2008 14:50:49 -0700 | Subject | Re: [PATCH] X86: reboot-notify additions |
| |
Cliff Wickman <cpw@sgi.com> writes:
> On Thu, Jun 19, 2008 at 01:02:14PM +0200, Ingo Molnar wrote: >> >> * Cliff Wickman <cpw@sgi.com> wrote: >> >> > From: Cliff Wickman <cpw@sgi.com> >> > >> > X86 reboot-notify additions. > > As Andi Kleen pointed out, this is not X86-specific. (it started out > to be, but what I hoped to achieve for X86 turns out to be generic). > >> > This patch adds scans of the "reboot_notifier_list" callback chain in >> > a three other places where the kernel is being stopped and/or restarted. >> > >> > Adds calls to blocking_notifier_call_chain() in: >> > crash_kexec(), emergency_restart(), sys_kexec_load() >> > >> > In the crash_kexec() and emergency_restart() cases it is indicated to the >> > called-back function that the system is not in a sane state, so that >> > it can avoid taking a lock or some such potentially blocking action. >> > >> > These callbacks are important to a partition system. The stopped kernel > needs >> > to inform other partitions of their need to disconnect (stop sharing > memory). >> > >> > Diffed against 2.6.26-rc6 >> > >> > Signed-off-by: Cliff Wickman <cpw@sgi.com> >> > --- >> > include/linux/notifier.h | 4 ++++ >> > kernel/kexec.c | 5 +++++ >> > kernel/sys.c | 1 + >> > 3 files changed, 10 insertions(+) >> > >> > Index: linux/include/linux/notifier.h >> > =================================================================== >> > --- linux.orig/include/linux/notifier.h >> > +++ linux/include/linux/notifier.h >> > @@ -202,6 +202,10 @@ static inline int notifier_to_errno(int >> > #define SYS_RESTART SYS_DOWN >> > #define SYS_HALT 0x0002 /* Notify of system halt */ >> > #define SYS_POWER_OFF 0x0003 /* Notify of system power off */ >> > +#define SYS_INSANE 0x0004 /* Notify of system error/panic/oops */ >> > +/* For the SYS_INSANE case, no locks should be taken by the called-back >> > + * function. The kernel is ready for an immediate reboot. >> > + */ >> > >> > #define NETLINK_URELEASE 0x0001 /* Unicast netlink socket released */ >> > >> > Index: linux/kernel/kexec.c >> > =================================================================== >> > --- linux.orig/kernel/kexec.c >> > +++ linux/kernel/kexec.c >> > @@ -1001,6 +1001,9 @@ asmlinkage long sys_kexec_load(unsigned >> > if (result) >> > goto out; >> > } >> > + >> > + blocking_notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL); >> > + >> > /* Install the new kernel, and Uninstall the old */ >> > image = xchg(dest_image, image); >> > >> > @@ -1068,6 +1071,8 @@ void crash_kexec(struct pt_regs *regs) >> > if (!locked) { >> > if (kexec_crash_image) { >> > struct pt_regs fixed_regs; >> > + blocking_notifier_call_chain(&reboot_notifier_list, >> > + SYS_INSANE, NULL); >> > crash_setup_regs(&fixed_regs, regs); >> > crash_save_vmcoreinfo(); >> > machine_crash_shutdown(&fixed_regs); >> > Index: linux/kernel/sys.c >> > =================================================================== >> > --- linux.orig/kernel/sys.c >> > +++ linux/kernel/sys.c >> > @@ -270,6 +270,7 @@ out_unlock: >> > */ >> > void emergency_restart(void) >> > { >> > + blocking_notifier_call_chain(&reboot_notifier_list, SYS_INSANE, NULL); >> > machine_emergency_restart(); >> > } >> > EXPORT_SYMBOL_GPL(emergency_restart); >> >> i dont think this is a good idea. reboot_notifier_list is a blocking >> notifier, i.e. it comes with a notifier->rwsem read-write mutex that is >> taken when blocking_notifier_call_chain() is executed. >> >> i.e. this patch puts a sleeping mutex operation (a down_read()) into a >> highly critical code path of the kernel. This will decrease the >> reliability of the kernel. > > Andi pointed this out, too. > > For these emergency cases (I'll change "SYS_INSANE" to "SYS_EMERGENCY") > I probably should be using raw_notifier_call_chain(), which requires > a slightly different form of list header but doesn't try to protect > against someone else adding to the notifier list. >> >> what exactly are you trying to achieve? >> >> Ingo > > The impetus for these additions is to call back a driver in every case > that the kernel is going down. In a partitioned system we need such a > driver to inform all other partitions that they need to disconnect from > the rebooting/halting/panicing partition (kernel image). If they are not > informed, they may bring themselves crashing down as well. > (xpc is such a cross_partition driver)
Cool. Someone who wants this kind of functionality and has code in the kernel. Perhaps we can have a reasonable discussion about this. The last time this came up people wanted a hook so they could support their out of tree blobs in an enterprise kernel.
emergency_restart only happens or only should happen with explicit admin request Sysreq-r. Or when a watchdog detects the system is borked. By design it is not expected to call drivers. The kexec on panic case is similar.
sys_kexec_load is a ridiculous place to call any kind of reboot notifier because it is a prep function that doesn't require any kind of connection to rebooting.
As far as this being a generic problem I half agree. It seems to depend on your platform if you need something like this.
With that said. I suggest we have a single platform specific function that can be called. Possibly something like pm_power_off. It is insanely important that we be able to audit all of the code on these code paths, and that a random loadable driver not be able to get in and mess things up.
Eric
| |