Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

[Andy Lutomirski]

On Aug 23, 2016 12:11 AM, "Linus Torvalds"
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > Convert arch_within_stack_frames() to use the new unwinder.
>
> Please don't do this.
>
> There's no real reason to unwind the stack frame. If it's not on the
> current stack page, it shouldn't be a valid source anyway, so
> unwidning things just seems entirely pointless.
>
> Quite frankly, I think the whole "look at the stack frames" logic
> should be removed from this. It's classic crap that external patches
> do. How many call-sites does it actually check, and how many of them
> aren't already checked by the existing static checks for constant
> addresses within existing objects?
>

I'm a bit confused by what you're objecting to.  If I write:

char buf[123];

func(buf, size);

And func eventually does some usercopy to buf, the idea is to check
that size is in bounds.  Now admittedly this kind of code should be
quite rare in the kernel, and it should be even rarer for the buffer
to be more than a frame or two up the stack.

So the fact that this seems to have any significant effect on
performance suggests to me that it's being run unnecessarily or that
somehow we're walking all the way to the top of the stack in cases
where we shouldn't have done so.

Josh, can you see an example call site in a profile of your test to
find out what this code is doing?

All that being said, Linus, assuming that Josh's new unwinder can be
made reasonably performant, I don't understand your objection to this
patch in particular.  Josh isn't changing the way that usercopy
hardening works -- he's just changing the stack walking
implementation.  It seems that you're objecting to this code in
general, but that predates Josh's patch, no?

--Andy