linux-kernel - Re: [RESEND PATCH V3] perf/x86: Consider pinned events for group validation

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: <20200117091341.GX2827@hirez.programming.kicks-ass.net>
Date:   Fri, 17 Jan 2020 10:13:41 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     kan.liang@...ux.intel.com
Cc:     mingo@...hat.com, acme@...nel.org, linux-kernel@...r.kernel.org,
        eranian@...gle.com, ak@...ux.intel.com
Subject: Re: [RESEND PATCH V3] perf/x86: Consider pinned events for group
 validation

On Thu, Jan 16, 2020 at 11:00:25AM -0800, kan.liang@...ux.intel.com wrote:
> @@ -2054,9 +2057,38 @@ static int validate_group(struct perf_event *event)
>  	if (n < 0)
>  		goto out;
>  
> +	/*
> +	 * Disable interrupts and preemption to prevent the events in this
> +	 * CPU's cpuc going away and getting freed.
> +	 */
> +	local_irq_save(flags);
> +
> +	/*
> +	 * The new group must can be scheduled together with current pinned
> +	 * events. Otherwise, it will never get a chance to be scheduled later.
> +	 *
> +	 * It won't catch all possible cases that cannot schedule, such as
> +	 * events pinned on CPU1, but the validation for a new CPU1 event
> +	 * running on other CPU. However, it's good enough to handle common
> +	 * cases like the global NMI watchdog.
> +	 */
> +	for (i = 0; i < cpuc->n_events; i++) {
> +		pinned_event = cpuc->event_list[i];
> +		if (WARN_ON_ONCE(!pinned_event))
> +			continue;
> +		if (!pinned_event->attr.pinned)
> +			continue;
> +		fake_cpuc->n_events = n;
> +		n = collect_events(fake_cpuc, pinned_event, false);
> +		if (n < 0)
> +			goto irq;
> +	}
> +

So I still completely hate this, because it makes the counter scheduling
more eratic.

It changes a situation where we only have false-positives (we allow
scheduling a group that might not ever get to run) into a situation
where we can have both false-positives and false-negatives.

Imagine the pinned event is for a currently running task; and that task
only runs sporadically. Then you can sometimes not create the group, but
mostly it'll work.

Yes, this is all very annoying, but I really don't see how this makes
anything any better.