linux-kernel - Re: syscall_regfunc() && TIF_SYSCALL

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]

Date:	Fri, 30 Mar 2012 15:02:11 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...hat.com>, Jason Baron <jbaron@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: syscall_regfunc() && TIF_SYSCALL_TRACEPOINT

On Fri, 2012-03-30 at 20:31 +0200, Oleg Nesterov wrote:
> Hello.
> 
> I've looked at syscall_regfunc/unregfunc by accident, and I am
> a bit confused...
> 
> 	void syscall_regfunc(void)
> 	{
> 		unsigned long flags;
> 		struct task_struct *g, *t;
> 
> 		if (!sys_tracepoint_refcount) {
> 			read_lock_irqsave(&tasklist_lock, flags);
> 
> Why _irqsave? write_lock(tasklist) needs to disable irqs, but read_
> doesn't. Any subtle reason I missed?

As long as an interrupt doesn't take the tasklist lock as write, we
don't. If it doesn't then we should be safe not to disable interrupts
here.

> 
> 			do_each_thread(g, t) {
> 				/* Skip kernel threads. */
> 				if (t->mm)
> 
> We should check PF_KTHREAD, not ->mm.

A lot of places test ->mm for kernel threads. Just search for ->mm in
kernel/sched/core.c


> 
> 					set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
> 
> But the main question is, can't we race with clone() and miss the
> new child? The new task is not "visible" to do_each_thread() until
> copy_process()->list_add_tail_rcu(thread_group/init_task.tasks).
> 
> Don't we need something like the patch below?
> 
> Oleg.
> 
> 
> --- x/kernel/fork.c
> +++ x/kernel/fork.c
> @@ -1446,7 +1446,12 @@ static struct task_struct *copy_process(
>  
>  	total_forks++;
>  	spin_unlock(&current->sighand->siglock);
> +#ifdef CONFIG_TRACEPOINTS
> +	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> +		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);

I'm not so worried about the set (although that should be done) but it
is entirely possible that we need a clear too. Leaving this flag set
would cause a task to take the overhead of tracing syscalls without ever
tracing them.

-- Steve


> +#endif
>  	write_unlock_irq(&tasklist_lock);
> +
>  	proc_fork_connector(p);
>  	cgroup_post_fork(p);
>  	if (clone_flags & CLONE_THREAD)


--
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/