linux-kernel - Re: [RFC PATCH] percpu system call: fast userspace percpu critical sections

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: <20150521183240.GW3644@twins.programming.kicks-ass.net>
Date:	Thu, 21 May 2015 20:32:40 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Paul Turner <pjt@...gle.com>, Andrew Hunter <ahh@...gle.com>,
	Ben Maurer <bmaurer@...com>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Josh Triplett <josh@...htriplett.org>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH] percpu system call: fast userspace percpu critical
 sections

On Thu, May 21, 2015 at 10:44:47AM -0400, Mathieu Desnoyers wrote:

> +struct thread_percpu_user {
> +	int32_t nesting;
> +	int32_t signal_sent;
> +	int32_t signo;
> +	int32_t current_cpu;
> +};

I would require this thing be naturally aligned, such that it does not
cross cacheline boundaries.

> +
> +static void percpu_user_sched_in(struct preempt_notifier *notifier, int cpu)
> +{
> +	struct thread_percpu_user __user *tpu_user;
> +	struct thread_percpu_user tpu;
> +	struct task_struct *t = current;
> +
> +	tpu_user = t->percpu_user;
> +	if (tpu_user == NULL)
> +		return;
> +	if (unlikely(t->flags & PF_EXITING))
> +		return;
> +	/*
> +	 * access_ok() of tpu_user has already been checked by sys_percpu().
> +	 */
> +	if (__put_user(smp_processor_id(), &tpu_user->current_cpu)) {
> +		WARN_ON_ONCE(1);
> +		return;
> +	}

This seems a waste; you already read the number unconditionally, might
as well double check and avoid the store.

> +	if (__copy_from_user(&tpu, tpu_user, sizeof(tpu))) {
> +		WARN_ON_ONCE(1);
> +		return;
> +	}

	if (tpu.current_cpu != smp_processor_id())
		__put_user();



> +	if (!tpu.nesting || tpu.signal_sent)
> +		return;
> +	if (do_send_sig_info(tpu.signo, SEND_SIG_PRIV, t, 0)) {
> +		WARN_ON_ONCE(1);
> +		return;
> +	}
> +	tpu.signal_sent = 1;
> +	if (__copy_to_user(tpu_user, &tpu, sizeof(tpu))) {
> +		WARN_ON_ONCE(1);
> +		return;
> +	}
> +}

Please do not use preempt notifiers for this.

Second, this all is done with preemption disabled, this means that all
that user access can fail.

You print useless WARNs and misbehave. If you detect a desire to fault,
you could delay until return to userspace and try again there. But it
all adds complexity.

The big advantage pjt's scheme had is that we have the instruction
pointer, we do not need to go read userspace memory that might not be
there. And it being limited  to a single range, while inconvenient,
simplifies the entire kernel side to:

	if ((unsigned long)(ip - offset) < size)
		do_magic();

Which is still simpler than the above.
--
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/