Mark get*[ug]id() as NOLOCK
Recently guenther changed user credentials to be a per-process
resource, but kept a per-thread cache of credentials that get
refreshed at each system call entry. All of the get*[ug]id() system
calls simply access the thread cached credentials, and possibly
copyout() them if necessary, so they're safe to mark as NOLOCK.
ok?
Index: syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.141
diff -u -p -r1.141 syscalls.master
--- syscalls.master 6 Jul 2014 20:55:58 - 1.141
+++ syscalls.master 7 Jul 2014 18:06:34 -
@@ -80,8 +80,8 @@
int flags, void *data); }
22 STD { int sys_unmount(const char *path, int flags); }
23 STD { int sys_setuid(uid_t uid); }
-24 STD { uid_t sys_getuid(void); }
-25 STD { uid_t sys_geteuid(void); }
+24 STD NOLOCK { uid_t sys_getuid(void); }
+25 STD NOLOCK { uid_t sys_geteuid(void); }
#ifdef PTRACE
26 STD { int sys_ptrace(int req, pid_t pid, caddr_t addr, \
int data); }
@@ -112,7 +112,7 @@
41 STD { int sys_dup(int fd); }
42 STD { int sys_fstatat(int fd, const char *path, \
struct stat *buf, int flag); }
-43 STD { gid_t sys_getegid(void); }
+43 STD NOLOCK { gid_t sys_getegid(void); }
44 STD { int sys_profil(caddr_t samples, size_t size, \
u_long offset, u_int scale); }
#ifdef KTRACE
@@ -124,7 +124,7 @@
46 STD { int sys_sigaction(int signum, \
const struct sigaction *nsa, \
struct sigaction *osa); }
-47 STD { gid_t sys_getgid(void); }
+47 STD NOLOCK { gid_t sys_getgid(void); }
48 STD { int sys_sigprocmask(int how, sigset_t mask); }
49 STD { int sys_getlogin(char *namebuf, u_int namelen); }
50 STD { int sys_setlogin(const char *namebuf); }
@@ -181,7 +181,7 @@
const struct timeval *tptr); }
78 STD { int sys_mincore(void *addr, size_t len, \
char *vec); }
-79 STD { int sys_getgroups(int gidsetsize, \
+79 STD NOLOCK { int sys_getgroups(int gidsetsize, \
gid_t *gidset); }
80 STD { int sys_setgroups(int gidsetsize, \
const gid_t *gidset); }
@@ -476,11 +476,11 @@
278UNIMPL sys_extattr_set_fd
279UNIMPL sys_extattr_get_fd
280UNIMPL sys_extattr_delete_fd
-281STD { int sys_getresuid(uid_t *ruid, uid_t *euid, \
+281STD NOLOCK { int sys_getresuid(uid_t *ruid, uid_t *euid, \
uid_t *suid); }
282STD { int sys_setresuid(uid_t ruid, uid_t euid, \
uid_t suid); }
-283STD { int sys_getresgid(gid_t *rgid, gid_t *egid, \
+283STD NOLOCK { int sys_getresgid(gid_t *rgid, gid_t *egid, \
gid_t *sgid); }
284STD { int sys_setresgid(gid_t rgid, gid_t egid, \
gid_t sgid); }
Re: Mark get*[ug]id() as NOLOCK
Date: Mon, 7 Jul 2014 11:18:53 -0700
From: Matthew Dempsky matt...@dempsky.org
Recently guenther changed user credentials to be a per-process
resource, but kept a per-thread cache of credentials that get
refreshed at each system call entry. All of the get*[ug]id() system
calls simply access the thread cached credentials, and possibly
copyout() them if necessary, so they're safe to mark as NOLOCK.
ok?
Makes sense to me. But let's give guenther@ a chance to comment.
Index: syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.141
diff -u -p -r1.141 syscalls.master
--- syscalls.master 6 Jul 2014 20:55:58 - 1.141
+++ syscalls.master 7 Jul 2014 18:06:34 -
@@ -80,8 +80,8 @@
int flags, void *data); }
22 STD { int sys_unmount(const char *path, int flags); }
23 STD { int sys_setuid(uid_t uid); }
-24 STD { uid_t sys_getuid(void); }
-25 STD { uid_t sys_geteuid(void); }
+24 STD NOLOCK { uid_t sys_getuid(void); }
+25 STD NOLOCK { uid_t sys_geteuid(void); }
#ifdef PTRACE
26 STD { int sys_ptrace(int req, pid_t pid, caddr_t addr, \
int data); }
@@ -112,7 +112,7 @@
41 STD { int sys_dup(int fd); }
42 STD { int sys_fstatat(int fd, const char *path, \
struct stat *buf, int flag); }
-43 STD { gid_t sys_getegid(void); }
+43 STD NOLOCK { gid_t sys_getegid(void); }
44 STD { int sys_profil(caddr_t samples, size_t size, \
u_long offset, u_int scale); }
#ifdef KTRACE
@@ -124,7 +124,7 @@
46 STD { int sys_sigaction(int signum, \
const struct sigaction *nsa, \
struct sigaction *osa); }
-47 STD { gid_t sys_getgid(void); }
+47 STD NOLOCK { gid_t sys_getgid(void); }
48 STD { int sys_sigprocmask(int how, sigset_t mask); }
49 STD { int sys_getlogin(char *namebuf, u_int namelen); }
50 STD { int sys_setlogin(const char *namebuf); }
@@ -181,7 +181,7 @@
const struct timeval *tptr); }
78 STD { int sys_mincore(void *addr, size_t len, \
char *vec); }
-79 STD { int sys_getgroups(int gidsetsize, \
+79 STD NOLOCK { int sys_getgroups(int gidsetsize, \
gid_t *gidset); }
80 STD { int sys_setgroups(int gidsetsize, \
const gid_t *gidset); }
@@ -476,11 +476,11 @@
278 UNIMPL sys_extattr_set_fd
279 UNIMPL sys_extattr_get_fd
280 UNIMPL sys_extattr_delete_fd
-281 STD { int sys_getresuid(uid_t *ruid, uid_t *euid, \
+281 STD NOLOCK { int sys_getresuid(uid_t *ruid, uid_t *euid, \
uid_t *suid); }
282 STD { int sys_setresuid(uid_t ruid, uid_t euid, \
uid_t suid); }
-283 STD { int sys_getresgid(gid_t *rgid, gid_t *egid, \
+283 STD NOLOCK { int sys_getresgid(gid_t *rgid, gid_t *egid, \
gid_t *sgid); }
284 STD { int sys_setresgid(gid_t rgid, gid_t egid, \
gid_t sgid); }
Re: Mark get*[ug]id() as NOLOCK
On Mon, Jul 07, 2014 at 11:18, Matthew Dempsky wrote: Recently guenther changed user credentials to be a per-process resource, but kept a per-thread cache of credentials that get refreshed at each system call entry. All of the get*[ug]id() system calls simply access the thread cached credentials, and possibly copyout() them if necessary, so they're safe to mark as NOLOCK. Is copyout safe to call with no lock?
Re: Mark get*[ug]id() as NOLOCK
Date: Mon, 07 Jul 2014 15:06:47 -0400 From: Ted Unangst t...@tedunangst.com On Mon, Jul 07, 2014 at 11:18, Matthew Dempsky wrote: Recently guenther changed user credentials to be a per-process resource, but kept a per-thread cache of credentials that get refreshed at each system call entry. All of the get*[ug]id() system calls simply access the thread cached credentials, and possibly copyout() them if necessary, so they're safe to mark as NOLOCK. Is copyout safe to call with no lock? yes
Re: Mark get*[ug]id() as NOLOCK
On Mon, Jul 7, 2014 at 11:18 AM, Matthew Dempsky matt...@dempsky.org wrote: Recently guenther changed user credentials to be a per-process resource, but kept a per-thread cache of credentials that get refreshed at each system call entry. All of the get*[ug]id() system calls simply access the thread cached credentials, and possibly copyout() them if necessary, so they're safe to mark as NOLOCK. ok? Oh yeah, I knew there was another benefit to that work. Thanks! ok guenther@
Use atomics for mutating p_sigmask
p_sigmask is only modified by the owning thread from process context
(e.g., sys_sigprocmask(), sys_sigreturn(), userret(), or postsig()),
but it can be accessed anywhere (e.g., interrupts or threads on other
CPUs). Currently sys_sigprocmask() protects p_sigmask with splhigh(),
but that's not MP-safe.
The simpler solution is to take advantage of our atomics APIs.
Unfortunately for implementing SIG_SETMASK, we don't have an
atomic_store_int() function, and I can't bring myself to abuse
volatile for this purpose, so I've resorted to atomic_swap_uint().
While here, I also refactored the P_SIGSUSPEND code a bit so there's
less code duplication.
I've included just amd64 and i386's machdep.c for now, because those
were the only ones I'm readily able to test. The others should be
easy to similarly fix though, and I can send a followup diff for them.
ok?
Index: sys/proc.h
===
RCS file: /home/matthew/cvs-mirror/cvs/src/sys/sys/proc.h,v
retrieving revision 1.186
diff -u -p -r1.186 proc.h
--- sys/proc.h 15 May 2014 03:52:25 - 1.186
+++ sys/proc.h 7 Jul 2014 19:23:06 -
@@ -520,6 +520,8 @@ int proc_cansugid(struct proc *);
void proc_finish_wait(struct proc *, struct proc *);
void process_zap(struct process *);
void proc_free(struct proc *);
+void proc_sigsetmask(struct proc *, sigset_t);
+void proc_sigsuspend(struct proc *, sigset_t);
struct sleep_state {
int sls_s;
@@ -559,4 +561,3 @@ struct cpu_info *cpuset_first(struct cpu
#endif /* _KERNEL */
#endif /* !_SYS_PROC_H_ */
-
Index: kern/kern_sig.c
===
RCS file: /home/matthew/cvs-mirror/cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.166
diff -u -p -r1.166 kern_sig.c
--- kern/kern_sig.c 4 May 2014 05:03:26 - 1.166
+++ kern/kern_sig.c 7 Jul 2014 19:24:07 -
@@ -62,6 +62,7 @@
#include sys/ptrace.h
#include sys/sched.h
#include sys/user.h
+#include sys/atomic.h
#include sys/mount.h
#include sys/syscallargs.h
@@ -439,37 +440,55 @@ sys_sigprocmask(struct proc *p, void *v,
syscallarg(int) how;
syscallarg(sigset_t) mask;
} */ *uap = v;
- int error = 0;
- int s;
- sigset_t mask;
+ sigset_t mask = SCARG(uap, mask);
*retval = p-p_sigmask;
- mask = SCARG(uap, mask);
- s = splhigh();
switch (SCARG(uap, how)) {
case SIG_BLOCK:
- p-p_sigmask |= mask ~ sigcantmask;
+ atomic_setbits_int(p-p_sigmask, mask ~ sigcantmask);
break;
case SIG_UNBLOCK:
- p-p_sigmask = ~mask;
+ atomic_clearbits_int(p-p_sigmask, mask);
break;
case SIG_SETMASK:
- p-p_sigmask = mask ~ sigcantmask;
+ proc_sigsetmask(p, mask ~ sigcantmask);
break;
default:
- error = EINVAL;
- break;
+ return (EINVAL);
}
- splx(s);
- return (error);
+
+ return (0);
+}
+
+void
+proc_sigsetmask(struct proc *p, sigset_t mask)
+{
+ KASSERT(p == curproc);
+
+ /* XXX: An atomic store would suffice here. */
+ (void)atomic_swap_uint(p-p_sigmask, mask);
+}
+
+/*
+ * Temporarily replace calling proc's signal mask for the duration of a
+ * system call. Original signal mask will be restored by userret().
+ */
+void
+proc_sigsuspend(struct proc *p, sigset_t mask)
+{
+ KASSERT(p == curproc);
+ KASSERT(!(p-p_flag P_SIGSUSPEND));
+
+ p-p_oldmask = p-p_sigmask;
+ atomic_setbits_int(p-p_flag, P_SIGSUSPEND);
+ proc_sigsetmask(p, mask);
}
/* ARGSUSED */
int
sys_sigpending(struct proc *p, void *v, register_t *retval)
{
-
*retval = p-p_siglist;
return (0);
}
@@ -489,16 +508,7 @@ sys_sigsuspend(struct proc *p, void *v,
struct process *pr = p-p_p;
struct sigacts *ps = pr-ps_sigacts;
- /*
-* When returning from sigpause, we want
-* the old mask to be restored after the
-* signal handler has finished. Thus, we
-* save it here and mark the sigacts structure
-* to indicate this.
-*/
- p-p_oldmask = p-p_sigmask;
- atomic_setbits_int(p-p_flag, P_SIGSUSPEND);
- p-p_sigmask = SCARG(uap, mask) ~ sigcantmask;
+ proc_sigsuspend(p, SCARG(uap, mask) ~ sigcantmask);
while (tsleep(ps, PPAUSE|PCATCH, pause, 0) == 0)
/* void */;
/* always return EINTR rather than ERESTART... */
Index: kern/sys_generic.c
===
RCS file: /home/matthew/cvs-mirror/cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.86
diff -u -p -r1.86 sys_generic.c
--- kern/sys_generic.c 30 Mar 2014 21:54:48 - 1.86
+++ kern/sys_generic.c 7 Jul 2014 19:24:07 -
@@ -658,9 +620,7 @@ dopselect(struct proc
