diff mbox

[2/4] Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin

Message ID 1de94c83-96da-f380-9964-1472f63270c9@mentor.com
State New
Headers show

Commit Message

Tom de Vries June 26, 2017, 11:38 a.m. UTC 
On 06/26/2017 01:24 PM, Tom de Vries wrote:
> Hi,
> 
> I've written a patch series to facilitate debugging libgomp openacc 
> testcase failures on the nvptx accelerator.
> 
> 
> When running an openacc test-case on an nvptx accelerator, the following 
> happens:
> - the plugin obtains the ptx assembly for the acceleration kernels
> - it calls the cuda jit to compile and link the ptx into a module
> - it loads the module
> - it starts an acceleration kernel
> 
> The patch series adds these environment variables:
> - GOMP_OPENACC_NVPTX_SAVE_TEMPS: a means to save the resulting module
>    such that it can be investigated using nvdisasm and cuobjdump.
> - GOMP_OPENACC_NVPTX_DISASM: a means to see the resulting module in
>    the debug output,  by writing it into a file and calling nvdisasm on
>    it
> - GOMP_OPENACC_NVPTX_JIT: a means to set parameters of the
>    compilation/linking process, currently supporting:
>    * -O[0-4], mapping onto CU_JIT_OPTIMIZATION_LEVEL
>    * -ori, mapping onto CU_JIT_NEW_SM3X_OPT
> 
> 
> The patch series consists of these patches:
> 
> 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin

This patch adds handling of:
- GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
- GOMP_OPENACC_NVPTX_DISASM=[01]

The filename used for dumping the module is plugin-nvptx.<pid>.cubin.

Thanks,
- Tom

Comments

Joseph Myers June 26, 2017, 3:26 p.m. UTC | #1 
On Mon, 26 Jun 2017, Tom de Vries wrote:

> > 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
> 
> This patch adds handling of:
> - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
> - GOMP_OPENACC_NVPTX_DISASM=[01]
> 
> The filename used for dumping the module is plugin-nvptx.<pid>.cubin.

Are you sure this use of getenv and writing to that file is safe for 
setuid/setgid programs?  I'd expect you to need to use secure_getenv as in 
plugin-hsa.c; certainly for anything that could results in writes to a 
file like that.
Jakub Jelinek June 26, 2017, 3:29 p.m. UTC | #2 
On Mon, Jun 26, 2017 at 03:26:57PM +0000, Joseph Myers wrote:
> On Mon, 26 Jun 2017, Tom de Vries wrote:
> 
> > > 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
> > 
> > This patch adds handling of:
> > - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
> > - GOMP_OPENACC_NVPTX_DISASM=[01]
> > 
> > The filename used for dumping the module is plugin-nvptx.<pid>.cubin.
> 
> Are you sure this use of getenv and writing to that file is safe for 
> setuid/setgid programs?  I'd expect you to need to use secure_getenv as in 
> plugin-hsa.c; certainly for anything that could results in writes to a 
> file like that.

Yeah, definitely it should be using secure_getenv/__secure_getenv.
And IMNSHO GOMP_DEBUG too.

	Jakub
Thomas Schwinge July 3, 2017, 2:08 p.m. UTC | #3 
Hi!

On Mon, 26 Jun 2017 17:29:11 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jun 26, 2017 at 03:26:57PM +0000, Joseph Myers wrote:
> > On Mon, 26 Jun 2017, Tom de Vries wrote:
> > 
> > > > 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
> > > 
> > > This patch adds handling of:
> > > - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
> > > - GOMP_OPENACC_NVPTX_DISASM=[01]

Why the "OPENACC" in these names?  Doesn't this debugging aid apply to
any variant of offloading?

> > > The filename used for dumping the module is plugin-nvptx.<pid>.cubin.

Also, I suggest to make these names similar to their controlling options,
that is: "gomp-nvptx*", for example.

> > Are you sure this use of getenv and writing to that file is safe for 
> > setuid/setgid programs?  I'd expect you to need to use secure_getenv as in 
> > plugin-hsa.c; certainly for anything that could results in writes to a 
> > file like that.
> 
> Yeah, definitely it should be using secure_getenv/__secure_getenv.

ACK.

> And IMNSHO GOMP_DEBUG too.

But why that?  Isn't GOMP_DEBUG just controlling terminal debugging
output (that you'd also like to see in setuid/setgid programs)?


Grüße
 Thomas
Jakub Jelinek July 3, 2017, 2:18 p.m. UTC | #4 
On Mon, Jul 03, 2017 at 04:08:10PM +0200, Thomas Schwinge wrote:
> > And IMNSHO GOMP_DEBUG too.
> 
> But why that?  Isn't GOMP_DEBUG just controlling terminal debugging
> output (that you'd also like to see in setuid/setgid programs)?

The output could go into stderr, which could very well be redirected into
some file and some other program could be expecting specific content in
there.  So allowing an attacker to add there other stuff is really
dangerous.  If you want to use GOMP_DEBUG on suid/sgid processes, just
run them under root.

	Jakub
Tom de Vries July 3, 2017, 2:24 p.m. UTC | #5 
On 07/03/2017 04:08 PM, Thomas Schwinge wrote:
> Hi!
> 
> On Mon, 26 Jun 2017 17:29:11 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Mon, Jun 26, 2017 at 03:26:57PM +0000, Joseph Myers wrote:
>>> On Mon, 26 Jun 2017, Tom de Vries wrote:
>>>
>>>>> 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
>>>>
>>>> This patch adds handling of:
>>>> - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
>>>> - GOMP_OPENACC_NVPTX_DISASM=[01]
> 
> Why the "OPENACC" in these names?

I took the format from 'GOMP_OPENACC_DIM'.

> Doesn't this debugging aid apply to
> any variant of offloading?

I guess you're right. These environment variables would also be 
applicable for f.i. offloading via openmp on nvptx. I'll strip the 
'OPENACC_' bit from the variables.

>>>> The filename used for dumping the module is plugin-nvptx.<pid>.cubin.
> 
> Also, I suggest to make these names similar to their controlling options,
> that is: "gomp-nvptx*", for example.
> 

Makes sense, will do.

Thanks,
- Tom
Tom de Vries July 4, 2017, 10:05 a.m. UTC | #6 
On 07/03/2017 04:24 PM, Tom de Vries wrote:
> On 07/03/2017 04:08 PM, Thomas Schwinge wrote:
>> Hi!
>>
>> On Mon, 26 Jun 2017 17:29:11 +0200, Jakub Jelinek <jakub@redhat.com> 
>> wrote:
>>> On Mon, Jun 26, 2017 at 03:26:57PM +0000, Joseph Myers wrote:
>>>> On Mon, 26 Jun 2017, Tom de Vries wrote:
>>>>
>>>>>> 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx 
>>>>>> plugin
>>>>>
>>>>> This patch adds handling of:
>>>>> - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
>>>>> - GOMP_OPENACC_NVPTX_DISASM=[01]
>>
>> Why the "OPENACC" in these names?
> 
> I took the format from 'GOMP_OPENACC_DIM'.
> 
>> Doesn't this debugging aid apply to
>> any variant of offloading?
> 
> I guess you're right. These environment variables would also be 
> applicable for f.i. offloading via openmp on nvptx. I'll strip the 
> 'OPENACC_' bit from the variables.
> 
>>>>> The filename used for dumping the module is plugin-nvptx.<pid>.cubin.
>>
>> Also, I suggest to make these names similar to their controlling options,
>> that is: "gomp-nvptx*", for example.
>>
> 
> Makes sense, will do.

Changes in the patch series:
- removed OPENACC_ from environment variable names
- made temp files use gomp-nvptx prefix.
- fixed build error due to missing _GNU_SOURCE in libgomp-nvptx.c.
- merged the three GOMP_NVPTX_JIT patches into one
- rewrote GOMP_NVPTX_JIT to add no extra flags to the JIT compiler
   invocation if GOMP_NVPTX_JIT if not defined, removing the need for
   hardcoding default values
- added CU_JIT_TARGET to plugin/cuda/cuda.h

Build on x86_64 with nvptx offloading enabled (using plugin/cuda/cuda.h).

The patch series now looks like:
1. Handle GOMP_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
2. Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin
3. Handle GOMP_NVPTX_JIT={-O[0-4],-ori,-arch=<n>} in libgomp nvptx
    plugin

I'll repost the patch series in reply to this email.

Thanks,
- Tom
Cesar Philippidis Nov. 7, 2017, 2:48 p.m. UTC | #7 
On 07/04/2017 03:05 AM, Tom de Vries wrote:
> On 07/03/2017 04:24 PM, Tom de Vries wrote:
>> On 07/03/2017 04:08 PM, Thomas Schwinge wrote:
>>> Hi!
>>>
>>> On Mon, 26 Jun 2017 17:29:11 +0200, Jakub Jelinek <jakub@redhat.com>
>>> wrote:
>>>> On Mon, Jun 26, 2017 at 03:26:57PM +0000, Joseph Myers wrote:
>>>>> On Mon, 26 Jun 2017, Tom de Vries wrote:
>>>>>
>>>>>>> 2. Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx
>>>>>>> plugin
>>>>>>
>>>>>> This patch adds handling of:
>>>>>> - GOMP_OPENACC_NVPTX_SAVE_TEMPS=[01], and
>>>>>> - GOMP_OPENACC_NVPTX_DISASM=[01]
>>>
>>> Why the "OPENACC" in these names?
>>
>> I took the format from 'GOMP_OPENACC_DIM'.
>>
>>> Doesn't this debugging aid apply to
>>> any variant of offloading?
>>
>> I guess you're right. These environment variables would also be
>> applicable for f.i. offloading via openmp on nvptx. I'll strip the
>> 'OPENACC_' bit from the variables.
>>
>>>>>> The filename used for dumping the module is plugin-nvptx.<pid>.cubin.
>>>
>>> Also, I suggest to make these names similar to their controlling
>>> options,
>>> that is: "gomp-nvptx*", for example.
>>>
>>
>> Makes sense, will do.
> 
> Changes in the patch series:
> - removed OPENACC_ from environment variable names
> - made temp files use gomp-nvptx prefix.
> - fixed build error due to missing _GNU_SOURCE in libgomp-nvptx.c.
> - merged the three GOMP_NVPTX_JIT patches into one
> - rewrote GOMP_NVPTX_JIT to add no extra flags to the JIT compiler
>   invocation if GOMP_NVPTX_JIT if not defined, removing the need for
>   hardcoding default values
> - added CU_JIT_TARGET to plugin/cuda/cuda.h
> 
> Build on x86_64 with nvptx offloading enabled (using plugin/cuda/cuda.h).
> 
> The patch series now looks like:
> 1. Handle GOMP_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
> 2. Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin
> 3. Handle GOMP_NVPTX_JIT={-O[0-4],-ori,-arch=<n>} in libgomp nvptx
>    plugin
> 
> I'll repost the patch series in reply to this email.

Ping.

Can we get this patch series into trunk and og7? The ability to easily
modify PTX code, via GOMP_NVPTX_PTXRW, is extremely helpful. It helped
me isolate one problem already.

Thanks,
Cesar
Jakub Jelinek Nov. 7, 2017, 2:54 p.m. UTC | #8 
On Tue, Nov 07, 2017 at 06:48:25AM -0800, Cesar Philippidis wrote:
> > Changes in the patch series:
> > - removed OPENACC_ from environment variable names
> > - made temp files use gomp-nvptx prefix.
> > - fixed build error due to missing _GNU_SOURCE in libgomp-nvptx.c.
> > - merged the three GOMP_NVPTX_JIT patches into one
> > - rewrote GOMP_NVPTX_JIT to add no extra flags to the JIT compiler
> >   invocation if GOMP_NVPTX_JIT if not defined, removing the need for
> >   hardcoding default values
> > - added CU_JIT_TARGET to plugin/cuda/cuda.h
> > 
> > Build on x86_64 with nvptx offloading enabled (using plugin/cuda/cuda.h).
> > 
> > The patch series now looks like:
> > 1. Handle GOMP_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin
> > 2. Handle GOMP_NVPTX_PTXRW in libgomp nvptx plugin
> > 3. Handle GOMP_NVPTX_JIT={-O[0-4],-ori,-arch=<n>} in libgomp nvptx
> >    plugin
> > 
> > I'll repost the patch series in reply to this email.
> 
> Ping.
> 
> Can we get this patch series into trunk and og7? The ability to easily
> modify PTX code, via GOMP_NVPTX_PTXRW, is extremely helpful. It helped
> me isolate one problem already.

It can be helpful for debugging, but I'm afraid about having such code in
production, I think something like this would be very easy to exploit.
Sure, running a suid or sgid program with offloading is probably very
dangerous anyway, but it could be just some minor priviledge escalation
in the app (SELinux, ACLs, whatever else) and this stuff would allow anyone
to run anything else.
So, IMNSHO if it should be added, only enabled by non-default configure
option.

	Jakub
diff mbox

Patch

Handle GOMP_OPENACC_NVPTX_{DISASM,SAVE_TEMPS} in libgomp nvptx plugin

2017-06-26  Tom de Vries  <tom@codesourcery.com>

	* plugin/plugin-nvptx.c (do_prog, debug_linkout): New function.
	(link_ptx): Use debug_linkout.

---
 libgomp/plugin/plugin-nvptx.c | 103 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/libgomp/plugin/plugin-nvptx.c b/libgomp/plugin/plugin-nvptx.c
index 71630b5..df1bfdd 100644
--- a/libgomp/plugin/plugin-nvptx.c
+++ b/libgomp/plugin/plugin-nvptx.c
@@ -47,6 +47,9 @@ 
 #include <unistd.h>
 #include <assert.h>
 #include <errno.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #if PLUGIN_NVPTX_DYNAMIC
 # include <dlfcn.h>
@@ -876,6 +879,104 @@  notify_var (const char *var_name, const char *env_var)
     GOMP_PLUGIN_debug (0, "%s: '%s'\n", var_name, env_var);
 }
 
+static void
+do_prog (const char *prog, const char *arg)
+{
+  pid_t pid = fork ();
+
+  if (pid == -1)
+    {
+      GOMP_PLUGIN_error ("Fork failed");
+      return;
+    }
+  else if (pid > 0)
+    {
+      int status;
+      waitpid (pid, &status, 0);
+      if (!WIFEXITED (status))
+	GOMP_PLUGIN_error ("Running %s %s failed", prog, arg);
+    }
+  else
+    {
+      execlp (prog, prog /* argv[0] */, arg, NULL);
+      abort ();
+    }
+}
+
+static void
+debug_linkout (void *linkout, size_t linkoutsize)
+{
+  static int gomp_openacc_nvptx_disasm = -1;
+  if (gomp_openacc_nvptx_disasm == -1)
+    {
+      const char *var_name = "GOMP_OPENACC_NVPTX_DISASM";
+      const char *env_var = getenv (var_name);
+      notify_var (var_name, env_var);
+      gomp_openacc_nvptx_disasm
+	= ((env_var != NULL && env_var[0] == '1' && env_var[1] == '\0')
+	   ? 1 : 0);
+    }
+
+  static int gomp_openacc_nvptx_save_temps = -1;
+  if (gomp_openacc_nvptx_save_temps == -1)
+    {
+      const char *var_name = "GOMP_OPENACC_NVPTX_SAVE_TEMPS";
+      const char *env_var = getenv (var_name);
+      notify_var (var_name, env_var);
+      gomp_openacc_nvptx_save_temps
+	= ((env_var != NULL && env_var[0] == '1' && env_var[1] == '\0')
+	   ? 1 : 0);
+    }
+
+  if (gomp_openacc_nvptx_disasm == 0
+      && gomp_openacc_nvptx_save_temps == 0)
+    return;
+
+  const char *prefix = "plugin-nvptx.";
+  const char *postfix = ".cubin";
+  const int len =	(strlen (prefix)
+			 + 20 /* %lld.  */
+			 + strlen (postfix)
+			 + 1  /* '\0'.  */);
+  char file_name[len];
+  int res = snprintf (file_name, len, "%s%lld%s", prefix,
+		      (long long)getpid (), postfix);
+  assert (res < len); /* Assert there's no truncation.  */
+
+  GOMP_PLUGIN_debug (0, "Generating %s with size %zu\n",
+		     file_name, linkoutsize);
+  FILE *cubin_file = fopen (file_name, "wb");
+  if (cubin_file == NULL)
+    {
+      GOMP_PLUGIN_debug (0, "Opening %s failed\n", file_name);
+      return;
+    }
+
+  fwrite (linkout, linkoutsize, 1, cubin_file);
+  unsigned int write_succeeded = ferror (cubin_file) == 0;
+  if (!write_succeeded)
+    GOMP_PLUGIN_debug (0, "Writing %s failed\n", file_name);
+
+  res = fclose (cubin_file);
+  if (res != 0)
+    GOMP_PLUGIN_debug (0, "Closing %s failed\n", file_name);
+
+  if (!write_succeeded)
+    return;
+
+  if (gomp_openacc_nvptx_disasm == 1)
+    {
+      GOMP_PLUGIN_debug (0, "Disassembling %s\n", file_name);
+      do_prog ("nvdisasm", file_name);
+    }
+
+  if (gomp_openacc_nvptx_save_temps == 0)
+    {
+      GOMP_PLUGIN_debug (0, "Removing %s\n", file_name);
+      remove (file_name);
+    }
+}
+
 static bool
 link_ptx (CUmodule *module, const struct targ_ptx_obj *ptx_objs,
 	  unsigned num_objs)
@@ -939,6 +1040,8 @@  link_ptx (CUmodule *module, const struct targ_ptx_obj *ptx_objs,
       return false;
     }
 
+  debug_linkout (linkout, linkoutsize);
+
   CUDA_CALL (cuModuleLoadData, module, linkout);
   CUDA_CALL (cuLinkDestroy, linkstate);
   return true;