From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755862AbcA1Vx1 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 28 Jan 2016 16:53:27 -0500
Received: from www.linutronix.de ([62.245.132.108]:37459 "EHLO
	Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1161294AbcA1VxZ (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 28 Jan 2016 16:53:25 -0500
Date: Thu, 28 Jan 2016 22:52:13 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
cc: Paul Turner <pjt@google.com>, Andrew Hunter <ahh@google.com>,
        Peter Zijlstra <peterz@infradead.org>, linux-kernel@vger.kernel.org,
        linux-api@vger.kernel.org, Andy Lutomirski <luto@amacapital.net>,
        Andi Kleen <andi@firstfloor.org>, Dave Watson <davejwatson@fb.com>,
        Chris Lameter <cl@linux.com>, Ingo Molnar <mingo@redhat.com>,
        Ben Maurer <bmaurer@fb.com>, Steven Rostedt <rostedt@goodmis.org>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Russell King <linux@arm.linux.org.uk>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [RFC PATCH v3 1/5] getcpu_cache system call: cache CPU number
 of running thread
In-Reply-To: <1454014885-29012-2-git-send-email-mathieu.desnoyers@efficios.com>
Message-ID: <alpine.DEB.2.11.1601282205290.3886@nanos>
References: <1454014885-29012-1-git-send-email-mathieu.desnoyers@efficios.com> <1454014885-29012-2-git-send-email-mathieu.desnoyers@efficios.com>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
X-Linutronix-Spam-Score: -1.0
X-Linutronix-Spam-Level: -
X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required,  ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 28 Jan 2016, Mathieu Desnoyers wrote:

> +static int __get_cpu_cache_ptr(int32_t __user **cpu_cache,

cpu_cache is __user ????

> +		int32_t __user * __user *cpu_cachep)
> +{
> +#ifdef CONFIG_COMPAT
> +	if (is_compat_task()) {
> +		compat_uptr_t *compat_cachep = (compat_uptr_t *) cpu_cachep;
> +		compat_uptr_t compat_cache;
> +
> +		if (get_user(compat_cache, compat_cachep))
> +			return -EFAULT;
> +		*cpu_cache = compat_ptr(compat_cache);

sparse should have told you that :)

> +		return 0;
> +	}
> +#endif
> +	return get_user(*cpu_cache, cpu_cachep);
> +}
> +
> +#define get_cpu_cache_ptr(cpu_cache, cpu_cachep)	\
> +	__get_cpu_cache_ptr(&(cpu_cache), cpu_cachep)
> +
> +static int put_cpu_cache_ptr(int32_t __user *cpu_cache,

Ditto

> +		int32_t __user * __user *cpu_cachep)
> +{
> +#ifdef CONFIG_COMPAT
> +	if (is_compat_task()) {
> +		compat_uptr_t compat_cache = ptr_to_compat(cpu_cache);
> +		compat_uptr_t *compat_cachep = (compat_uptr_t *) cpu_cachep;
> +
> +		return put_user(compat_cache, compat_cachep);
> +	}
> +#endif
> +	return put_user(cpu_cache, cpu_cachep);
> +}
> +		current->cpu_cache = cpu_cache;
> +		/*
> +		 * Migration checks the getcpu cache to see whether the
> +		 * notify_resume flag should be set.
> +		 * Therefore, we need to ensure that the scheduler sees
> +		 * the getcpu cache pointer update before we update the getcpu
> +		 * cache content with the current CPU number.
> +		 */
> +		barrier();

And how does that barrier ensure this? Not at all. And why would the scheduler
care? All the scheduler cares about is tsk->cpu_cache.

> +		/*
> +		 * Do an initial cpu cache update to ensure we won't hit
> +		 * SIGSEGV if put_user() fails in the resume notifier.
> +		 */

If you get migrated before that call, then you SIGSEGV nevertheless.

You need that call here for the case you are NOT migrated before returning to
user space because otherwise the variable is not updated.

If you want to verify that user address without a potential SIGSEGV, then you
need to do this before setting current->cpu_cache. You still need the update
after setting current->cpu_cache.

> +		if (getcpu_cache_update(cpu_cache)) {
> +			current->cpu_cache = NULL;
> +			return -EFAULT;
> +		}
> +		return 0;

Thanks,

	tglx