Re: port-i386/57197: GENERIC kernel crash on pentium-III and earlier CPUs

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

Can you try the attached patch?

From e20d2a498a991899ab794174c5fcae888cbc84a4 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Tue, 24 Jan 2023 10:00:45 +0000
Subject: [PATCH] x86/intr: Work around sleazy clockintr with a secret frame
 argument.

PR kern/57197
---
 sys/arch/x86/include/intr_private.h | 39 +++++++++++++++++++++++++++++
 sys/arch/x86/isa/clock.c            | 14 +++++++----
 sys/arch/x86/x86/intr.c             | 16 ++++++++++--
 3 files changed, 62 insertions(+), 7 deletions(-)
 create mode 100644 sys/arch/x86/include/intr_private.h

diff --git a/sys/arch/x86/include/intr_private.h b/sys/arch/x86/include/intr_private.h
new file mode 100644
index 000000000000..183e904a7dba
--- /dev/null
+++ b/sys/arch/x86/include/intr_private.h
@@ -0,0 +1,39 @@
+/*	$NetBSD$	*/
+
+/*-
+ * Copyright (c) 2023 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef	_X86_INTR_PRIVATE_H_
+#define	_X86_INTR_PRIVATE_H_
+
+/*
+ * XXX This is a horrible kludge to let intr_establish_xname detect
+ * when it needs to handle a sleazy extra argument to the interrupt
+ * handler that's not part of the normal interrupt handler signature.
+ */
+int i8254_clockintr(void *, struct intrframe *);
+
+#endif	/* _X86_INTR_PRIVATE_H_ */
diff --git a/sys/arch/x86/isa/clock.c b/sys/arch/x86/isa/clock.c
index c50704cd13a8..399bbd46c6ed 100644
--- a/sys/arch/x86/isa/clock.c
+++ b/sys/arch/x86/isa/clock.c
@@ -152,6 +152,7 @@ __KERNEL_RCSID(0, "$NetBSD: clock.c,v 1.39 2020/05/29 12:30:41 rin Exp $");
 #include <x86/lock.h>
 #include <machine/specialreg.h>
 #include <x86/rtc.h>
+#include <x86/intr_private.h>
 
 #ifndef __x86_64__
 #include "mca.h"
@@ -188,8 +189,6 @@ void (*x86_delay)(unsigned int) = i8254_delay;
 void		sysbeep(int, int);
 static void     tickle_tc(void);
 
-static int	clockintr(void *, struct intrframe *);
-
 int 		sysbeepdetach(device_t, int);
 
 static unsigned int	gettick_broken_latch(void);
@@ -371,8 +370,8 @@ tickle_tc(void)
 
 }
 
-static int
-clockintr(void *arg, struct intrframe *frame)
+int
+i8254_clockintr(void *arg, struct intrframe *frame)
 {
 	tickle_tc();
 
@@ -555,9 +554,14 @@ i8254_initclocks(void)
 	/*
 	 * XXX If you're doing strange things with multiple clocks, you might
 	 * want to keep track of clock handlers.
+	 *
+	 * XXX This is an abuse of the interrupt handler signature with
+	 * __FPTRCAST which requires a special case for IPL_CLOCK in
+	 * intr_establish_xname.  Please fix this nonsense!  See also
+	 * the comments about i8254_clockintr in x86/x86/intr.c.
 	 */
 	(void)isa_intr_establish(NULL, 0, IST_PULSE, IPL_CLOCK,
-	    __FPTRCAST(int (*)(void *), clockintr), 0);
+	    __FPTRCAST(int (*)(void *), i8254_clockintr), 0);
 }
 
 void
diff --git a/sys/arch/x86/x86/intr.c b/sys/arch/x86/x86/intr.c
index 5bde34cf7514..54474897377a 100644
--- a/sys/arch/x86/x86/intr.c
+++ b/sys/arch/x86/x86/intr.c
@@ -162,6 +162,8 @@ __KERNEL_RCSID(0, "$NetBSD: intr.c,v 1.163 2022/10/29 13:59:04 riastradh Exp $")
 #include <machine/i8259.h>
 #include <machine/pio.h>
 
+#include <x86/intr_private.h>
+
 #include "ioapic.h"
 #include "lapic.h"
 #include "pci.h"
@@ -944,11 +946,21 @@ intr_establish_xname(int legacy_irq, struct pic *pic, int pin, int type,
 	ih->ih_slot = slot;
 	strlcpy(ih->ih_xname, xname, sizeof(ih->ih_xname));
 #ifdef KDTRACE_HOOKS
-	ih->ih_fun = intr_kdtrace_wrapper;
-	ih->ih_arg = ih;
+	/*
+	 * XXX i8254_clockintr is special -- takes a magic extra
+	 * argument.  This should be fixed properly in some way that
+	 * doesn't involve sketchy function pointer casts.  See also
+	 * the comments in x86/isa/clock.c.
+	 */
+	if (handler != __FPTRCAST(int (*)(void *), i8254_clockintr)) {
+		ih->ih_fun = intr_kdtrace_wrapper;
+		ih->ih_arg = ih;
+	}
 #endif
 #ifdef MULTIPROCESSOR
 	if (!mpsafe) {
+		KASSERT(handler !=			/* XXX */
+		    __FPTRCAST(int (*)(void *), i8254_clockintr));
 		ih->ih_fun = intr_biglock_wrapper;
 		ih->ih_arg = ih;
 	}