Re: [RFA][PATCH] Stack clash protection 07/08 -- V4 (aarch64 bits)

[Jeff Law <law@redhat.com>]

On 10/13/2017 02:26 PM, Wilco Dijkstra wrote:
> Hi,
> 
> To continue the review of the AArch64 frame code I tried a few examples
> to figure out what it does now. For initial_adjust <= 63*1024 and final_adjust <
> 1024 there are no probes inserted as expected, ie. the vast majority of
> functions are unaffected. So that works perfectly.
Right.

> 
> For larger frames the first oddity is that there are now 2 separate params
> controlling how probes are generated:
> 
> stack-clash-protection-guard-size (default 12, but set to 16 on AArch64)
> stack-clash-protection-probe-interval (default 12)
> 
> I don't see how this makes sense. These values are closely related, so if
> one is different from the other, probing becomes ineffective/incorrect. 
> For example we generate code that trivially bypasses the guard despite
> all the probing:
My hope would be that we simply don't ever use the params.  They were
done as much for *you* to experiment with as anything.  I'd happy just
delete them as there's essentially no guard rails to ensure their values
are sane.


> 
> --param=stack-clash-protection-probe-interval=13
> --param=stack-clash-protection-guard-size=12
> 
> So if there is a good reason to continue with 2 separate values, we must
> force probe interval <= guard size!
The param code really isn't designed to enforce values that are
inter-dependent.  It has a min, max & default values.  No more, no less.
 If you set up something inconsistent with the params, it's simply not
going to work.


> 
> Also on AArch64 --param=stack-clash-protection-probe-interval=16 causes
> crashes due to the offsets used in the probes - we don't need large offsets
> as we want to probe close to the bottom of the stack.
Not a surprise.  While I tried to handle larger intervals, I certainly
didn't test them.  Given the ISA I wouldn't expect an interval > 12 to
be useful or necessarily even work correctly.


> 
> Functions with a large stack emit like alloca a lot of code, here I used
> --param=stack-clash-protection-probe-interval=15:
> 
> int f1(int x)
> {
>   char arr[128*1024];
>   return arr[x];
> }
> 
> f1:
> 	mov	x16, 64512
> 	sub	sp, sp, x16
> 	.cfi_def_cfa_offset 64512
> 	mov	x16, -32768
> 	add	sp, sp, x16
> 	.cfi_def_cfa_offset -1024
> 	str	xzr, [sp, 32760]
> 	add	sp, sp, x16
> 	.cfi_def_cfa_offset -66560
> 	str	xzr, [sp, 32760]
> 	sub	sp, sp, #1024
> 	.cfi_def_cfa_offset -65536
> 	str	xzr, [sp, 1016]
> 	ldrb	w0, [sp, w0, sxtw]
> 	.cfi_def_cfa_offset 131072
> 	add	sp, sp, 131072
> 	.cfi_def_cfa_offset 0
> 	ret
> 
> Note the cfa offsets are wrong.
Yes.  They definitely look wrong.  There's a clear logic error in
setting up the ADJUST_CFA note when the probing interval is larger than
2**12.  That should be easily fixed.  Let me poke at it.

> 
> There is an odd mix of a big initial adjustment, then some probes+adjustments and
> then a final adjustment and probe for the remainder. I can't see the point of having
> both an initial and remainder adjustment. I would expect this:
> 
> 	sub	sp, sp, 65536
> 	str	xzr, [sp, 1024]
> 	sub	sp, sp, 65536
> 	str	xzr, [sp, 1024]
> 	ldrb	w0, [sp, w0, sxtw]
> 	add	sp, sp, 131072
> 	ret
I'm really not able to justify spending further time optimizing the
aarch64 implementation.  I've done the best I can.  You can take the
work as-is or improve it, but I really can't justify further time
investment on that architecture.

> 
> 
> int f2(int x)
> {
>   char arr[128*1024];
>   return arr[x];
> }
> 
> f2:
> 	mov	x16, 64512
> 	sub	sp, sp, x16
> 	mov	x16, -65536
> 	movk	x16, 0xfffd, lsl 16
> 	add	x16, sp, x16
> .LPSRL0:
> 	sub	sp, sp, 4096
> 	str	xzr, [sp, 4088]
> 	cmp	sp, x16
> 	b.ne	.LPSRL0
> 	sub	sp, sp, #1024
> 	str	xzr, [sp, 1016]
> 	ldrb	w0, [sp, w0, sxtw]
> 	add	sp, sp, 262144
> 	ret
> 
> The cfa entries are OK for this case. There is a mix of positive/negative offsets which
> makes things confusing. Again there are 3 kinds of adjustments when for this size we
> only need the loop.
> 
> Reusing the existing gen_probe_stack_range code appears a bad idea since
> it ignores the probe interval and just defaults to 4KB. I don't see why it should be
> any more complex than this:
> 
> 	sub	x16, sp, 262144  // only need temporary if > 1MB
> .LPSRL0:
> 	sub	sp, sp, 65536
> 	str	xzr, [sp, 1024]
> 	cmp	sp, x16
> 	b.ne	.LPSRL0
> 	ldrb	w0, [sp, w0, sxtw]
> 	add	sp, sp, 262144
> 	ret
> 
> Probe insertion if final adjustment >= 1024 also generates a lot of redundant
> code - although this is more a theoretical issue given this is so rare.
Again, if ARM wants this optimized, then ARM's engineers are going to
have to take the lead here.  I've invested all I can reasonably invest
in terms of trying optimize the probing for this target.

jeff