linux-kernel - Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events

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: <20160608081141.GF2527@techsingularity.net>
Date:	Wed, 8 Jun 2016 09:11:41 +0100
From:	Mel Gorman <mgorman@...hsingularity.net>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Subject: Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events

On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote:
> When enabling sched trace events via:
> 
>   echo 1 > /sys/kernel/debug/tracing/events/sched/enable
> 
> I see a hang, with the following BUG in the printk buffer:
> 

I was certain I had tested this case because I was monitoring to make
sure the stats were not updating until the tuning knob was used. It was
because the context was unsafe that any printing happened because at the
time trying to enable stats in that context blew up.

> In addition to the deadlock, I think this code has other issues:
> 
> 1. Instead of just warning and allowing the tracepoints to be broken,
>    I'd argue that it would be better to make them work by forcing
>    schedstats enabled and printing a warning about that, which is what's
>    already being done in other similar cases (latencytop and profiling).
>    Otherwise things like "perf sched record" won't have the intended
>    effect.  In fact the comment in the above code snippet seems to agree
>    with me, so maybe that was the original intent.
> 
> 2. It's called in the scheduler hot path from enqueue_entity().
> 
> So change the warning to a call to force_schedstat_enabled(), and do it
> only when the tracepoint gets enabled.
> 
> Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
> Cc: stable@...r.kernel.org
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>

I like it! I wasn't aware there was an option to have a registration
callback.

Acked-by: Mel Gorman <mgorman@...hsingularity.net>

-- 
Mel Gorman
SUSE Labs