[PATCH v2] loader: check return values of wld_mprotect and wld_mmap calls

[Sebastian Lackner]

On 22.12.2015 16:31, Paul Gofman wrote:
> On 12/22/2015 05:33 PM, Sebastian Lackner wrote:
>> Its not my decision, but I don't think its a good idea to add all the (unnecessary) ifs
>> just to make this code compatible with ICC. I assume the compiler doesn't handle inline
>> assembly correctly, but this would mean that there are several more affected places.
>> It would be nice to have a more general solution to fix/workaround this issue.
>> If this is not possible, I would at least suggest to add a workaround to the functions
>> directly, instead of adding checks to all places where they are called.
>>
>>
>     I think the cause of problem is is ICC aggressive optimization which
> is sometimes buggy under specific conditions. In this case ICC seems to
> ignore the side effect of asm syscall. I think it does not mean that it
> handles all of inline assembly incorrectly. This problem is hard to
> reproduce: I can't get the same problem using the same wld_mmap and
> wld_mprotect functions in a small separate test case, so this likely
> happens under some specific conditions only.
>     Yes, surely there might be other places like (or not quite like)
> that which will misbehave the same way which I did not come across yet.
> But I do not know any general way to track and fix them all at once.
> Maybe except for disabling (some) optimization which actually makes use
> of ICC non sensible.

Trying different optimization flags is one possibility. Alternatively, you could
also try to remove static/inline or adding __volatile__ to the assembler code.

>     I was hoping that adding a few checks like that won't hurt the code
> (I understand though that the checks may be paranoid or redundant apart
> from ICC issue). If this is not the case then maybe some other
> solution/workaround for this ICC issue can be found.
>     Do you think it is better to move return value check directly into
> wld_mmap and wld_mprotect instead of checking their status on call? I
> thought that checking return status rather than building in such an
> effect into system function wrapper is a common approach. Or maybe I
> could add wrapper inline functions with error checking?

There are probably more ways to workaround the issue besides if-checks. If possible,
something else (which keeps the return value) would definitely be preferred. A
couple of ideas which might help (not sure if they work):

- Replace SYSCALL_RET() macro with an (inline?) function.
- Replace inline assembly with a global ASM function (without C wrapper around).
  See for example how its done for _start, or for the x86_64 functions. If you need
  help with the assembler code, feel free to ask.
- Pass address of ret to the assembler code, and let assembler code write the result.
  The compiler is probably no longer able to optimize away the function content then.