Add UNSUPPORTED check in elf/tst-pldd.

Hi,

the testcase forks a child process and runs pldd with PID of
this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
differs from zero, pldd will fail with
/usr/bin/pldd: cannot attach to process 3: Operation not permitted

This patch checks if ptrace_scope is zero and otherwise marks the
test as UNSUPPORTED.

Bye
Stefan

ChangeLog:

	* elf/tst-pldd.c (do_test): Add UNSUPPORTED check.

On 27/08/2019 07:19, Stefan Liebler wrote:
> Hi,
> 
> the testcase forks a child process and runs pldd with PID of
> this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
> differs from zero, pldd will fail with
> /usr/bin/pldd: cannot attach to process 3: Operation not permitted
> 
> This patch checks if ptrace_scope is zero and otherwise marks the
> test as UNSUPPORTED.
> 
> Bye
> Stefan
> 
> ChangeLog:
> 
>     * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
> 
> 20190826_tst-pldd.patch
> 
> commit 9c0b03c38bdd31618909da46b8bd4e09b5a236d2
> Author: Stefan Liebler <stli@linux.ibm.com>
> Date:   Mon Aug 26 15:45:07 2019 +0200
> 
>     Add UNSUPPORTED check in elf/tst-pldd.
>     
>     The testcase forks a child process and runs pldd with PID of
>     this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
>     differs from zero, pldd will fail with
>     /usr/bin/pldd: cannot attach to process 3: Operation not permitted
>     
>     This patch checks if ptrace_scope is zero and otherwise marks the
>     test as UNSUPPORTED.
>     
>     ChangeLog:
>     
>             * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
> 
> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> index 6b7c94a1c0..3f211dc342 100644
> --- a/elf/tst-pldd.c
> +++ b/elf/tst-pldd.c
> @@ -52,6 +52,24 @@ in_str_list (const char *libname, const char *const strlist[])
>  static int
>  do_test (void)
>  {
> +  /* Check if all processes can be debugged with ptrace.  */
> +  {
> +    FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
> +    if (f != NULL)
> +      {
> +	/* If ptrace_scope exists, then it has to be 0 which means
> +	   "classic ptrace permissions".  A process can PTRACE_ATTACH
> +	   to any other process running under the same uid, as long as
> +	   it is dumpable.  Otherwise pldd will fail to attach to the
> +	   subprocess.  */
> +	int i = 99;
> +	fscanf (f, "%d", &i);
> +	fclose (f);
> +	if (i != 0)
> +	  FAIL_UNSUPPORTED ("/proc/sys/kernel/yama/ptrace_scope != 0");
> +      }
> +  }
> +

This is a Linuxism and I think we should create a 'support_can_ptrace' similar
to 'support_can_chroot'.  The logic to detect it seems correct, I would just
check fscanf returned to value and use xfclose.  It would be something like

bool
support_can_ptrace (void)
{
  bool ret = true;

#ifdef __linux__
  /* YAMA may be not enabled.  If it is then ptrace_scope it has to be 0
     which means "classic ptrace permissions".  A process can 
     PTRACE_ATTACH to any other process running under the same uid, as
     long as it is dumpable.  Otherwise pldd will fail to attach to the
     subprocess.  */
  FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
  if (f == NULL)
    return true;

  int i = 99;
  TEST_COMPARE (fscanf (f, "%d", &i), 1);
  xfclose (f);
  ret = i == 0;
#endif

  return ret;
}

And I think we might eventually need to handle seccomp as well.

>    /* Create a copy of current test to check with pldd.  */
>    struct support_subprocess target = support_subprocess (target_process, NULL);
>  
>

* Adhemerval Zanella:

> This is a Linuxism and I think we should create a 'support_can_ptrace'
> similar to 'support_can_chroot'.  The logic to detect it seems
> correct, I would just check fscanf returned to value and use xfclose.
> It would be something like

The test does the right thing if the path does not exist, so I'm not
sure if it is necessary.

support_can_ptrace would be misleading because even with YAMA scope
support, tracing direct subprocesses will still work.

I fact, I think we might be able to get this test to work even with
fairly restrictive YAMA scopes, by proper ordering of forks and using
execve to run tst-pldd.

Thanks,
Florian

On 27/08/2019 12:14, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> This is a Linuxism and I think we should create a 'support_can_ptrace'
>> similar to 'support_can_chroot'.  The logic to detect it seems
>> correct, I would just check fscanf returned to value and use xfclose.
>> It would be something like
> 
> The test does the right thing if the path does not exist, so I'm not
> sure if it is necessary.

Even though, a check for a Linux specific path should be restricted to
Linux builds only.

> 
> support_can_ptrace would be misleading because even with YAMA scope
> support, tracing direct subprocesses will still work.

Indeed, a better prototype would be indeed:

/* Return the current YAMA mode set on the machine (0 to 3) or -1
   if YAMA is not supported.  */
int support_ptrace_scope (void);

> 
> I fact, I think we might be able to get this test to work even with
> fairly restrictive YAMA scopes, by proper ordering of forks and using
> execve to run tst-pldd.

The problem with ptrace_scope 1 is tst-pldd will either make its fork 
helper process a pldd descendant or pass pldd pid to the forked process 
so it can call prctl (PR_SET_PTRACER, ...).  In either case I am not 
really sure it is possible without change pldd process itself (since 
it does not have an interactive mode), nor if the complexity to support 
this specific scenarios pays off. 

The ptrace_scope 2 is even more tricky since it requires CAP_SYS_PTRACE.

In any case I think by restricting the test to run on ptrace_scope 0
is a fair assumption.

> 
> Thanks,
> Florian
>

On 8/27/19 9:11 PM, Adhemerval Zanella wrote:
> 
> 
> On 27/08/2019 12:14, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> This is a Linuxism and I think we should create a 'support_can_ptrace'
>>> similar to 'support_can_chroot'.  The logic to detect it seems
>>> correct, I would just check fscanf returned to value and use xfclose.
>>> It would be something like
>>
>> The test does the right thing if the path does not exist, so I'm not
>> sure if it is necessary.
> 
> Even though, a check for a Linux specific path should be restricted to
> Linux builds only.
> 
>>
>> support_can_ptrace would be misleading because even with YAMA scope
>> support, tracing direct subprocesses will still work.
> 
> Indeed, a better prototype would be indeed:
> 
> /* Return the current YAMA mode set on the machine (0 to 3) or -1
>     if YAMA is not supported.  */
> int support_ptrace_scope (void);
> 
>>
>> I fact, I think we might be able to get this test to work even with
>> fairly restrictive YAMA scopes, by proper ordering of forks and using
>> execve to run tst-pldd.
> 
> The problem with ptrace_scope 1 is tst-pldd will either make its fork
> helper process a pldd descendant or pass pldd pid to the forked process
> so it can call prctl (PR_SET_PTRACER, ...).  In either case I am not
> really sure it is possible without change pldd process itself (since
> it does not have an interactive mode), nor if the complexity to support
> this specific scenarios pays off.
> 
> The ptrace_scope 2 is even more tricky since it requires CAP_SYS_PTRACE.
> 
> In any case I think by restricting the test to run on ptrace_scope 0
> is a fair assumption.
> 
>>
>> Thanks,
>> Florian
>>

Please have a look at the adjusted the patch.

I've introduced new support functions.
And if ptrace_scope is 1 "restricted ptrace", I've just call prctl 
(PR_SET_PTRACER, PR_SET_PTRACER_ANY,...) on the target process.
This way the target process does not need to know the pid of pldd and 
pldd is allowed to attach to the target process.

If ptrace_scope is 2 or 3, the test is marked as UNSUPPORTED.

Thanks.
Stefan

* Stefan Liebler:

>  static void
>  target_process (void *arg)
>  {
> +  if (ptrace_scope == 1)
> +    {
> +      /* YAMA is configured to "restricted ptrace".
> +	 Disable the restriction for this subprocess.  */
> +      support_ptrace_process_set_ptracer_any ();
> +    }
> +
>    pause ();
>  }

I think this has a race condition if pldd attaches to the process before
the support_ptrace_process_set_ptracer_any call.  I have no idea how
hard it is in practice to hit this race.  It should be possible to use a
process-shared barrier or some other form of synchronization to avoid
this issue.

Thanks,
Florian

On 28/08/2019 06:06, Stefan Liebler wrote:
> On 8/27/19 9:11 PM, Adhemerval Zanella wrote:
>>
>>
>> On 27/08/2019 12:14, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> This is a Linuxism and I think we should create a 'support_can_ptrace'
>>>> similar to 'support_can_chroot'.  The logic to detect it seems
>>>> correct, I would just check fscanf returned to value and use xfclose.
>>>> It would be something like
>>>
>>> The test does the right thing if the path does not exist, so I'm not
>>> sure if it is necessary.
>>
>> Even though, a check for a Linux specific path should be restricted to
>> Linux builds only.
>>
>>>
>>> support_can_ptrace would be misleading because even with YAMA scope
>>> support, tracing direct subprocesses will still work.
>>
>> Indeed, a better prototype would be indeed:
>>
>> /* Return the current YAMA mode set on the machine (0 to 3) or -1
>>     if YAMA is not supported.  */
>> int support_ptrace_scope (void);
>>
>>>
>>> I fact, I think we might be able to get this test to work even with
>>> fairly restrictive YAMA scopes, by proper ordering of forks and using
>>> execve to run tst-pldd.
>>
>> The problem with ptrace_scope 1 is tst-pldd will either make its fork
>> helper process a pldd descendant or pass pldd pid to the forked process
>> so it can call prctl (PR_SET_PTRACER, ...).  In either case I am not
>> really sure it is possible without change pldd process itself (since
>> it does not have an interactive mode), nor if the complexity to support
>> this specific scenarios pays off.
>>
>> The ptrace_scope 2 is even more tricky since it requires CAP_SYS_PTRACE.
>>
>> In any case I think by restricting the test to run on ptrace_scope 0
>> is a fair assumption.
>>
>>>
>>> Thanks,
>>> Florian
>>>
> 
> Please have a look at the adjusted the patch.
> 
> I've introduced new support functions.
> And if ptrace_scope is 1 "restricted ptrace", I've just call prctl (PR_SET_PTRACER, PR_SET_PTRACER_ANY,...) on the target process.
> This way the target process does not need to know the pid of pldd and pldd is allowed to attach to the target process.
> 
> If ptrace_scope is 2 or 3, the test is marked as UNSUPPORTED.
> 
> Thanks.
> Stefan
> 
> 20190828_tst-pldd.patch
> 
> commit 7eba88e4f44df6f4a6d174e566b1796f2abc490c
> Author: Stefan Liebler <stli@linux.ibm.com>
> Date:   Mon Aug 26 15:45:07 2019 +0200
> 
>     Add UNSUPPORTED check in elf/tst-pldd.
>     
>     The testcase forks a child process and runs pldd with PID of
>     this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
>     differs from zero, pldd will fail with
>     /usr/bin/pldd: cannot attach to process 3: Operation not permitted
>     
>     This patch checks if ptrace_scope exists, is zero "classic ptrace permissions"
>     or one "restricted ptrace".  In case of "restricted ptrace", we can
>     effectively disable the restriction by using prctl (PR_SET_PTRACER_ANY).
>     
>     If ptrace_scope exists and has a higher restriction, then the test
>     is marked as UNSUPPORTED.
>     
>     ChangeLog:
>     
>             * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
>             (target_process): Disable restricted ptrace.
>             * support/Makefile (libsupport-routines): Add support_ptrace.
>             * support/ptrace.h: New file.
>             * support/support_ptrace.c: Likewise.

LGTM with a change below.

> 
> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> index 6b7c94a1c0..728272d177 100644
> --- a/elf/tst-pldd.c
> +++ b/elf/tst-pldd.c
> @@ -30,10 +30,20 @@
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
>  #include <support/support.h>
> +#include <support/ptrace.h>
> +
> +static int ptrace_scope;
>  
>  static void
>  target_process (void *arg)
>  {
> +  if (ptrace_scope == 1)
> +    {
> +      /* YAMA is configured to "restricted ptrace".
> +	 Disable the restriction for this subprocess.  */
> +      support_ptrace_process_set_ptracer_any ();
> +    }
> +
>    pause ();
>  }
>  
> @@ -52,6 +62,11 @@ in_str_list (const char *libname, const char *const strlist[])
>  static int
>  do_test (void)
>  {
> +  /* Check if our subprocess can be debugged with ptrace.  */
> +  ptrace_scope = support_ptrace_scope ();
> +  if (ptrace_scope >= 2)
> +    FAIL_UNSUPPORTED ("/proc/sys/kernel/yama/ptrace_scope >= 2");
> +
>    /* Create a copy of current test to check with pldd.  */
>    struct support_subprocess target = support_subprocess (target_process, NULL);
>  

I think we can restrict the test to ptrace_scope 0 for now, since as Florian
has brought we will need some synchronization with parent after the
support_ptrace_process_set_ptracer_any call.

> diff --git a/support/Makefile b/support/Makefile
> index ab66913a02..34d14eb1bb 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -56,6 +56,7 @@ libsupport-routines = \
>    support_format_hostent \
>    support_format_netent \
>    support_isolate_in_subprocess \
> +  support_ptrace \
>    support_openpty \
>    support_paths \
>    support_quote_blob \

Ok.

> diff --git a/support/ptrace.h b/support/ptrace.h
> new file mode 100644
> index 0000000000..82f79ff25c
> --- /dev/null
> +++ b/support/ptrace.h
> @@ -0,0 +1,36 @@
> +/* Support functions handling ptrace_scope.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_PTRACE_H
> +#define SUPPORT_PTRACE_H
> +
> +#include <sys/cdefs.h>
> +
> +__BEGIN_DECLS
> +
> +/* Return the current YAMA mode set on the machine (0 to 3) or -1
> +   if YAMA is not supported.  */
> +int support_ptrace_scope (void);
> +
> +/* Effectively disables YAMA restriction for the calling process
> +   if support_ptrace_scope returns 1 "restricted ptrace".  */
> +void support_ptrace_process_set_ptracer_any (void);
> +
> +__END_DECLS
> +
> +#endif

Ok.

> diff --git a/support/support_ptrace.c b/support/support_ptrace.c
> new file mode 100644
> index 0000000000..e9410384b5
> --- /dev/null
> +++ b/support/support_ptrace.c
> @@ -0,0 +1,58 @@
> +/* Support functions handling ptrace_scope.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/ptrace.h>
> +#include <sys/prctl.h>
> +
> +int
> +support_ptrace_scope (void)
> +{
> +  int ptrace_scope = -1;
> +
> +#ifdef __linux__
> +  /* YAMA may be not enabled.  Otherwise it contains a value from 0 to 3:
> +     - 0 classic ptrace permissions
> +     - 1 restricted ptrace
> +     - 2 admin-only attach
> +     - 3 no attach  */
> +  FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
> +  if (f != NULL)
> +    {
> +      TEST_COMPARE (fscanf (f, "%d", &ptrace_scope), 1);
> +      xfclose (f);
> +    }
> +#endif
> +
> +  return ptrace_scope;
> +}
> +
> +void
> +support_ptrace_process_set_ptracer_any (void)
> +{
> +#ifdef __linux__
> +  int ret = prctl (PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
> +  if (ret != 0)
> +    FAIL_EXIT1 ("Failed to disable YAMA restriction. (prctl returned %d: %m)",
> +		ret);
> +#else
> +  FAIL_UNSUPPORTED ("prctl (PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) "
> +		    "not supported!");
> +#endif
> +}
> 

Ok.

On 8/28/19 11:24 AM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>>   static void
>>   target_process (void *arg)
>>   {
>> +  if (ptrace_scope == 1)
>> +    {
>> +      /* YAMA is configured to "restricted ptrace".
>> +	 Disable the restriction for this subprocess.  */
>> +      support_ptrace_process_set_ptracer_any ();
>> +    }
>> +
>>     pause ();
>>   }
> 
> I think this has a race condition if pldd attaches to the process before
> the support_ptrace_process_set_ptracer_any call.  I have no idea how
> hard it is in practice to hit this race.  It should be possible to use a
> process-shared barrier or some other form of synchronization to avoid
> this issue.
> 
> Thanks,
> Florian
> 

I've added a synchronization with stdatomic.h on a shared memory mapping.
I've not used pthread* functions as I don't want to link against 
libpthread.so. Then further adjustments are needed.

Or should I just restrict the test ptrace_scope 0 as Adhemerval has 
proposed in his post?

Bye.
Stefan

* Stefan Liebler:

> On 8/28/19 11:24 AM, Florian Weimer wrote:
>> * Stefan Liebler:
>>
>>>   static void
>>>   target_process (void *arg)
>>>   {
>>> +  if (ptrace_scope == 1)
>>> +    {
>>> +      /* YAMA is configured to "restricted ptrace".
>>> +	 Disable the restriction for this subprocess.  */
>>> +      support_ptrace_process_set_ptracer_any ();
>>> +    }
>>> +
>>>     pause ();
>>>   }
>>
>> I think this has a race condition if pldd attaches to the process before
>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>> hard it is in practice to hit this race.  It should be possible to use a
>> process-shared barrier or some other form of synchronization to avoid
>> this issue.
>>
>> Thanks,
>> Florian
>>
>
> I've added a synchronization with stdatomic.h on a shared memory mapping.
> I've not used pthread* functions as I don't want to link against
> libpthread.so. Then further adjustments are needed.
>
> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
> proposed in his post?

Is it possible to create a process tree like this?


  parent (performs output checks)
    subprocess 1 (becomes pldd via execve)
      subprocess 2

If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
ptrace scope for ptrace_scope < 2?

Thanks,
Florian

On 8/29/19 10:47 AM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>> * Stefan Liebler:
>>>
>>>>    static void
>>>>    target_process (void *arg)
>>>>    {
>>>> +  if (ptrace_scope == 1)
>>>> +    {
>>>> +      /* YAMA is configured to "restricted ptrace".
>>>> +	 Disable the restriction for this subprocess.  */
>>>> +      support_ptrace_process_set_ptracer_any ();
>>>> +    }
>>>> +
>>>>      pause ();
>>>>    }
>>>
>>> I think this has a race condition if pldd attaches to the process before
>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>> hard it is in practice to hit this race.  It should be possible to use a
>>> process-shared barrier or some other form of synchronization to avoid
>>> this issue.
>>>
>>> Thanks,
>>> Florian
>>>
>>
>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>> I've not used pthread* functions as I don't want to link against
>> libpthread.so. Then further adjustments are needed.
>>
>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>> proposed in his post?
> 
> Is it possible to create a process tree like this?
> 
> 
>    parent (performs output checks)
>      subprocess 1 (becomes pldd via execve)
>        subprocess 2
> 
> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
> ptrace scope for ptrace_scope < 2?
Yes, this is possible.
I've rearranged the subprocesses. See attached patch.
Now we have a new function pldd_process which forks target_process,
stores the pid of target_prcess to a shared memory mapping as do_test 
needs to know this pid.

Afterwards it execve to pldd which successfully ptrace target_process in 
case of "restricted ptrace".

Please review the usage of support-subprocess-functions.

Bye,
Stefan
> 
> Thanks,
> Florian
>

On 29/08/2019 05:47, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>> * Stefan Liebler:
>>>
>>>>   static void
>>>>   target_process (void *arg)
>>>>   {
>>>> +  if (ptrace_scope == 1)
>>>> +    {
>>>> +      /* YAMA is configured to "restricted ptrace".
>>>> +	 Disable the restriction for this subprocess.  */
>>>> +      support_ptrace_process_set_ptracer_any ();
>>>> +    }
>>>> +
>>>>     pause ();
>>>>   }
>>>
>>> I think this has a race condition if pldd attaches to the process before
>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>> hard it is in practice to hit this race.  It should be possible to use a
>>> process-shared barrier or some other form of synchronization to avoid
>>> this issue.
>>>
>>> Thanks,
>>> Florian
>>>
>>
>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>> I've not used pthread* functions as I don't want to link against
>> libpthread.so. Then further adjustments are needed.
>>
>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>> proposed in his post?
> 
> Is it possible to create a process tree like this?
> 
> 
>   parent (performs output checks)
>     subprocess 1 (becomes pldd via execve)
>       subprocess 2
> 
> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
> ptrace scope for ptrace_scope < 2?

Do we really need that ad-hoc support on tst-pldd to make it support 
ptrace_scope 1?

I don't oppose the support Stefan has added on latest iteration to
make it work, but this is a lot of code to support a very specific
scenario...

On 9/2/19 9:37 PM, Adhemerval Zanella wrote:
> 
> 
> On 29/08/2019 05:47, Florian Weimer wrote:
>> * Stefan Liebler:
>>
>>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>>> * Stefan Liebler:
>>>>
>>>>>    static void
>>>>>    target_process (void *arg)
>>>>>    {
>>>>> +  if (ptrace_scope == 1)
>>>>> +    {
>>>>> +      /* YAMA is configured to "restricted ptrace".
>>>>> +	 Disable the restriction for this subprocess.  */
>>>>> +      support_ptrace_process_set_ptracer_any ();
>>>>> +    }
>>>>> +
>>>>>      pause ();
>>>>>    }
>>>>
>>>> I think this has a race condition if pldd attaches to the process before
>>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>>> hard it is in practice to hit this race.  It should be possible to use a
>>>> process-shared barrier or some other form of synchronization to avoid
>>>> this issue.
>>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>
>>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>>> I've not used pthread* functions as I don't want to link against
>>> libpthread.so. Then further adjustments are needed.
>>>
>>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>>> proposed in his post?
>>
>> Is it possible to create a process tree like this?
>>
>>
>>    parent (performs output checks)
>>      subprocess 1 (becomes pldd via execve)
>>        subprocess 2
>>
>> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
>> ptrace scope for ptrace_scope < 2?
> 
> Do we really need that ad-hoc support on tst-pldd to make it support
> ptrace_scope 1?
> 
> I don't oppose the support Stefan has added on latest iteration to
> make it work, but this is a lot of code to support a very specific
> scenario...
> 
As there are systems where ptrace_scope is configured to 1 by default, 
we should adjust the testcase as the FAIL is misleading.
(I've just recognized that Steve Ellcey had also seen this FAIL: 
https://www.sourceware.org/ml/libc-alpha/2019-07/msg00618.html)

The minimum change should be to detect ptrace_scope == 1 and mark the 
test as UNSUPPORTED. Or we change a bit more and let the test also run 
in this scenario. (Either by support_ptrace_process_set_ptracer_any or 
adjusting the subprocess-tree)

Bye
Stefan

On 03/09/2019 03:30, Stefan Liebler wrote:
> On 9/2/19 9:37 PM, Adhemerval Zanella wrote:
>>
>>
>> On 29/08/2019 05:47, Florian Weimer wrote:
>>> * Stefan Liebler:
>>>
>>>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>>>> * Stefan Liebler:
>>>>>
>>>>>>    static void
>>>>>>    target_process (void *arg)
>>>>>>    {
>>>>>> +  if (ptrace_scope == 1)
>>>>>> +    {
>>>>>> +      /* YAMA is configured to "restricted ptrace".
>>>>>> +     Disable the restriction for this subprocess.  */
>>>>>> +      support_ptrace_process_set_ptracer_any ();
>>>>>> +    }
>>>>>> +
>>>>>>      pause ();
>>>>>>    }
>>>>>
>>>>> I think this has a race condition if pldd attaches to the process before
>>>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>>>> hard it is in practice to hit this race.  It should be possible to use a
>>>>> process-shared barrier or some other form of synchronization to avoid
>>>>> this issue.
>>>>>
>>>>> Thanks,
>>>>> Florian
>>>>>
>>>>
>>>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>>>> I've not used pthread* functions as I don't want to link against
>>>> libpthread.so. Then further adjustments are needed.
>>>>
>>>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>>>> proposed in his post?
>>>
>>> Is it possible to create a process tree like this?
>>>
>>>
>>>    parent (performs output checks)
>>>      subprocess 1 (becomes pldd via execve)
>>>        subprocess 2
>>>
>>> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
>>> ptrace scope for ptrace_scope < 2?
>>
>> Do we really need that ad-hoc support on tst-pldd to make it support
>> ptrace_scope 1?
>>
>> I don't oppose the support Stefan has added on latest iteration to
>> make it work, but this is a lot of code to support a very specific
>> scenario...
>>
> As there are systems where ptrace_scope is configured to 1 by default, we should adjust the testcase as the FAIL is misleading.
> (I've just recognized that Steve Ellcey had also seen this FAIL: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00618.html)
> 
> The minimum change should be to detect ptrace_scope == 1 and mark the test as UNSUPPORTED. Or we change a bit more and let the test also run in this scenario. (Either by support_ptrace_process_set_ptracer_any or adjusting the subprocess-tree)

Yes, my initial suggestion was just to make it as UNSUPPORTED for ptrace_scope >= 1.
But I do not oppose adjusting it to run on ptrace_scope 1, it is just that
the required hackery lead to make it somewhat as complex than the test itself.

On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
> Yes, my initial suggestion was just to make it as UNSUPPORTED for
> ptrace_scope >= 1. But I do not oppose adjusting it to run on
> ptrace_scope 1, it is just that the required hackery lead to make it
> somewhat as complex than the test itself.

The flip side of the coin is that the more "UNSUPPORTED" results we
add *implies* there is "one valid way" to setup a glibc test run
and we don't clearly document how to turn all the "UNSUPPORTED"
entries into supported tests?

Stefan's code can at least be refactored into support/ if we need
to do the same thing again in another test.

On 9/6/19 5:21 AM, Carlos O'Donell wrote:
> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>> ptrace_scope 1, it is just that the required hackery lead to make it
>> somewhat as complex than the test itself.
> 
> The flip side of the coin is that the more "UNSUPPORTED" results we
> add *implies* there is "one valid way" to setup a glibc test run
> and we don't clearly document how to turn all the "UNSUPPORTED"
> entries into supported tests?
> 
> Stefan's code can at least be refactored into support/ if we need
> to do the same thing again in another test.
> 

PING.

As I have already posted multiple versions of the patch, how to proceed?
1) UNSUPPORTED if support_ptrace_scope() >= 2;
Support support_ptrace_scope() == 1
by adjusting the process tree;
(see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)

2) UNSUPPORTED if support_ptrace_scope() >= 2;
Support support_ptrace_scope() == 1
by calling support_ptrace_process_set_ptracer_any();
(see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)

3) UNSUPPORTED if support_ptrace_scope() != 0
(patch would use support_ptrace_scope() of one of the patches above in 
order to trigger FAIL_UNSUPPORTED)

Bye,
Stefan

On 10/09/2019 05:46, Stefan Liebler wrote:
> On 9/6/19 5:21 AM, Carlos O'Donell wrote:
>> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>>> ptrace_scope 1, it is just that the required hackery lead to make it
>>> somewhat as complex than the test itself.
>>
>> The flip side of the coin is that the more "UNSUPPORTED" results we
>> add *implies* there is "one valid way" to setup a glibc test run
>> and we don't clearly document how to turn all the "UNSUPPORTED"
>> entries into supported tests?
>>
>> Stefan's code can at least be refactored into support/ if we need
>> to do the same thing again in another test.
>>
> 
> PING.
> 
> As I have already posted multiple versions of the patch, how to proceed?
> 1) UNSUPPORTED if support_ptrace_scope() >= 2;
> Support support_ptrace_scope() == 1
> by adjusting the process tree;
> (see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)
> 
> 2) UNSUPPORTED if support_ptrace_scope() >= 2;
> Support support_ptrace_scope() == 1
> by calling support_ptrace_process_set_ptracer_any();
> (see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)
> 
> 3) UNSUPPORTED if support_ptrace_scope() != 0
> (patch would use support_ptrace_scope() of one of the patches above in order to trigger FAIL_UNSUPPORTED)

My view is although 2) is way complex that I would like, I think it should
the more complete solution. Does still need review or is it ready to land?

On 9/10/19 3:32 PM, Adhemerval Zanella wrote:
> 
> 
> On 10/09/2019 05:46, Stefan Liebler wrote:
>> On 9/6/19 5:21 AM, Carlos O'Donell wrote:
>>> On 9/3/19 9:34 AM, Adhemerval Zanella wrote:
>>>> Yes, my initial suggestion was just to make it as UNSUPPORTED for
>>>> ptrace_scope >= 1. But I do not oppose adjusting it to run on
>>>> ptrace_scope 1, it is just that the required hackery lead to make it
>>>> somewhat as complex than the test itself.
>>>
>>> The flip side of the coin is that the more "UNSUPPORTED" results we
>>> add *implies* there is "one valid way" to setup a glibc test run
>>> and we don't clearly document how to turn all the "UNSUPPORTED"
>>> entries into supported tests?
>>>
>>> Stefan's code can at least be refactored into support/ if we need
>>> to do the same thing again in another test.
>>>
>>
>> PING.
>>
>> As I have already posted multiple versions of the patch, how to proceed?
>> 1) UNSUPPORTED if support_ptrace_scope() >= 2;
>> Support support_ptrace_scope() == 1
>> by adjusting the process tree;
>> (see https://www.sourceware.org/ml/libc-alpha/2019-09/msg00024.html)
>>
>> 2) UNSUPPORTED if support_ptrace_scope() >= 2;
>> Support support_ptrace_scope() == 1
>> by calling support_ptrace_process_set_ptracer_any();
>> (see https://www.sourceware.org/ml/libc-alpha/2019-08/msg00722.html)
>>
>> 3) UNSUPPORTED if support_ptrace_scope() != 0
>> (patch would use support_ptrace_scope() of one of the patches above in order to trigger FAIL_UNSUPPORTED)
> 
> My view is although 2) is way complex that I would like, I think it should
> the more complete solution. Does still need review or is it ready to land?
> 
You've already reviewed the support part on a previous version of "patch 
2)" (the support part was not changed in the latest version; see 
https://www.sourceware.org/ml/libc-alpha/2019-08/msg00703.html).

But it needs review for the synchronization part between the 
target_process and do_test in tst-pldd.c.

On 02/09/2019 12:28, Stefan Liebler wrote:
> On 8/29/19 10:47 AM, Florian Weimer wrote:
>> * Stefan Liebler:
>>
>>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>>> * Stefan Liebler:
>>>>
>>>>>    static void
>>>>>    target_process (void *arg)
>>>>>    {
>>>>> +  if (ptrace_scope == 1)
>>>>> +    {
>>>>> +      /* YAMA is configured to "restricted ptrace".
>>>>> +     Disable the restriction for this subprocess.  */
>>>>> +      support_ptrace_process_set_ptracer_any ();
>>>>> +    }
>>>>> +
>>>>>      pause ();
>>>>>    }
>>>>
>>>> I think this has a race condition if pldd attaches to the process before
>>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>>> hard it is in practice to hit this race.  It should be possible to use a
>>>> process-shared barrier or some other form of synchronization to avoid
>>>> this issue.
>>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>
>>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>>> I've not used pthread* functions as I don't want to link against
>>> libpthread.so. Then further adjustments are needed.
>>>
>>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>>> proposed in his post?
>>
>> Is it possible to create a process tree like this?
>>
>>
>>    parent (performs output checks)
>>      subprocess 1 (becomes pldd via execve)
>>        subprocess 2
>>
>> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
>> ptrace scope for ptrace_scope < 2?
> Yes, this is possible.
> I've rearranged the subprocesses. See attached patch.
> Now we have a new function pldd_process which forks target_process,
> stores the pid of target_prcess to a shared memory mapping as do_test needs to know this pid.
> 
> Afterwards it execve to pldd which successfully ptrace target_process in case of "restricted ptrace".
> 
> Please review the usage of support-subprocess-functions.
> 
> Bye,
> Stefan
>>
>> Thanks,
>> Florian
>>
> 
> 
> 20190902_tst-pldd.patch
> 
> commit ad51263d51d12ce6ca2ce9304efe5ba05b3912b1
> Author: Stefan Liebler <stli@linux.ibm.com>
> Date:   Mon Aug 26 15:45:07 2019 +0200
> 
>     Add UNSUPPORTED check in elf/tst-pldd.
>     
>     The testcase forks a child process and runs pldd with PID of
>     this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
>     differs from zero, pldd will fail with
>     /usr/bin/pldd: cannot attach to process 3: Operation not permitted
>     
>     This patch checks if ptrace_scope exists, is zero "classic ptrace permissions"
>     or one "restricted ptrace".  If ptrace_scope exists and has a higher
>     restriction, then the test is marked as UNSUPPORTED.
>     
>     The case "restricted ptrace" is handled by rearranging the processes involved
>     during the test.  Now we have the following process tree:
>     -parent: do_test (performs output checks)
>     --subprocess 1: pldd_process (becomes pldd via execve)
>     ---subprocess 2: target_process (ptraced via pldd)
>     
>     ChangeLog:
>     
>             * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
>             Rearrange subprocesses.
>             (pldd_process): New function.
>             * support/Makefile (libsupport-routines): Add support_ptrace.
>             * support/ptrace.h: New file.
>             * support/support_ptrace.c: Likewise.

LGTM with just a change below, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
> index 6b7c94a1c0..eb1b9cb5f5 100644
> --- a/elf/tst-pldd.c
> +++ b/elf/tst-pldd.c
> @@ -30,6 +30,12 @@
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
>  #include <support/support.h>
> +#include <support/ptrace.h>
> +#include <support/xunistd.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +#include <signal.h>
> +
>  
>  static void
>  target_process (void *arg)
> @@ -37,6 +43,34 @@ target_process (void *arg)
>    pause ();
>  }
>  
> +static void
> +pldd_process (void *arg)
> +{
> +  pid_t *target_pid_ptr = (pid_t *) arg;
> +
> +  /* Create a copy of current test to check with pldd.  As the
> +     target_process is a child of this pldd_process, pldd is also able
> +     to attach to target_process if YAMA is configured to 1 =
> +     "restricted ptrace".  */
> +  struct support_subprocess target = support_subprocess (target_process, NULL);
> +
> +  /* Store the pid of target-process as do_test needs it in order to
> +     e.g. terminate it at end of the test.  */
> +  *target_pid_ptr = target.pid;
> +
> +  /* Three digits per byte plus null terminator.  */
> +  char pid[3 * sizeof (uint32_t) + 1];
> +  snprintf (pid, array_length (pid), "%d", target.pid);
> +
> +  char *prog = xasprintf ("%s/pldd", support_bindir_prefix);
> +
> +  /* Run pldd and use the pid of target_process as argument.  */
> +  execve (prog, (char *const []) { (char *) prog, pid, NULL },
> +	  (char *const []) { NULL });
> +
> +  FAIL_EXIT1 ("Returned from execve: errno=%d=%m\n", errno);
> +}
> +

Ok.

>  /* The test runs in a container because pldd does not support tracing
>     a binary started by the loader iself (as with testrun.sh).  */
>  
> @@ -52,25 +86,22 @@ in_str_list (const char *libname, const char *const strlist[])
>  static int
>  do_test (void)
>  {
> -  /* Create a copy of current test to check with pldd.  */
> -  struct support_subprocess target = support_subprocess (target_process, NULL);
> -
> -  /* Run 'pldd' on test subprocess.  */
> -  struct support_capture_subprocess pldd;
> +  /* Check if our subprocess can be debugged with ptrace.  */
>    {
> -    /* Three digits per byte plus null terminator.  */
> -    char pid[3 * sizeof (uint32_t) + 1];
> -    snprintf (pid, array_length (pid), "%d", target.pid);
> -
> -    char *prog = xasprintf ("%s/pldd", support_bindir_prefix);
> -
> -    pldd = support_capture_subprogram (prog,
> -      (char *const []) { (char *) prog, pid, NULL });
> +    int ptrace_scope = support_ptrace_scope ();
> +    if (ptrace_scope >= 2)
> +      FAIL_UNSUPPORTED ("/proc/sys/kernel/yama/ptrace_scope >= 2");
> +  }
>  

Ok.

> -    support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout);
> +  pid_t *target_pid_ptr = (pid_t *) xmmap (NULL, sizeof (pid_t),
> +					   PROT_READ | PROT_WRITE,
> +					   MAP_SHARED | MAP_ANONYMOUS, -1);
>  
> -    free (prog);
> -  }

Ok.

> +  /* Run 'pldd' on test subprocess which will be created in pldd_process.
> +     The pid of the subprocess will be written to target_pid_ptr.  */
> +  struct support_capture_subprocess pldd;
> +  pldd = support_capture_subprocess (pldd_process, target_pid_ptr);
> +  support_capture_subprocess_check (&pldd, "pldd", 0, sc_allow_stdout);

Ok, this is will updated by the pldd_process.

>  
>    /* Check 'pldd' output.  The test is expected to be linked against only
>       loader and libc.  */
> @@ -85,7 +116,7 @@ do_test (void)
>      /* First line is in the form of <pid>: <full path of executable>  */
>      TEST_COMPARE (fscanf (out, "%u: " STRINPUT (512), &pid, buffer), 2);
>  
> -    TEST_COMPARE (pid, target.pid);
> +    TEST_COMPARE (pid, *target_pid_ptr);
>      TEST_COMPARE (strcmp (basename (buffer), "tst-pldd"), 0);
>  
>      /* It expects only one loader and libc loaded by the program.  */
> @@ -93,7 +124,7 @@ do_test (void)
>      while (fgets (buffer, array_length (buffer), out) != NULL)
>        {
>  	/* Ignore vDSO.  */
> -        if (buffer[0] != '/')
> +	if (buffer[0] != '/')
>  	  continue;
>  

Ok.

>  	/* Remove newline so baseline (buffer) can compare against the
> @@ -128,7 +159,9 @@ do_test (void)
>    }
>  
>    support_capture_subprocess_free (&pldd);
> -  support_process_terminate (&target);
> +  if (kill (*target_pid_ptr, SIGKILL) != 0)
> +    FAIL_EXIT1 ("Unable to kill target_process: errno=%d=%m\n", errno);
> +  xmunmap (target_pid_ptr, sizeof (pid_t));
>  
>    return 0;
>  }

Ok.

> diff --git a/support/Makefile b/support/Makefile
> index ab66913a02..34d14eb1bb 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -56,6 +56,7 @@ libsupport-routines = \
>    support_format_hostent \
>    support_format_netent \
>    support_isolate_in_subprocess \
> +  support_ptrace \
>    support_openpty \
>    support_paths \
>    support_quote_blob \

Ok.

> diff --git a/support/ptrace.h b/support/ptrace.h
> new file mode 100644
> index 0000000000..90006a6b75
> --- /dev/null
> +++ b/support/ptrace.h
> @@ -0,0 +1,32 @@
> +/* Support functions handling ptrace_scope.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_PTRACE_H
> +#define SUPPORT_PTRACE_H
> +
> +#include <sys/cdefs.h>
> +
> +__BEGIN_DECLS
> +
> +/* Return the current YAMA mode set on the machine (0 to 3) or -1
> +   if YAMA is not supported.  */
> +int support_ptrace_scope (void);
> +
> +__END_DECLS
> +
> +#endif

I think it should named xptrace.h.

> diff --git a/support/support_ptrace.c b/support/support_ptrace.c
> new file mode 100644
> index 0000000000..2084e5a189
> --- /dev/null
> +++ b/support/support_ptrace.c
> @@ -0,0 +1,44 @@
> +/* Support functions handling ptrace_scope.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/ptrace.h>
> +#include <sys/prctl.h>
> +
> +int
> +support_ptrace_scope (void)
> +{
> +  int ptrace_scope = -1;
> +
> +#ifdef __linux__
> +  /* YAMA may be not enabled.  Otherwise it contains a value from 0 to 3:
> +     - 0 classic ptrace permissions
> +     - 1 restricted ptrace
> +     - 2 admin-only attach
> +     - 3 no attach  */
> +  FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
> +  if (f != NULL)
> +    {
> +      TEST_COMPARE (fscanf (f, "%d", &ptrace_scope), 1);
> +      xfclose (f);
> +    }
> +#endif
> +
> +  return ptrace_scope;
> +}
> 

Ok.

On 9/17/19 3:31 PM, Adhemerval Zanella wrote:
> 
> 
> On 02/09/2019 12:28, Stefan Liebler wrote:
>> On 8/29/19 10:47 AM, Florian Weimer wrote:
>>> * Stefan Liebler:
>>>
>>>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>>>> * Stefan Liebler:
>>>>>
>>>>>>     static void
>>>>>>     target_process (void *arg)
>>>>>>     {
>>>>>> +  if (ptrace_scope == 1)
>>>>>> +    {
>>>>>> +      /* YAMA is configured to "restricted ptrace".
>>>>>> +     Disable the restriction for this subprocess.  */
>>>>>> +      support_ptrace_process_set_ptracer_any ();
>>>>>> +    }
>>>>>> +
>>>>>>       pause ();
>>>>>>     }
>>>>>
>>>>> I think this has a race condition if pldd attaches to the process before
>>>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>>>> hard it is in practice to hit this race.  It should be possible to use a
>>>>> process-shared barrier or some other form of synchronization to avoid
>>>>> this issue.
>>>>>
>>>>> Thanks,
>>>>> Florian
>>>>>
>>>>
>>>> I've added a synchronization with stdatomic.h on a shared memory mapping.
>>>> I've not used pthread* functions as I don't want to link against
>>>> libpthread.so. Then further adjustments are needed.
>>>>
>>>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>>>> proposed in his post?
>>>
>>> Is it possible to create a process tree like this?
>>>
>>>
>>>     parent (performs output checks)
>>>       subprocess 1 (becomes pldd via execve)
>>>         subprocess 2
>>>
>>> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
>>> ptrace scope for ptrace_scope < 2?
>> Yes, this is possible.
>> I've rearranged the subprocesses. See attached patch.
>> Now we have a new function pldd_process which forks target_process,
>> stores the pid of target_prcess to a shared memory mapping as do_test needs to know this pid.
>>
>> Afterwards it execve to pldd which successfully ptrace target_process in case of "restricted ptrace".
>>
>> Please review the usage of support-subprocess-functions.
>>
>> Bye,
>> Stefan
>>>
>>> Thanks,
>>> Florian
>>>
>>
>>
>> 20190902_tst-pldd.patch
>>
>> commit ad51263d51d12ce6ca2ce9304efe5ba05b3912b1
>> Author: Stefan Liebler <stli@linux.ibm.com>
>> Date:   Mon Aug 26 15:45:07 2019 +0200
>>
>>      Add UNSUPPORTED check in elf/tst-pldd.
>>      
>>      The testcase forks a child process and runs pldd with PID of
>>      this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
>>      differs from zero, pldd will fail with
>>      /usr/bin/pldd: cannot attach to process 3: Operation not permitted
>>      
>>      This patch checks if ptrace_scope exists, is zero "classic ptrace permissions"
>>      or one "restricted ptrace".  If ptrace_scope exists and has a higher
>>      restriction, then the test is marked as UNSUPPORTED.
>>      
>>      The case "restricted ptrace" is handled by rearranging the processes involved
>>      during the test.  Now we have the following process tree:
>>      -parent: do_test (performs output checks)
>>      --subprocess 1: pldd_process (becomes pldd via execve)
>>      ---subprocess 2: target_process (ptraced via pldd)
>>      
>>      ChangeLog:
>>      
>>              * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
>>              Rearrange subprocesses.
>>              (pldd_process): New function.
>>              * support/Makefile (libsupport-routines): Add support_ptrace.
>>              * support/ptrace.h: New file.
>>              * support/support_ptrace.c: Likewise.
> 
> LGTM with just a change below, thanks.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
...
>> diff --git a/support/ptrace.h b/support/ptrace.h
>> new file mode 100644
>> index 0000000000..90006a6b75
>> --- /dev/null
>> +++ b/support/ptrace.h
>> @@ -0,0 +1,32 @@
>> +/* Support functions handling ptrace_scope.
>> +   Copyright (C) 2019 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef SUPPORT_PTRACE_H
>> +#define SUPPORT_PTRACE_H
>> +
>> +#include <sys/cdefs.h>
>> +
>> +__BEGIN_DECLS
>> +
>> +/* Return the current YAMA mode set on the machine (0 to 3) or -1
>> +   if YAMA is not supported.  */
>> +int support_ptrace_scope (void);
>> +
>> +__END_DECLS
>> +
>> +#endif
> 
> I think it should named xptrace.h.
> 
...
Okay. Then I will rename it to support/xptrace.h and if nobody opposes, 
I'll commit it tomorrow.

Thanks for the review.
Stefan

On 9/17/19 5:17 PM, Stefan Liebler wrote:
> On 9/17/19 3:31 PM, Adhemerval Zanella wrote:
>>
>>
>> On 02/09/2019 12:28, Stefan Liebler wrote:
>>> On 8/29/19 10:47 AM, Florian Weimer wrote:
>>>> * Stefan Liebler:
>>>>
>>>>> On 8/28/19 11:24 AM, Florian Weimer wrote:
>>>>>> * Stefan Liebler:
>>>>>>
>>>>>>>     static void
>>>>>>>     target_process (void *arg)
>>>>>>>     {
>>>>>>> +  if (ptrace_scope == 1)
>>>>>>> +    {
>>>>>>> +      /* YAMA is configured to "restricted ptrace".
>>>>>>> +     Disable the restriction for this subprocess.  */
>>>>>>> +      support_ptrace_process_set_ptracer_any ();
>>>>>>> +    }
>>>>>>> +
>>>>>>>       pause ();
>>>>>>>     }
>>>>>>
>>>>>> I think this has a race condition if pldd attaches to the process 
>>>>>> before
>>>>>> the support_ptrace_process_set_ptracer_any call.  I have no idea how
>>>>>> hard it is in practice to hit this race.  It should be possible to 
>>>>>> use a
>>>>>> process-shared barrier or some other form of synchronization to avoid
>>>>>> this issue.
>>>>>>
>>>>>> Thanks,
>>>>>> Florian
>>>>>>
>>>>>
>>>>> I've added a synchronization with stdatomic.h on a shared memory 
>>>>> mapping.
>>>>> I've not used pthread* functions as I don't want to link against
>>>>> libpthread.so. Then further adjustments are needed.
>>>>>
>>>>> Or should I just restrict the test ptrace_scope 0 as Adhemerval has
>>>>> proposed in his post?
>>>>
>>>> Is it possible to create a process tree like this?
>>>>
>>>>
>>>>     parent (performs output checks)
>>>>       subprocess 1 (becomes pldd via execve)
>>>>         subprocess 2
>>>>
>>>> If you execve pldd from subprocess 1, wouldn't subprocess 2 in its
>>>> ptrace scope for ptrace_scope < 2?
>>> Yes, this is possible.
>>> I've rearranged the subprocesses. See attached patch.
>>> Now we have a new function pldd_process which forks target_process,
>>> stores the pid of target_prcess to a shared memory mapping as do_test 
>>> needs to know this pid.
>>>
>>> Afterwards it execve to pldd which successfully ptrace target_process 
>>> in case of "restricted ptrace".
>>>
>>> Please review the usage of support-subprocess-functions.
>>>
>>> Bye,
>>> Stefan
>>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>
>>>
>>> 20190902_tst-pldd.patch
>>>
>>> commit ad51263d51d12ce6ca2ce9304efe5ba05b3912b1
>>> Author: Stefan Liebler <stli@linux.ibm.com>
>>> Date:   Mon Aug 26 15:45:07 2019 +0200
>>>
>>>      Add UNSUPPORTED check in elf/tst-pldd.
>>>      The testcase forks a child process and runs pldd with PID of
>>>      this child.  On systems where /proc/sys/kernel/yama/ptrace_scope
>>>      differs from zero, pldd will fail with
>>>      /usr/bin/pldd: cannot attach to process 3: Operation not permitted
>>>      This patch checks if ptrace_scope exists, is zero "classic 
>>> ptrace permissions"
>>>      or one "restricted ptrace".  If ptrace_scope exists and has a 
>>> higher
>>>      restriction, then the test is marked as UNSUPPORTED.
>>>      The case "restricted ptrace" is handled by rearranging the 
>>> processes involved
>>>      during the test.  Now we have the following process tree:
>>>      -parent: do_test (performs output checks)
>>>      --subprocess 1: pldd_process (becomes pldd via execve)
>>>      ---subprocess 2: target_process (ptraced via pldd)
>>>      ChangeLog:
>>>              * elf/tst-pldd.c (do_test): Add UNSUPPORTED check.
>>>              Rearrange subprocesses.
>>>              (pldd_process): New function.
>>>              * support/Makefile (libsupport-routines): Add 
>>> support_ptrace.
>>>              * support/ptrace.h: New file.
>>>              * support/support_ptrace.c: Likewise.
>>
>> LGTM with just a change below, thanks.
>>
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
> ...
>>> diff --git a/support/ptrace.h b/support/ptrace.h
>>> new file mode 100644
>>> index 0000000000..90006a6b75
>>> --- /dev/null
>>> +++ b/support/ptrace.h
>>> @@ -0,0 +1,32 @@
>>> +/* Support functions handling ptrace_scope.
>>> +   Copyright (C) 2019 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <http://www.gnu.org/licenses/>.  */
>>> +
>>> +#ifndef SUPPORT_PTRACE_H
>>> +#define SUPPORT_PTRACE_H
>>> +
>>> +#include <sys/cdefs.h>
>>> +
>>> +__BEGIN_DECLS
>>> +
>>> +/* Return the current YAMA mode set on the machine (0 to 3) or -1
>>> +   if YAMA is not supported.  */
>>> +int support_ptrace_scope (void);
>>> +
>>> +__END_DECLS
>>> +
>>> +#endif
>>
>> I think it should named xptrace.h.
>>
> ...
> Okay. Then I will rename it to support/xptrace.h and if nobody opposes, 
> I'll commit it tomorrow.
> 
> Thanks for the review.
> Stefan
> 
Committed with xptrace.h instead of ptrace.h.

This has broken the build for i686-gnu.

In file included from support_ptrace.c:22:
../include/sys/prctl.h:2:15: fatal error: sys/prctl.h: No such file or directory
 #include_next <sys/prctl.h>
               ^~~~~~~~~~~~~

On 9/18/19 5:17 PM, Joseph Myers wrote:
> This has broken the build for i686-gnu.
> 
> In file included from support_ptrace.c:22:
> ../include/sys/prctl.h:2:15: fatal error: sys/prctl.h: No such file or directory
>   #include_next <sys/prctl.h>
>                 ^~~~~~~~~~~~~
> 

Sorry for that.
This include of sys/prctl.h is a leftover from a previous patch version.
I've just committed the attached patch which just removes it.

Thanks.
Stefan