Re: [patch] let CONFIG_SECCOMP default to n

Re: [patch] let CONFIG_SECCOMP default to n

[Chuck Ebbert]

In-Reply-To: <p73wtain80h.fsf@verdi.suse.de>

On 12 Jul 2006 17:43:42 +0200, Andi Kleen wrote:

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> > 
> > I really don't care about cpushare and patents for some users of the
> > code in question. On the other hand turning on performance harming code
> > for a tiny number of users is dumb. If it were a loadable module it
> > would be different.
> 
> Actually there are some promising applications of seccomp outside
> cpushare.
> 
> e.g. Andrea at some point proposed to run codecs which often
> have security issues in a simple cpusec jail.  That's ok for 
> them because they normally don't need to do any system calls.
> 
> I liked the idea. While this can be done with LSM (e.g. apparmor) too 
> seccomp is definitely much easier and simpler and more "obviously safe"
> than anything LSM based.
> 
> If the TSC disabling code is taken out the runtime overhead
> of seccomp is also very small because it's only tested in slow
> paths.

We can just fold the TSC disable stuff into the new thread_flags test
at context-switch time:

[compile tested only; requires just-sent fix to i386 system.h]

 arch/i386/kernel/process.c     |   61 ++++++++++++++---------------------------
 include/asm-i386/thread_info.h |    3 +-
 2 files changed, 24 insertions(+), 40 deletions(-)

--- 2.6.18-rc1-nb.orig/arch/i386/kernel/process.c
+++ 2.6.18-rc1-nb/arch/i386/kernel/process.c
@@ -535,12 +535,11 @@ int dump_task_regs(struct task_struct *t
 	return 1;
 }
 
-static noinline void __switch_to_xtra(struct task_struct *next_p,
-				    struct tss_struct *tss)
+static noinline void
+__switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
+		 struct tss_struct *tss)
 {
-	struct thread_struct *next;
-
-	next = &next_p->thread;
+	struct thread_struct *next = &next_p->thread;
 
 	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
 		set_debugreg(next->debugreg[0], 0);
@@ -552,6 +551,19 @@ static noinline void __switch_to_xtra(st
 		set_debugreg(next->debugreg[7], 7);
 	}
 
+	/*
+	 * Context switch may need to tweak the TSC disable bit in CR4.
+	 * The optimizer should remove this code when !CONFIG_SECCOMP.
+	 */
+	if (has_secure_computing(task_thread_info(prev_p)) ^
+	    has_secure_computing(task_thread_info(next_p))) {
+		/* prev and next are different */
+		if (has_secure_computing(task_thread_info(next_p)))
+			write_cr4(read_cr4() | X86_CR4_TSD);
+		else
+			write_cr4(read_cr4() & ~X86_CR4_TSD);
+	}
+
 	if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
 		/*
 		 * Disable the bitmap via an invalid offset. We still cache
@@ -561,7 +573,7 @@ static noinline void __switch_to_xtra(st
 		return;
 	}
 
-	if (likely(next == tss->io_bitmap_owner)) {
+	if (likely(tss->io_bitmap_owner == next)) {
 		/*
 		 * Previous owner of the bitmap (hence the bitmap content)
 		 * matches the next task, we dont have to do anything but
@@ -583,33 +595,6 @@ static noinline void __switch_to_xtra(st
 }
 
 /*
- * This function selects if the context switch from prev to next
- * has to tweak the TSC disable bit in the cr4.
- */
-static inline void disable_tsc(struct task_struct *prev_p,
-			       struct task_struct *next_p)
-{
-	struct thread_info *prev, *next;
-
-	/*
-	 * gcc should eliminate the ->thread_info dereference if
-	 * has_secure_computing returns 0 at compile time (SECCOMP=n).
-	 */
-	prev = task_thread_info(prev_p);
-	next = task_thread_info(next_p);
-
-	if (has_secure_computing(prev) || has_secure_computing(next)) {
-		/* slow path here */
-		if (has_secure_computing(prev) &&
-		    !has_secure_computing(next)) {
-			write_cr4(read_cr4() & ~X86_CR4_TSD);
-		} else if (!has_secure_computing(prev) &&
-			   has_secure_computing(next))
-			write_cr4(read_cr4() | X86_CR4_TSD);
-	}
-}
-
-/*
  *	switch_to(x,yn) should switch tasks from x to y.
  *
  * We fsave/fwait so that an exception goes off at the right time
@@ -688,13 +673,11 @@ struct task_struct fastcall * __switch_t
 		set_iopl_mask(next->iopl);
 
 	/*
-	 * Now maybe handle debug registers and/or IO bitmaps
+	 * Now maybe handle debug registers, IO bitmaps, TSC disable
 	 */
-	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
-	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
-		__switch_to_xtra(next_p, tss);
-
-	disable_tsc(prev_p, next_p);
+	if (unlikely(task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV ||
+		     task_thread_info(next_p)->flags & _TIF_WORK_CTXSW_NEXT))
+		__switch_to_xtra(prev_p, next_p, tss);
 
 	return prev_p;
 }
--- 2.6.18-rc1-nb.orig/include/asm-i386/thread_info.h
+++ 2.6.18-rc1-nb/include/asm-i386/thread_info.h
@@ -164,7 +164,8 @@ static inline struct thread_info *curren
 #define _TIF_ALLWORK_MASK	(0x0000FFFF & ~_TIF_SECCOMP)
 
 /* flags to check in __switch_to() */
-#define _TIF_WORK_CTXSW (_TIF_DEBUG|_TIF_IO_BITMAP)
+#define _TIF_WORK_CTXSW_NEXT (_TIF_IO_BITMAP | _TIF_SECCOMP | _TIF_DEBUG)
+#define _TIF_WORK_CTXSW_PREV (_TIF_IO_BITMAP | _TIF_SECCOMP)
 
 /*
  * Thread-synchronous status.
-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: [patch] let CONFIG_SECCOMP default to n

[Linus Torvalds]

On Wed, 12 Jul 2006, Chuck Ebbert wrote:
>
> We can just fold the TSC disable stuff into the new thread_flags test
> at context-switch time:

I really think that this should be cleaned up to _not_ confuse the issue 
of TSC with any "secure computing" issue.

The two have nothing to do with each other from a technical standpoint. 

Just make the flag be called "TIF_NOTSC", and then any random usage 
(whether it be seccomp or anything else) can just set that flag, the same 
way ioperm() sets TIF_IO_BITMAP.

Much cleaner. 

There's no point in mixing up an implementation detail like SECCOMP with a 
feature like this.

			Linus

Re: [patch] let CONFIG_SECCOMP default to n

[andrea]

> On Wed, 12 Jul 2006, Chuck Ebbert wrote:
> >
> > We can just fold the TSC disable stuff into the new thread_flags test
> > at context-switch time:

Great idea Chunk! We already use them in the syscall, it sounds a
perfect fit ;).

On Wed, Jul 12, 2006 at 02:55:38PM -0700, Linus Torvalds wrote:
> 
> I really think that this should be cleaned up to _not_ confuse the issue 
> of TSC with any "secure computing" issue.
> 
> The two have nothing to do with each other from a technical standpoint. 
> 
> Just make the flag be called "TIF_NOTSC", and then any random usage 
> (whether it be seccomp or anything else) can just set that flag, the same 
> way ioperm() sets TIF_IO_BITMAP.
> 
> Much cleaner. 
> 
> There's no point in mixing up an implementation detail like SECCOMP with a 
> feature like this.

The only single advantage I can see in remaining purely in function of
seccomp instead of going in function of _TIF_NOTSC, is that the below
block would be completely optimized away at compile time when
CONFIG_SECCOMP is set to N. This now become a slow-path, but then I'm
unsure if the anti-seccomp advocates can live with this block in the
slow path given that seccomp will be the only user of the feature. I
suspect they won't like it but then I could be wrong.

I like it either ways.

	/*
	 * Context switch may need to tweak the TSC disable bit in CR4.
	 * The optimizer should remove this code when !CONFIG_SECCOMP.
	 */
	if (has_secure_computing(task_thread_info(prev_p)) ^
	    has_secure_computing(task_thread_info(next_p))) {
		/* prev and next are different */
		if (has_secure_computing(task_thread_info(next_p)))
			write_cr4(read_cr4() | X86_CR4_TSD);
		else
			write_cr4(read_cr4() & ~X86_CR4_TSD);
	}

Re: [patch] let CONFIG_SECCOMP default to n

[Andi Kleen]

> We can just fold the TSC disable stuff into the new thread_flags test
> at context-switch time:

I don't think it will work because you need to check state of 
both previous and next task. The thread_flags test only checks the
state of the next task.

-Andi

Re: [patch] let CONFIG_SECCOMP default to n II

[Andi Kleen]

On Wednesday 12 July 2006 23:57, Andi Kleen wrote:
> 
> > We can just fold the TSC disable stuff into the new thread_flags test
> > at context-switch time:
> 
> I don't think it will work because you need to check state of 
> both previous and next task. The thread_flags test only checks the
> state of the next task.

Actually scratch that objection. It should work just fine and would
indeed turn it into zero overhead for the common case.

Still I don't think it's actually needed so there might be still
a good case to remove it.

-Andi