Re: [PATCH] Add UNSUPPORTED check in elf/tst-pldd.

[Stefan Liebler <stli at linux dot ibm dot com>]

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