linux-kernel - Re: TIF_NOHZ can escape nonhz mask? (Was: [PATCH v3 6/8] x86: Split syscall_trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140729175414.GA3289@redhat.com>
Date:	Tue, 29 Jul 2014 19:54:14 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
	Will Drewry <wad@...omium.org>, x86@...nel.org,
	linux-arm-kernel@...ts.infradead.org, linux-mips@...ux-mips.org,
	linux-arch@...r.kernel.org, linux-security-module@...r.kernel.org,
	Alexei Starovoitov <ast@...mgrid.com>, hpa@...or.com
Subject: Re: TIF_NOHZ can escape nonhz mask? (Was: [PATCH v3 6/8] x86:
	Split syscall_trace_enter into two phases)

On 07/28, Frederic Weisbecker wrote:
>
> On Mon, Jul 28, 2014 at 08:58:03PM +0200, Oleg Nesterov wrote:
> >
> > Frederic, don't we need the patch below? In fact clear_() can be moved
> > under "if ()" too. and probably copy_process() should clear this flag...
> >
> > Or. __context_tracking_task_switch() can simply do
> >
> > 	 if (context_tracking_cpu_is_enabled())
> > 	 	set_tsk_thread_flag(next, TIF_NOHZ);
> > 	 else
> > 	 	clear_tsk_thread_flag(next, TIF_NOHZ);
> >
> > and then we can forget about copy_process(). Or I am totally confused?
> >
> >
> > I am also wondering if we can extend user_return_notifier to handle
> > enter/exit and kill TIF_NOHZ.
> >
> > Oleg.
> >
> > --- x/kernel/context_tracking.c
> > +++ x/kernel/context_tracking.c
> > @@ -202,7 +202,8 @@ void __context_tracking_task_switch(stru
> >  				    struct task_struct *next)
> >  {
> >  	clear_tsk_thread_flag(prev, TIF_NOHZ);
> > -	set_tsk_thread_flag(next, TIF_NOHZ);
> > +	if (context_tracking_cpu_is_enabled())
> > +		set_tsk_thread_flag(next, TIF_NOHZ);
> >  }
> >
> >  #ifdef CONFIG_CONTEXT_TRACKING_FORCE
>
> Unfortunately, as long as tasks can migrate in and out a context tracked CPU, we
> need to track all CPUs.

Thanks Frederic for your explanations. Yes, I was confused. But cough, now I am
even more confused.

I didn't even try to read this code, perhaps I'll try later, but let me ask
another question while you are here ;)

The comment above __context_tracking_task_switch() says:

	 * The context tracking uses the syscall slow path to implement its user-kernel
	 * boundaries probes on syscalls. This way it doesn't impact the syscall fast
	 * path on CPUs that don't do context tracking.
	        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

How? Every running task should have TIF_NOHZ set if context_tracking_is_enabled() ?

	 * But we need to clear the flag on the previous task because it may later
	 * migrate to some CPU that doesn't do the context tracking. As such the TIF
	 * flag may not be desired there.

For what? How this can help? This flag will be set again when we switch to this
task again?

Looks like, we can kill context_tracking_task_switch() and simply change the
"__init" callers of context_tracking_cpu_set() to do set_thread_flag(TIF_NOHZ) ?
Then this flag will be propagated by copy_process().

Or I am totally confused? (quite possible).

> So here is a scenario where this is a problem: a task runs on CPU 0, passes the context
> tracking call before returning from a syscall to userspace, and gets an interrupt. The
> interrupt preempts the task and it moves to CPU 1. So it returns from preempt_schedule_irq()
> after which it is going to resume to userspace.
>
> In this scenario, if context tracking is only enabled on CPU 1, we have no way to know that
> the task is resuming to userspace, because we passed through the context tracking probe
> already and it was ignored on CPU 0.

Thanks. But I still can't understand... So if we only track CPU 1, then in this
case context_tracking.state == IN_USER on CPU 0, but it can be IN_USER or IN_KERNEL
on CPU 1.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/