Date:	Thu, 3 Mar 2016 14:52:10 -0500
From:	Chris Metcalf <cmetcalf@...lanox.com>
To:	Andy Lutomirski <luto@...capital.net>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Christoph Lameter <cl@...ux.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Ingo Molnar <mingo@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Tejun Heo <tj@...nel.org>,
	Gilad Ben Yossef <giladb@...hip.com>,
	Will Deacon <will.deacon@....com>,
	Rik van Riel <riel@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	X86 ML <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality

On 03/02/2016 07:36 PM, Andy Lutomirski wrote:
> On Mar 2, 2016 12:10 PM, "Chris Metcalf" <cmetcalf@...hip.com> wrote:
>> In prepare_exit_to_usermode(), call task_isolation_ready()
>> when we are checking the thread-info flags, and after we've handled
>> the other work, call task_isolation_enter() unconditionally.
>>
>> In syscall_trace_enter_phase1(), we add the necessary support for
>> strict-mode detection of syscalls.
>>
>> We add strict reporting for the kernel exception types that do
>> not result in signals, namely non-signalling page faults and
>> non-signalling MPX fixups.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@...hip.com>
>> ---
>>   arch/x86/entry/common.c | 18 ++++++++++++++++--
>>   arch/x86/kernel/traps.c |  2 ++
>>   arch/x86/mm/fault.c     |  2 ++
>>   3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index 03663740c866..27c71165416b 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/context_tracking.h>
>>   #include <linux/user-return-notifier.h>
>>   #include <linux/uprobes.h>
>> +#include <linux/isolation.h>
>>
>>   #include <asm/desc.h>
>>   #include <asm/traps.h>
>> @@ -91,6 +92,10 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
>>           */
>>          if (work & _TIF_NOHZ) {
>>                  enter_from_user_mode();
>> +               if (task_isolation_check_syscall(regs->orig_ax)) {
>> +                       regs->orig_ax = -1;
>> +                       return 0;
>> +               }
> This needs a comment indicating the intended semantics.
> And I've still heard no explanation of why this part can't use seccomp.

Here's an excerpt from my earlier reply to you from:

   https://lkml.kernel.org/r/55AE9EAC.4010202@ezchip.com

Admittedly this patch series has been moving very slowly through
review, so it's not surprising we have to revisit some things!

On 07/21/2015 03:34 PM, Chris Metcalf wrote:
> On 07/13/2015 05:47 PM, Andy Lutomirski wrote:
>> If a user wants a syscall to kill them, use
>> seccomp.  The kernel isn't at fault if the user does a syscall when it
>> didn't want to enter the kernel.
>
> Interesting!  I didn't realize how close SECCOMP_SET_MODE_STRICT
> was to what I wanted here.  One concern is that there doesn't seem
> to be a way to "escape" from seccomp strict mode, i.e. you can't
> call seccomp() again to turn it off - which makes sense for seccomp
> since it's a security issue, but not so much sense with cpu_isolated.
>
> So, do you think there's a good role for the seccomp() API to play
> in achieving this goal?  It's certainly not a question of "the kernel at
> fault" but rather "asking the kernel to help catch user mistakes"
> (typically third-party libraries in our customers' experience).  You
> could imagine a SECCOMP_SET_MODE_ISOLATED or something.
>
> Alternatively, we could stick with the API proposed in my patch
> series, or something similar, and just try to piggy-back on the seccomp
> internals to make it happen.  It would require Kconfig to ensure
> that SECCOMP was enabled though, which obviously isn't currently
> required to do cpu isolation.

On looking at this again just now, one thing that strikes me is that
it may not be necessary to forbid the syscall like seccomp does.
It may be sufficient just to trigger the task isolation strict signal
and then allow the syscall to complete.  After all, we don't "fail"
any of the other things that upset strict mode, like page faults; we
let them complete, but add a signal.  So for consistency, I think it
may in fact make sense to simply trigger the signal but let the
syscall do its thing.  After all, perhaps the signal is handled
and logged and we don't mind having the application continue; the
signal handler can certainly choose to fail hard, or in the usual
case of no signal handler, that kills the task just fine too.
Allowing the syscall to complete is really kind of incidental.

After the changes proposed lower down in this email, this call
site will end up looking like:

	/* Generate a task isolation strict signal if necessary. */
	if ((work & _TIF_TASK_ISOLATION) && task_isolation_strict())
		task_isolation_syscall(regs->orig_ax);

But do let me know if you think more specifically that there is
a way seccomp can be helpful.

>>                  work &= ~_TIF_NOHZ;
>>          }
>>   #endif
>> @@ -254,17 +259,26 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
>>                  if (cached_flags & _TIF_USER_RETURN_NOTIFY)
>>                          fire_user_return_notifiers();
>>
>> +               task_isolation_enter();
>> +
>>                  /* Disable IRQs and retry */
>>                  local_irq_disable();
>>
>>                  cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);
>>
>> -               if (!(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
>> +               if (!(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS) &&
>> +                   task_isolation_ready())
>>                          break;
>>
>>          }
>>   }
>>
>> +#ifdef CONFIG_TASK_ISOLATION
>> +# define EXIT_TO_USERMODE_FLAGS (EXIT_TO_USERMODE_LOOP_FLAGS | _TIF_NOHZ)
>> +#else
>> +# define EXIT_TO_USERMODE_FLAGS EXIT_TO_USERMODE_LOOP_FLAGS
>> +#endif
>> +
> TIF_NOHZ is already a hack and IMO this just makes it worse.  At the
> very least this should have a comment.  It really ought to be either a
> static_branch or a flag that's actually switched per cpu.
> But this is also a confusing change.  Don't override the definition
> here -- stick it in the header where it belongs.

The EXIT_TO_USERMODE_xxx stuff is only defined in common.c, not in a header.

The more I look at this, though, the more I think it would be cleanest
to add a new flag _TIF_TASK_ISOLATION that is set just for tasks that have
enabled task isolation.  That can be used unconditionally to check to see
if we need to call exit_to_usermode_loop() from prepare_exit_to_usermode(),
and also means that non-task-isolation tasks don't need to go into the
exit loop unconditionally, which is obviously a performance drag for them.

So I will move the EXIT_TO_USERMODE_FLAGS definition to immediately follow
the EXIT_TO_USERMODE_LOOP_FLAGS definition and write it this way:

/*
  * Task-isolated tasks have to enter exit_to_usermode_loop unconditionally,
  * but the state isn't cleared in the loop.
  */
#define EXIT_TO_USERMODE_FLAGS \
	(EXIT_TO_USERMODE_LOOP_FLAGS | _TIF_TASK_ISOLATION)

Thanks for the feedback!

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

Powered by blists - more mailing lists