linux-kernel - Re: A bug in ftrace

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: <20160809081748.5c8e7d1b@gandalf.local.home>
Date:	Tue, 9 Aug 2016 08:17:48 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: A bug in ftrace - dynamic fops

On Tue, 9 Aug 2016 10:16:00 +0200 (CEST)
Miroslav Benes <mbenes@...e.cz> wrote:

> I agree it is kind of shooting oneself in the foot bug, because explicit 
> call to a sleeping function may not be the brightest thing to do. However 
> I see two (closely related) issues with this.
> 
> 1. It is a change in behaviour. Ftrace silently relies on an atomicity of 
> ops->func(). I don't see it documented anywhere, but it did not matter 
> because the atomicity was always guaranteed as described above. Now there 
> is a possibility to achieve a situation which breaks the assumption. It 
> makes me worried.

Why? It's something that a kernel developer should be aware of. I mean,
that ops->func can easily be called from *any* context, like irq,
softirq, or even an NMI. One who hooks into any function of the kernel
should understand that it has special requirements, just like we don't
document that you can't sleep in an NMI.

And if you only hook to functions that can sleep, then great! You are
allowed to do that too. Just like calling a module function that can
sleep. You need to make sure nothing is calling your function when you
unload the module. I don't see anything that is deceptive here.

> 
> 2. Previously if someone called a function which could sleep he was 
> immediately warned not to do so via "sleeping in atomic context" BUG. Now 
> he wouldn't know. That's because in_atomic() and might_sleep() 
> infrastructure does not work in ops->func(). in_atomic() gives 0 even if 
> it is an atomic context in fact. But well, the comment for in_atomic() in 
> linux/preempt.h warns about exactly this situation I guess.

It will warn if you hook to a function that can sleep. And if you never
do, then there's nothing wrong. If the only functions you hook to can
sleep, then it is fine for you to sleep in your code too. But if you
do, you must synchronize that logic. You must make sure all functions
are out of the sleep when you unresgister. Just like you must make sure
all functions are out of a sleeping function in a module. This is
kernel programming 101.

I never saw a need to have sleeping functions being called by
ops->func() and I don't know of a case that would. If there is a
legitimate case (not hypothetical) and then I could add a way to
postpone freeing of an ops if need be.

Because note, that TASK_RCU will only be called when CONFIG_PREEMPT is
enabled. It would be overkill to do it for !CONFIG_PREEMPT, thus it
will not solve what you want here.

-- Steve