On 9/25/18 12:40 PM, Jakub Jelinek wrote:
> On Tue, Sep 25, 2018 at 12:10:42PM +0200, Martin Liška wrote: On 9/25/18
>> 11:24 AM, Jakub Jelinek wrote:
>>> On Tue, Sep 25, 2018 at 11:05:30AM +0200, Martin Liška wrote:
>>>> As requested in PR81715, GCC emits bigger middle redzones for small variables.
>>>> It's analyzed in following comment: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715#c28
>>>
>>> First of all, does LLVM make the variable sized red zone size only for
>>> automatic variables, or also for global/local statics, or for alloca?
>>
>> Yes, definitely for global variables, as seen here:
>>
>> lib/Transforms/Instrumentation/AddressSanitizer.cpp:
>>   2122      Type *Ty = G->getValueType();
>>   2123      uint64_t SizeInBytes = DL.getTypeAllocSize(Ty);
>>   2124      uint64_t MinRZ = MinRedzoneSizeForGlobal();
>>   2125      // MinRZ <= RZ <= kMaxGlobalRedzone
>>   2126      // and trying to make RZ to be ~ 1/4 of SizeInBytes.
>>   2127      uint64_t RZ = std::max(
>>   2128          MinRZ, std::min(kMaxGlobalRedzone, (SizeInBytes / MinRZ / 4) * MinRZ));
>>   2129      uint64_t RightRedzoneSize = RZ;
>>   2130      // Round up to MinRZ
>>   2131      if (SizeInBytes % MinRZ) RightRedzoneSize += MinRZ - (SizeInBytes % MinRZ);
>>   2132      assert(((RightRedzoneSize + SizeInBytes) % MinRZ) == 0);
>>   2133      Type *RightRedZoneTy = ArrayType::get(IRB.getInt8Ty(), RightRedzoneSize);
>>
>> So roughly to SizeInBytes / 4. Confirmed:
> 
> Changing non-automatic vars will be certainly harder.  Let's do it later.
> 
>>> Have you considered also making the red zones larger for very large
>>> variables?
>>
>> I was mainly focused on shrinking as that's limiting usage of asan-stack in KASAN.
>> But I'm open for it. Would you follow what LLVM does, or do you have a specific idea how
>> to growth?
> 
> Dunno if they've done some analysis before picking the current sizes, unless
> we you do some I'd follow their numbers, it doesn't look totally
> unreasonable, a compromise between not wasting too much for more frequent
> smaller vars and for larger vars catching even larger out of bound accesses.

Agree with that!

> 
>>> What exactly would need changing to support the 12-15 bytes long red zones
>>> for 4-1 bytes long automatic vars?
>>> Just asan_emit_stack_protection or something other?
>>
>> Primarily this function, that would need a generalization. Apart from that we're
>> also doing alignment to ASAN_RED_ZONE_SIZE:
>>
>> 	      prev_offset = align_base (prev_offset,
>> 					MAX (alignb, ASAN_RED_ZONE_SIZE),
>> 					!FRAME_GROWS_DOWNWARD);
>>
>> Maybe it has consequences I don't see right now?
> 
> Actually, I think even:
> +                 && i != n - 1                                                                                                                    
> in your patch isn't correct, vars could be last even if they aren't == n - 1
> or vice versa, the sorting is done by many factors, vars can go into
> multiple buckets based on predicate etc.
> So, rather than trying to guess what is last here it should be left to
> expand_used_vars for one side, and perhaps based on whether any vars were
> placed at all on the other side (don't remember if asan supports anything
> but FRAME_GROWS_DOWNWARD).

OK, it only affects size of red redzone, that can be bigger.

> 
>> First feedback is positive:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715#c30
>>
>> It's questionable whether handling of variables 1-4B wide worth further changes.
> 
> I'd think the really small vars are quite common, isn't that the case (sure,
> address taken ones will be less common, but still not rare).

So I decided to write the patch properly, I have a working version that survives
asan.exp tests. The code that creates red-zones is more generic and all stack
vars are now aligned just to ASAN_SHADOW_GRANULARITY.

The only missing piece is how to implement asan_emit_redzone_payload more smart.
It means doing memory stores with 8,4,2,1 sizes in order to reduce # of insns.
Do we have somewhere a similar code?

Do you like the generalization I did in general?

Thanks,
Maritn

> 
> 	Jakub
>