Re: [PATCH 3/4] Add libgomp plugin for Intel MIC

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, Oct 23, 2014 at 07:41:12PM +0400, Ilya Verbin wrote:
> > malloc can fail, SIGSEGV in response to that is not desirable.
> > Can't you fallback to alloca, or use just alloca, or use alloca
> > with malloc fallback?
> 
> I replaced it with alloca.

There is a risk if a suid or otherwise priviledge escalated program
uses it and attacker passes huge env vars.
Perhaps use alloca if it is <= 2KB and malloc otherwise, and in that case
if malloc fails, just do a fatal error?

> > Where does this artificial limit come from?  Using libNNN.so library names?
> > Can't you use lib%d.so instead?
> 
> Yes, it comes from the Image structure (liboffloadmic/runtime/offload_host.h:52)
> It must contain a null-terminated name, therefore I need to allocate some space
> for the name in plugin's struct TargetImage.  But the structure can't contain
> any bytes after the trailing zero and before the actual data.
> So, now I extended the name to 10 digits and removed the comparison with 1000.

Ok.

> > Also, seeing register_image, shouldn't there be
> > GOMP_OFFLOAD_unregister_image which would be invoked when the library
> > containing MIC offloading regions is dlclosed?
> > One could use __cxa_atexit or similar for that, something that is given
> > &__dso_handle.  Or is no cleanup necessary?  At least unregistering it
> > from translation tables, because the same addresses might be reused by a
> > different shared library?
> > With dlopen/dlclose in mind, 1000 might be easily reached, consider 10000
> > times dlopening/dlclosing (perhaps over longer time, by long running daemon)
> > a shared library containg #pragma omp target region.
> 
> Hmm, previously we've tested only cases when all libraries are loaded before the
> first offload.  Offloading from a dlopened library after the call to
> gomp_target_init isn't working.  So, this will require some changes in
> libgomp/target.c .  Is it ok to fix this bug in a separate patch?

I guess it can be done incrementally, even during stage3.

	Jakub