re: Problem with syscall_disestablish() - PR kern/50430

[Paul Goyette <paul%vps1.whooppee.com@localhost>]

Based on earlier comments, I've come up with a much-less-intrusive
set of changes.  This time around, there are no bit masks and no new
members in any system structures.  (I'm pretty sure we won't even
need a kernel version bump for this.)

Instead, I've modified sy_call() to check the current value of
l_sysent before updating with the new syscall pointer.  If the
current value is non-NULL then set a new flag bit LP_LOST_SYSENT in
the lwp's l_pflag member.  This check is the only per-syscall cost:

	if (l->l_sysent != NULL)
		l->l_pflag |= LP_LOST_SYSENT;

There are no new considerations for syscall_establish().

For syscall_disestablish()'s FOREACH lwp loop, in addition to
checking to see if the lwp has the "target" syscall active, we check
to see if the lwp has the LP_LOST_SYSENT flag set.  If _any_ active
process has this flag set, we cannot know if it has the target
syscall active or not, so we err on the side of caution and disallow
removal of the syscall.

> > > there's a fairly significant problem with this implementation.
> > >
> > > you're only catching callers who use end up going through sy_call()
> > > and that's not the majority.  mostly they're called directly from
> > > MD code.  so to fix that, you have to update every platform syscall
> > > handler.

FWIW, I did check the archives from 8 years ago when ad@ added the
sy_call() wrapper.  It appears that he had already addressed the
several md-specific syscall dispatchers to use the wrapper, so it
seems that we don't need to do anything special.  See [1] for the
relevant commit.  If you can point out any other code that still
needs to be updated, please do!

[1]http://mail-index.netbsd.org/source-changes/2008/10/21/msg211565.html




+------------------+--------------------------+-------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org  |
+------------------+--------------------------+-------------------------+

Index: kern/kern_syscall.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_syscall.c,v
retrieving revision 1.11
diff -u -p -r1.11 kern_syscall.c
--- kern/kern_syscall.c	9 May 2015 05:56:36 -0000	1.11
+++ kern/kern_syscall.c	17 Nov 2015 22:55:38 -0000
@@ -178,7 +178,8 @@ syscall_disestablish(const struct emul *
 	for (i = 0; sp[i].sp_call != NULL; i++) {
 		mutex_enter(proc_lock);
 		LIST_FOREACH(l, &alllwp, l_list) {
-			if (l->l_sysent == &sy[sp[i].sp_code]) {
+			if (l->l_sysent == &sy[sp[i].sp_code] ||
+			    (l->l_pflag && LP_LOST_SYSENT)) {
 				break;
 			}
 		}
Index: sys/lwp.h
===================================================================
RCS file: /cvsroot/src/sys/sys/lwp.h,v
retrieving revision 1.170
diff -u -p -r1.170 lwp.h
--- sys/lwp.h	31 Mar 2015 01:10:02 -0000	1.170
+++ sys/lwp.h	17 Nov 2015 22:55:38 -0000
@@ -252,6 +252,7 @@ extern int		maxlwp __read_mostly;	/* max
 #define	LP_SYSCTLWRITE	0x00000080 /* sysctl write lock held */
 #define	LP_MUSTJOIN	0x00000100 /* Must join kthread on exit */
 #define	LP_VFORKWAIT	0x00000200 /* Waiting at vfork() for a child */
+#define	LP_LOST_SYSENT	0x00000400 /* l_sysent has been overwritten */
 #define	LP_TIMEINTR	0x00010000 /* Time this soft interrupt */
 #define	LP_RUNNING	0x20000000 /* Active on a CPU */
 #define	LP_BOUND	0x80000000 /* Bound to a CPU */
Index: sys/syscallvar.h
===================================================================
RCS file: /cvsroot/src/sys/sys/syscallvar.h,v
retrieving revision 1.11
diff -u -p -r1.11 syscallvar.h
--- sys/syscallvar.h	24 Sep 2015 14:34:22 -0000	1.11
+++ sys/syscallvar.h	17 Nov 2015 22:55:38 -0000
@@ -61,6 +61,8 @@ sy_call(const struct sysent *sy, struct 
 {
 	int error;
 
+	if (l->l_sysent != NULL)
+		l->l_pflag |= LP_LOST_SYSENT;
 	l->l_sysent = sy;
 	error = (*sy->sy_call)(l, uap, rval);
 	l->l_sysent = NULL;