Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 20 Apr 2012 09:16:42 -0700 | Subject | Re: [PATCH] perf, x86: Disable sanity check |
| |
On Fri, Apr 20, 2012 at 2:11 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > Makes me really nervous.. Ingo, Linus ?
I don't care as long as this only *ever* triggers for user stacks, and the code verifies that. And I'm not sure it does, actually.
Why am I not sure? Because it uses copy_from_user_nmi(), which in turn uses "access_ok()". But can we perhaps have the perf event happen *while* the kernel has done a "set_fs(KERNEL_DS)" - and we just happen to follow the user stack too? In which case we may be copying kernel memory.
So I think this user stack following code is buggy in *other* ways.
Guys: stack following has to be *f^&%ing* careful! This shows yet again how people blithely follow frame pointers without verifying everything they damn well can.
Also, I note that the deepest stack chain allowed is something *ridiculously* deep (like 255), and we use copy_from_user_nmi() for this each entry. Which is slow as hell. So I would suggest at least considering limiting that depth more.
End result: I'm ok with removing that one test. But I want more tests to replace it. The user frame pointer had damn well better be in user space (and no, "access_ok()" is not valid or sufficient in interrupt context!) and I suspect there are other things you could check.
Linus
| |