linux-kernel - Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function

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: <87r3hl8qxp.fsf@ashishki-desk.ger.corp.intel.com>
Date:	Wed, 13 Jan 2016 17:00:50 +0200
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	Peter Zijlstra <peterz@...radead.org>, mingo@...nel.org,
	eranian@...gle.com
Cc:	linux-kernel@...r.kernel.org, vince@...ter.net, dvyukov@...gle.com,
	andi@...stfloor.org, jolsa@...hat.com, peterz@...radead.org
Subject: Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

I think I caught one, below.

Peter Zijlstra <peterz@...radead.org> writes:

> +static int event_function(void *info)
> +{
> +	struct event_function_struct *efs = info;
> +	struct perf_event *event = efs->event;
> +	struct perf_event_context *ctx = event->ctx;
> +	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> +	struct perf_event_context *task_ctx = cpuctx->task_ctx;
> +
> +	WARN_ON_ONCE(!irqs_disabled());
> +
> +	/*
> +	 * Since we do the IPI call without holding ctx->lock things can have
> +	 * changed, double check we hit the task we set out to hit.
> +	 *
> +	 * If ctx->task == current, we know things must remain valid because
> +	 * we have IRQs disabled so we cannot schedule.
> +	 */
> +	if (ctx->task) {
> +		if (ctx->task != current)
> +			return -EAGAIN;
> +
> +		WARN_ON_ONCE(task_ctx != ctx);

Looks like between dropping ctx::lock in event_function_call() and here,
cpuctx::task_ctx may still become NULL.

> +	} else {
> +		WARN_ON_ONCE(&cpuctx->ctx != ctx);
> +	}
> +
> +	perf_ctx_lock(cpuctx, task_ctx);
> +	/*
> +	 * Now that we hold locks, double check state. Paranoia pays.
> +	 */
> +	if (task_ctx) {
> +		WARN_ON_ONCE(task_ctx->task != current);
> +		/*
> +		 * We only use event_function_call() on established contexts,
> +		 * and event_function() is only ever called when active (or
> +		 * rather, we'll have bailed in task_function_call() or the
> +		 * above ctx->task != current test), therefore we must have
> +		 * ctx->is_active here.
> +		 */
> +		WARN_ON_ONCE(!ctx->is_active);
> +		/*
> +		 * And since we have ctx->is_active, cpuctx->task_ctx must
> +		 * match.
> +		 */
> +		WARN_ON_ONCE(cpuctx->task_ctx != task_ctx);
> +	}
> +	efs->func(event, cpuctx, ctx, efs->data);

In which case we probably don't want to call the callback.

Not sure if this is what Dmitry ran into, his logs contain warnings from
this function, but hard to tell exactly which ones.

Regards,
--
Alex