Re: tidy syscall_mi.h
>> Date: Thu, 7 Apr 2016 13:10:23 +0200 >> From: Michal Mazurek >> >> Depending on the NOLOCK flag, the KERNEL_LOCK will, or will not be acquired >> before calling pledge_syscall(). I therefore conclude that KERNEL_LOCK >> is not needed to call pledge_syscall(). > >It really depends on what the behaviour of pledge(2) is supposed to be >in multi-threaded processes. Since pledge(2) itself isn't a NOLOCK >system call, the current code guarantees that all other system calls >that aren't marked as NOLOCK, have a consitent view of the >pledge-related state, at least as long as they don't sleep. With your >change those system calls will enounter the same race as NOLOCK system >calls. And these system calls are more likely to suffer from >side-effects as they tend to be more complex. > >This is not necessarily a big issue. But we need to be careful about >the checks we do. As long as we keep the upfront checks simple, the >risks are low. But people pledging should be aware that there are >races in multi-threaded code and that they really should call >pledge(2) before creating threads. Yes. I wrote it the way it is, because I am terrified of getting it wrong. I am very unsure that the provided diff makes the right gaurantees of interlock between threads. Imagine one thread performing the pledge, while another thread is merrily advancing doing NOLOCK system calls (I cannot be certain that is wrong, but the weak semantics feel wrong).
Re: tidy syscall_mi.h
Michal Mazurek wrote:
> Depending on the NOLOCK flag, the KERNEL_LOCK will, or will not be acquired
> before calling pledge_syscall(). I therefore conclude that KERNEL_LOCK
> is not needed to call pledge_syscall().
>
> Also remove the goto. The code is simple enough to avoid it easily.
>
> I think this complexity was caused by some reshuffling during tame/pledge
> deployment.
I think some of the code in pledge_syscall was also more complex. Now it's
only a lookup into a static table, so I think this is ok.
>
> Index: sys/sys/syscall_mi.h
> ===
> RCS file: /cvs/src/sys/sys/syscall_mi.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 syscall_mi.h
> --- sys/sys/syscall_mi.h 3 Nov 2015 16:14:14 - 1.15
> +++ sys/sys/syscall_mi.h 7 Apr 2016 11:01:08 -
> @@ -69,29 +69,24 @@ mi_syscall(struct proc *p, register_t co
> }
> #endif
>
> - if (lock)
> - KERNEL_LOCK();
> pledged = (p->p_p->ps_flags & PS_PLEDGE);
> if (pledged && (error = pledge_syscall(p, code, &tval))) {
> - if (!lock)
> - KERNEL_LOCK();
> + KERNEL_LOCK();
> error = pledge_fail(p, error, tval);
> KERNEL_UNLOCK();
> return (error);
> }
> #if NSYSTRACE > 0
> if (ISSET(p->p_flag, P_SYSTRACE)) {
> - if (!lock)
> - KERNEL_LOCK();
> + KERNEL_LOCK();
> error = systrace_redirect(code, p, argp, retval);
> - lock = 1;
> - goto done;
> + KERNEL_UNLOCK();
> + return (error);
> }
> #endif
> + if (lock)
> + KERNEL_LOCK();
> error = (*callp->sy_call)(p, argp, retval);
> -#if NSYSTRACE > 0
> -done:
> -#endif
> if (lock)
> KERNEL_UNLOCK();
>
>
> --
> Michal Mazurek
>
Re: tidy syscall_mi.h
> Date: Thu, 7 Apr 2016 13:10:23 +0200 > From: Michal Mazurek > > Depending on the NOLOCK flag, the KERNEL_LOCK will, or will not be acquired > before calling pledge_syscall(). I therefore conclude that KERNEL_LOCK > is not needed to call pledge_syscall(). It really depends on what the behaviour of pledge(2) is supposed to be in multi-threaded processes. Since pledge(2) itself isn't a NOLOCK system call, the current code guarantees that all other system calls that aren't marked as NOLOCK, have a consitent view of the pledge-related state, at least as long as they don't sleep. With your change those system calls will enounter the same race as NOLOCK system calls. And these system calls are more likely to suffer from side-effects as they tend to be more complex. This is not necessarily a big issue. But we need to be careful about the checks we do. As long as we keep the upfront checks simple, the risks are low. But people pledging should be aware that there are races in multi-threaded code and that they really should call pledge(2) before creating threads.
tidy syscall_mi.h
Depending on the NOLOCK flag, the KERNEL_LOCK will, or will not be acquired
before calling pledge_syscall(). I therefore conclude that KERNEL_LOCK
is not needed to call pledge_syscall().
Also remove the goto. The code is simple enough to avoid it easily.
I think this complexity was caused by some reshuffling during tame/pledge
deployment.
Index: sys/sys/syscall_mi.h
===
RCS file: /cvs/src/sys/sys/syscall_mi.h,v
retrieving revision 1.15
diff -u -p -r1.15 syscall_mi.h
--- sys/sys/syscall_mi.h3 Nov 2015 16:14:14 - 1.15
+++ sys/sys/syscall_mi.h7 Apr 2016 11:01:08 -
@@ -69,29 +69,24 @@ mi_syscall(struct proc *p, register_t co
}
#endif
- if (lock)
- KERNEL_LOCK();
pledged = (p->p_p->ps_flags & PS_PLEDGE);
if (pledged && (error = pledge_syscall(p, code, &tval))) {
- if (!lock)
- KERNEL_LOCK();
+ KERNEL_LOCK();
error = pledge_fail(p, error, tval);
KERNEL_UNLOCK();
return (error);
}
#if NSYSTRACE > 0
if (ISSET(p->p_flag, P_SYSTRACE)) {
- if (!lock)
- KERNEL_LOCK();
+ KERNEL_LOCK();
error = systrace_redirect(code, p, argp, retval);
- lock = 1;
- goto done;
+ KERNEL_UNLOCK();
+ return (error);
}
#endif
+ if (lock)
+ KERNEL_LOCK();
error = (*callp->sy_call)(p, argp, retval);
-#if NSYSTRACE > 0
-done:
-#endif
if (lock)
KERNEL_UNLOCK();
--
Michal Mazurek
