public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* Caching of PID/TID after fork
@ 2016-10-06 16:13 Robert Święcki
  2016-10-06 16:34 ` Paul Pluzhnikov
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Robert Święcki @ 2016-10-06 16:13 UTC (permalink / raw)
  To: libc-alpha

Hi,

This has probably been debated a lot of times in the past, but...

A lot of tool these days create Linux namespaces, using clone(). Using
glibc's clone() is not the best option, because e.g. it requires stack
pointer, (and fails with some error if it's NULL), while Linux kernel
will happily reuse the current SP (no need to write additional code to
deal with that). So, glibc's clone() adds imposed additional
restrictions wrt kernel.

And, after that getpid() will return old PID/TID.

It seems that currently Linux supports getpid() via vsyscall (fast) on
x86/64. Would you consider changing getpid() to be a wrapper to
syscall(__NR_getpid)?

One could say that if somebody uses syscall(__NR_clone) she/he should
be using syscall(__NR_getpid) as well, but in case of software
libraries, which might be preparing namespaces, the caller of the lib
might know nothing about the exact method used to create a new
process.

Thefore I'd like to ask for one of the following solutions:

1. Don't cache PID/TID

2. Provide some kind of symbol, which would force for TID/PID to be
reloaded in glibc.

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 16:13 Caching of PID/TID after fork Robert Święcki
@ 2016-10-06 16:34 ` Paul Pluzhnikov
  2016-10-06 17:03   ` Robert Święcki
  2016-10-06 17:26 ` Rich Felker
  2016-10-07 19:38 ` Florian Weimer
  2 siblings, 1 reply; 22+ messages in thread
From: Paul Pluzhnikov @ 2016-10-06 16:34 UTC (permalink / raw)
  To: Robert Święcki; +Cc: GLIBC Devel

On Thu, Oct 6, 2016 at 9:13 AM, Robert Święcki <robert@swiecki.net> wrote:

> This has probably been debated a lot of times in the past, but...

We should probably mention that caching of PID has caused bugs in the past, e.g.
https://sourceware.org/bugzilla/show_bug.cgi?id=19957
https://sourceware.org/ml/libc-alpha/2006-07/msg00123.html

There is also
http://www.eglibc.org/archives/issues/msg00061.html
http://yarchive.net/comp/linux/getpid_caching.html

-- 
Paul Pluzhnikov

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 16:34 ` Paul Pluzhnikov
@ 2016-10-06 17:03   ` Robert Święcki
  2016-10-06 18:32     ` Adhemerval Zanella
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-06 17:03 UTC (permalink / raw)
  To: Paul Pluzhnikov; +Cc: GLIBC Devel

3. Provide clone() wrapper, which doesn't impose additional
restrictions over kernel's __NR_clone and which updates the TID/PID
cache.

2016-10-06 18:33 GMT+02:00 Paul Pluzhnikov <ppluzhnikov@google.com>:
> On Thu, Oct 6, 2016 at 9:13 AM, Robert Święcki <robert@swiecki.net> wrote:
>
>> This has probably been debated a lot of times in the past, but...
>
> We should probably mention that caching of PID has caused bugs in the past, e.g.
> https://sourceware.org/bugzilla/show_bug.cgi?id=19957
> https://sourceware.org/ml/libc-alpha/2006-07/msg00123.html
>
> There is also
> http://www.eglibc.org/archives/issues/msg00061.html
> http://yarchive.net/comp/linux/getpid_caching.html
>
> --
> Paul Pluzhnikov



-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 16:13 Caching of PID/TID after fork Robert Święcki
  2016-10-06 16:34 ` Paul Pluzhnikov
@ 2016-10-06 17:26 ` Rich Felker
  2016-10-06 17:42   ` Robert Święcki
  2016-10-07 19:38 ` Florian Weimer
  2 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2016-10-06 17:26 UTC (permalink / raw)
  To: Robert Święcki; +Cc: libc-alpha

On Thu, Oct 06, 2016 at 06:13:41PM +0200, Robert Święcki wrote:
> Hi,
> 
> This has probably been debated a lot of times in the past, but...
> 
> A lot of tool these days create Linux namespaces, using clone(). Using
> glibc's clone() is not the best option, because e.g. it requires stack
> pointer, (and fails with some error if it's NULL), while Linux kernel
> will happily reuse the current SP (no need to write additional code to
> deal with that). So, glibc's clone() adds imposed additional
> restrictions wrt kernel.
> 
> And, after that getpid() will return old PID/TID.
> 
> It seems that currently Linux supports getpid() via vsyscall (fast) on
> x86/64. Would you consider changing getpid() to be a wrapper to
> syscall(__NR_getpid)?
> 
> One could say that if somebody uses syscall(__NR_clone) she/he should
> be using syscall(__NR_getpid) as well, but in case of software
> libraries, which might be preparing namespaces, the caller of the lib
> might know nothing about the exact method used to create a new
> process.
> 
> Thefore I'd like to ask for one of the following solutions:
> 
> 1. Don't cache PID/TID
> 
> 2. Provide some kind of symbol, which would force for TID/PID to be
> reloaded in glibc.

There's an easy solution that works with existing versions of glibc
(and other libcs) with no new symbol or new symbol version dependency:
call the libc clone() function with a tiny dummy stack and a function
which does nothing but longjmp out to the caller.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 17:26 ` Rich Felker
@ 2016-10-06 17:42   ` Robert Święcki
  2016-10-06 18:05     ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-06 17:42 UTC (permalink / raw)
  To: Rich Felker; +Cc: GLIBC Devel

2016-10-06 19:26 GMT+02:00 Rich Felker <dalias@libc.org>:

>> 2. Provide some kind of symbol, which would force for TID/PID to be
>> reloaded in glibc.
>
> There's an easy solution that works with existing versions of glibc
> (and other libcs) with no new symbol or new symbol version dependency:
> call the libc clone() function with a tiny dummy stack and a function
> which does nothing but longjmp out to the caller.

Thanks Rich, that's an interesting idea. I might use it.


Though, IMO, the problem still exist. Some subset of non-trivial
projects which are affected by this behavior:

AFL - https://lcamtuf.blogspot.com/2014/10/fuzzing-binaries-without-execve.html
("because the library caches the result of getpid() when initializing
- and without a way to make it reconsider, PID-dependent calls such as
abort() or raise() will go astray. There is also a library wrapper for
the clone() call that does update the cached PID - but the wrapper is
unwieldy and insists on messing with the process' stack.")

LXC/Docker - https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-August/009920.html

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 17:42   ` Robert Święcki
@ 2016-10-06 18:05     ` Rich Felker
  2016-10-06 18:26       ` Robert Święcki
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2016-10-06 18:05 UTC (permalink / raw)
  To: Robert Święcki; +Cc: GLIBC Devel

On Thu, Oct 06, 2016 at 07:42:25PM +0200, Robert Święcki wrote:
> 2016-10-06 19:26 GMT+02:00 Rich Felker <dalias@libc.org>:
> 
> >> 2. Provide some kind of symbol, which would force for TID/PID to be
> >> reloaded in glibc.
> >
> > There's an easy solution that works with existing versions of glibc
> > (and other libcs) with no new symbol or new symbol version dependency:
> > call the libc clone() function with a tiny dummy stack and a function
> > which does nothing but longjmp out to the caller.
> 
> Thanks Rich, that's an interesting idea. I might use it.
> 
> 
> Though, IMO, the problem still exist. Some subset of non-trivial
> projects which are affected by this behavior:
> 
> AFL - https://lcamtuf.blogspot.com/2014/10/fuzzing-binaries-without-execve.html
> ("because the library caches the result of getpid() when initializing
> - and without a way to make it reconsider, PID-dependent calls such as
> abort() or raise() will go astray. There is also a library wrapper for
> the clone() call that does update the cached PID - but the wrapper is
> unwieldy and insists on messing with the process' stack.")
> 
> LXC/Docker - https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-August/009920.html

Why can't they use the approach I just described?

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 18:05     ` Rich Felker
@ 2016-10-06 18:26       ` Robert Święcki
  2016-10-06 21:35         ` Robert Święcki
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-06 18:26 UTC (permalink / raw)
  To: Rich Felker; +Cc: GLIBC Devel

>> Thanks Rich, that's an interesting idea. I might use it.
>>
>>
>> Though, IMO, the problem still exist. Some subset of non-trivial
>> projects which are affected by this behavior:
>>
>> AFL - https://lcamtuf.blogspot.com/2014/10/fuzzing-binaries-without-execve.html
>> ("because the library caches the result of getpid() when initializing
>> - and without a way to make it reconsider, PID-dependent calls such as
>> abort() or raise() will go astray. There is also a library wrapper for
>> the clone() call that does update the cached PID - but the wrapper is
>> unwieldy and insists on messing with the process' stack.")
>>
>> LXC/Docker - https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-August/009920.html
>
> Why can't they use the approach I just described?

Admittedly, they probably could... it's just the clone interface is
probably supposed to be used by projects like that (non trivial ones,
depending deeply on kernel interfaces), and it seems that the authors
of those non-trivial tools (possibly, quite experienced in the area of
computer architectures) are somewhat troubled by the current glibc
behavior.

Therefore, maybe there's some opportunity here for glibc to provide
them with an interface which might better suit their needs. In my
opinion, the path that every future developer dealing with this issue
have to take (from analyzing the kernel's __NR_clone implementation to
by-passing glibc wrapper with setjmp/longjmp) might be a bit more
complex than it should.

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 17:03   ` Robert Święcki
@ 2016-10-06 18:32     ` Adhemerval Zanella
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2016-10-06 18:32 UTC (permalink / raw)
  To: libc-alpha

Besides the clone(CLONE_VM) issues, another one related to pid/tid
caching was BZ#15368 [1].

The problem of providing a clone syscal that do not cache pid/tid
is current mutex, rwlocks, and some other implementations
(pthread_clock_gettime, etc.) relies on correct pid/tid information.
So if it is a potential source of issues if the exported clone
do not provide the correct semantics.  

Also, providing a glibc own symbol to update the tid/pid is messy:
it would be an internal implementation detail which in theory 
application should not have access to, it might add some 
synchronization issues (what happens if you try to force update 
after a mutex operation with default wrong value?), and it also 
might be tricky to add a compatibility symbol in case of change 
how tid/pid internally works.

Another possible solution was to get rid of the caching entirely,
but it might incur in some performance issues (each tid/pid get 
will trigger another syscall).  We can minimize the performance
issue by adding a vdso implementation of getpid/gettid, but some
architectures and configuration will still not benefit from it
(also, afaik current x86 kernel do not provide getpid/gettid
as vsyscall neither as vdso and recall that for glibc we
deprecated vsyscall use).

Another possibility is to use another thread unique identifier 
as the owner instead of tid (maybe a hash of pthread_t
to fit on a int).  By removing this requirement I think it is
feasible to get rid of caching.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=15368

On 06/10/2016 14:03, Robert Święcki wrote:
> 3. Provide clone() wrapper, which doesn't impose additional
> restrictions over kernel's __NR_clone and which updates the TID/PID
> cache.
> 
> 2016-10-06 18:33 GMT+02:00 Paul Pluzhnikov <ppluzhnikov@google.com>:
>> On Thu, Oct 6, 2016 at 9:13 AM, Robert Święcki <robert@swiecki.net> wrote:
>>
>>> This has probably been debated a lot of times in the past, but...
>>
>> We should probably mention that caching of PID has caused bugs in the past, e.g.
>> https://sourceware.org/bugzilla/show_bug.cgi?id=19957
>> https://sourceware.org/ml/libc-alpha/2006-07/msg00123.html
>>
>> There is also
>> http://www.eglibc.org/archives/issues/msg00061.html
>> http://yarchive.net/comp/linux/getpid_caching.html
>>
>> --
>> Paul Pluzhnikov
> 
> 
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 18:26       ` Robert Święcki
@ 2016-10-06 21:35         ` Robert Święcki
  2016-10-07  0:42           ` Zack Weinberg
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-06 21:35 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: GLIBC Devel

> Also, providing a glibc own symbol to update the tid/pid is messy:
> it would be an internal implementation detail which in theory
> application should not have access to, it might add some
> synchronization issues (what happens if you try to force update
> after a mutex operation with default wrong value?), and it also
> might be tricky to add a compatibility symbol in case of change
> how tid/pid internally works.

Thanks, this is good analysis.

> Another possibility is to use another thread unique identifier
> as the owner instead of tid (maybe a hash of pthread_t
> to fit on a int).  By removing this requirement I think it is
> feasible to get rid of caching.

Another dev told me that chromium developers implemented ForkWithFlags
- https://codereview.chromium.org/800183004/ - to deal exactly with
the problem is caching PID/TID. They used the setjmp/longjmp approach
suggested by Rich.

My point is that a lot of projects which use the clone syscall will be
hit by this problem, and getting to (or finding in glibc mail
archives) the working solution might take a lot of time. It's because
of the rise of namespaces, more and more projects will be using the
syscall for various forms of containerization.

Maybe providing something like fork_with_flags(int flags) would work
here? Seems that most (all?) the projects that I've seen, which use
clone() directly and had problems with caching of TID/PID, basically
need fork with some flags (mainly of the CLONE_NEW* type). Such
function would deal with tid/gid/locks/pthread_atfork as fork does,
but would also OR the provided flags with the canonical ones?

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 21:35         ` Robert Święcki
@ 2016-10-07  0:42           ` Zack Weinberg
  2016-10-07  0:43             ` Zack Weinberg
  0 siblings, 1 reply; 22+ messages in thread
From: Zack Weinberg @ 2016-10-07  0:42 UTC (permalink / raw)
  To: Robert Święcki; +Cc: Adhemerval Zanella, GLIBC Devel

On Thu, Oct 6, 2016 at 5:35 PM, Robert Święcki <robert@swiecki.net> wrote:
>
> Maybe providing something like fork_with_flags(int flags) would work
> here? Seems that most (all?) the projects that I've seen, which use
> clone() directly and had problems with caching of TID/PID, basically
> need fork with some flags (mainly of the CLONE_NEW* type). Such
> function would deal with tid/gid/locks/pthread_atfork as fork does,
> but would also OR the provided flags with the canonical ones?

I've thought for quite some time that there should be a second
clone-wrapper in glibc that doesn't ask for a new stack.  It might be
better to call it something with "clone" in the name, but I don't care
terribly much.

zw

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07  0:42           ` Zack Weinberg
@ 2016-10-07  0:43             ` Zack Weinberg
  2016-10-07 14:44               ` Robert Święcki
  2016-10-07 18:30               ` Adhemerval Zanella
  0 siblings, 2 replies; 22+ messages in thread
From: Zack Weinberg @ 2016-10-07  0:43 UTC (permalink / raw)
  To: Robert Święcki; +Cc: Adhemerval Zanella, GLIBC Devel

On Thu, Oct 6, 2016 at 8:42 PM, Zack Weinberg <zackw@panix.com> wrote:
> On Thu, Oct 6, 2016 at 5:35 PM, Robert Święcki <robert@swiecki.net> wrote:
>> Maybe providing something like fork_with_flags(int flags) would work
>> here? Seems that most (all?) the projects that I've seen, which use
>> clone() directly and had problems with caching of TID/PID, basically
>> need fork with some flags (mainly of the CLONE_NEW* type). Such
>> function would deal with tid/gid/locks/pthread_atfork as fork does,
>> but would also OR the provided flags with the canonical ones?
>
> I've thought for quite some time that there should be a second
> clone-wrapper in glibc that doesn't ask for a new stack.  It might be
> better to call it something with "clone" in the name, but I don't care
> terribly much.

Meant to say: in addition to the PID/TID caching issues, this would be
able to run pthread_atfork() handlers.

zw

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07  0:43             ` Zack Weinberg
@ 2016-10-07 14:44               ` Robert Święcki
  2016-10-07 18:20                 ` Adhemerval Zanella
  2016-10-07 18:30               ` Adhemerval Zanella
  1 sibling, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-07 14:44 UTC (permalink / raw)
  To: Zack Weinberg; +Cc: Adhemerval Zanella, GLIBC Devel

2016-10-07 2:43 GMT+02:00 Zack Weinberg <zackw@panix.com>:

>> I've thought for quite some time that there should be a second
>> clone-wrapper in glibc that doesn't ask for a new stack.  It might be
>> better to call it something with "clone" in the name, but I don't care
>> terribly much.
>
> Meant to say: in addition to the PID/TID caching issues, this would be
> able to run pthread_atfork() handlers.

Maybe..

 __clone3(unsigned long clone_flags, void* newsp, pid_t*
parent_tidptr, pid_t* child_tidptr, void* tls);

?

__clone2 seems to be used with ia64 already.

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07 14:44               ` Robert Święcki
@ 2016-10-07 18:20                 ` Adhemerval Zanella
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2016-10-07 18:20 UTC (permalink / raw)
  To: Robert Święcki, Zack Weinberg; +Cc: GLIBC Devel



On 07/10/2016 11:44, Robert Święcki wrote:
> 2016-10-07 2:43 GMT+02:00 Zack Weinberg <zackw@panix.com>:
> 
>>> I've thought for quite some time that there should be a second
>>> clone-wrapper in glibc that doesn't ask for a new stack.  It might be
>>> better to call it something with "clone" in the name, but I don't care
>>> terribly much.
>>
>> Meant to say: in addition to the PID/TID caching issues, this would be
>> able to run pthread_atfork() handlers.
> 
> Maybe..
> 
>  __clone3(unsigned long clone_flags, void* newsp, pid_t*
> parent_tidptr, pid_t* child_tidptr, void* tls);
> 
> ?
> 
> __clone2 seems to be used with ia64 already.
> 

The problem about this approach for clone it is current platform
specific, meaning it requires an assembly crafted routine for each
port.  Adding another one will require additional burden of making
current clone implementation either generic to provide clone and
clone3 or adding another clone3 implementation for *each* port.
And it would also add more additional effort for new ports.

I still think a better approach would be to just focus on try
remove the caching altogether without introducing performance
regressions. 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07  0:43             ` Zack Weinberg
  2016-10-07 14:44               ` Robert Święcki
@ 2016-10-07 18:30               ` Adhemerval Zanella
  1 sibling, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2016-10-07 18:30 UTC (permalink / raw)
  To: Zack Weinberg, Robert Święcki; +Cc: GLIBC Devel

On 06/10/2016 21:43, Zack Weinberg wrote:
> On Thu, Oct 6, 2016 at 8:42 PM, Zack Weinberg <zackw@panix.com> wrote:
>> On Thu, Oct 6, 2016 at 5:35 PM, Robert Święcki <robert@swiecki.net> wrote:
>>> Maybe providing something like fork_with_flags(int flags) would work
>>> here? Seems that most (all?) the projects that I've seen, which use
>>> clone() directly and had problems with caching of TID/PID, basically
>>> need fork with some flags (mainly of the CLONE_NEW* type). Such
>>> function would deal with tid/gid/locks/pthread_atfork as fork does,
>>> but would also OR the provided flags with the canonical ones?
>>
>> I've thought for quite some time that there should be a second
>> clone-wrapper in glibc that doesn't ask for a new stack.  It might be
>> better to call it something with "clone" in the name, but I don't care
>> terribly much.
> 
> Meant to say: in addition to the PID/TID caching issues, this would be
> able to run pthread_atfork() handlers.
> 
> zw

Adding a new implementation would require either change the clone
prototype (since flags is used for the kernel defined ones) or
adding a new arch-specific implementation.  This is an extra
burden on current assembly-only clone variants and for new
ports (an extra symbol to implement).

The stack check seems just error-prone one and I see it might be
justifiable to remove it.  I am not sure how we should act on 
semantic change for Linux-only syscall, so it might require to 
add compatibility symbol I would try to avoid it.  Also, this path
would lead to more arch-specific assembly hackery that I would
try to avoid.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-06 16:13 Caching of PID/TID after fork Robert Święcki
  2016-10-06 16:34 ` Paul Pluzhnikov
  2016-10-06 17:26 ` Rich Felker
@ 2016-10-07 19:38 ` Florian Weimer
  2016-10-07 21:23   ` Robert Święcki
  2 siblings, 1 reply; 22+ messages in thread
From: Florian Weimer @ 2016-10-07 19:38 UTC (permalink / raw)
  To: Robert Święcki; +Cc: libc-alpha

* Robert Święcki:

> Thefore I'd like to ask for one of the following solutions:
>
> 1. Don't cache PID/TID
>
> 2. Provide some kind of symbol, which would force for TID/PID to be
> reloaded in glibc.

I think we need a different design.

For example, when you clone a process, you really want that all
cryptographic random number generators are reinitialized.  Making this
depend on getpid does not really work, so we need a more general
solution.

The other question I have is whether we talk about clone-as-fork
exclusively.  Introducing a new thread into the process on which
non-free-standing code can run using clone is a different matter
altogether.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07 19:38 ` Florian Weimer
@ 2016-10-07 21:23   ` Robert Święcki
  2016-10-09 10:05     ` Florian Weimer
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-07 21:23 UTC (permalink / raw)
  To: Florian Weimer; +Cc: GLIBC Devel, Adhemerval Zanella

HI all,

> The stack check seems just error-prone one and I see it might be
> justifiable to remove it.  I am not sure how we should act on
> semantic change for Linux-only syscall, so it might require to
> add compatibility symbol I would try to avoid it.

I was thinking that maybe the following would work (in asm), without
the need to introduce additional files / codepaths.
ENTRY(clone):
  check_for_newsp_nonnull
  check_for_fn_nonnull
ENTRY(__clone3):
 clone_logic_wo_checks
END(__clone3)
END(clone)

EXPORT(clone)
EXPORT(__clone3)

But, all in all, I agree that doing it (TM) right way (with support
for pthread_at_fork) might be a better choice.

>
> > Thefore I'd like to ask for one of the following solutions:
> >
> > 1. Don't cache PID/TID
> >
> > 2. Provide some kind of symbol, which would force for TID/PID to be
> > reloaded in glibc.
>
> I think we need a different design.
>
> For example, when you clone a process, you really want that all
> cryptographic random number generators are reinitialized.  Making this
> depend on getpid does not really work, so we need a more general
> solution.

Agreed.

>
> The other question I have is whether we talk about clone-as-fork
> exclusively.  Introducing a new thread into the process on which
> non-free-standing code can run using clone is a different matter
> altogether.

To my knowledge, in the vast majority of cases (or even, in all
cases), where the TID/PID caching problem surfaced, it could have been
solved by fork_with_flags() indeed.

Though, one useful feature which could be potentially useful for some
porjects would be to create a detached process (for which the kernel
doesn't send termination signal - usually SIGCHLD - to the parent
process). And this cannot be solved by ORing custom flags with fork
ones, because SIGCHLD is already there. But, it's just a guess.

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-07 21:23   ` Robert Święcki
@ 2016-10-09 10:05     ` Florian Weimer
  2016-10-09 14:19       ` Robert Święcki
  0 siblings, 1 reply; 22+ messages in thread
From: Florian Weimer @ 2016-10-09 10:05 UTC (permalink / raw)
  To: Robert Święcki; +Cc: GLIBC Devel, Adhemerval Zanella

* Robert Święcki:

>> The other question I have is whether we talk about clone-as-fork
>> exclusively.  Introducing a new thread into the process on which
>> non-free-standing code can run using clone is a different matter
>> altogether.
>
> To my knowledge, in the vast majority of cases (or even, in all
> cases), where the TID/PID caching problem surfaced, it could have been
> solved by fork_with_flags() indeed.

So no new thread?  Good.

> Though, one useful feature which could be potentially useful for some
> porjects would be to create a detached process (for which the kernel
> doesn't send termination signal - usually SIGCHLD - to the parent
> process). And this cannot be solved by ORing custom flags with fork
> ones, because SIGCHLD is already there. But, it's just a guess.

Yes, I know of some cases where this would have helped as well.

If we go the fork-with-flags route, we should translate glibc-specific
flags to kernel flags anyway because the kernel might introduce new
flags which break the interface in subtle ways (see O_TMPFILE and mode
argument handling).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-09 10:05     ` Florian Weimer
@ 2016-10-09 14:19       ` Robert Święcki
  2016-10-10 18:03         ` Adhemerval Zanella
  0 siblings, 1 reply; 22+ messages in thread
From: Robert Święcki @ 2016-10-09 14:19 UTC (permalink / raw)
  To: Florian Weimer; +Cc: GLIBC Devel

>> Though, one useful feature which could be potentially useful for some
>> porjects would be to create a detached process (for which the kernel
>> doesn't send termination signal - usually SIGCHLD - to the parent
>> process). And this cannot be solved by ORing custom flags with fork
>> ones, because SIGCHLD is already there. But, it's just a guess.
>
> Yes, I know of some cases where this would have helped as well.
>
> If we go the fork-with-flags route, we should translate glibc-specific
> flags to kernel flags anyway because the kernel might introduce new
> flags which break the interface in subtle ways (see O_TMPFILE and mode
> argument handling).

An initial stab at the interface; it should avoid the problem you've
described above? I don't think additional rewrite of clone flags would
be required with such interface, even with future changes in the
kernel - in any case, using it requires good understanding of the
underlying kernel's clone() interface.

/* fork with flags */
pid_t ffork(int mode, unsigned long flags);

mode:
 FFORK_FLAG_SET - set flags directly
 FFORK_FLAG_OR - append flags to whatever fork uses internally

flags:
 as with clone()

ret val / errno:
 as with fork()

-- 
Robert Święcki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-09 14:19       ` Robert Święcki
@ 2016-10-10 18:03         ` Adhemerval Zanella
  2016-11-04 15:14           ` Florian Weimer
  0 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2016-10-10 18:03 UTC (permalink / raw)
  To: libc-alpha



On 09/10/2016 11:19, Robert Święcki wrote:
>>> Though, one useful feature which could be potentially useful for some
>>> porjects would be to create a detached process (for which the kernel
>>> doesn't send termination signal - usually SIGCHLD - to the parent
>>> process). And this cannot be solved by ORing custom flags with fork
>>> ones, because SIGCHLD is already there. But, it's just a guess.
>>
>> Yes, I know of some cases where this would have helped as well.
>>
>> If we go the fork-with-flags route, we should translate glibc-specific
>> flags to kernel flags anyway because the kernel might introduce new
>> flags which break the interface in subtle ways (see O_TMPFILE and mode
>> argument handling).
> 
> An initial stab at the interface; it should avoid the problem you've
> described above? I don't think additional rewrite of clone flags would
> be required with such interface, even with future changes in the
> kernel - in any case, using it requires good understanding of the
> underlying kernel's clone() interface.
> 
> /* fork with flags */
> pid_t ffork(int mode, unsigned long flags);
> 
> mode:
>  FFORK_FLAG_SET - set flags directly
>  FFORK_FLAG_OR - append flags to whatever fork uses internally
> 
> flags:
>  as with clone()
> 
> ret val / errno:
>  as with fork()
> 

I really do not think adding a another GLIBC/GNU specific extension to
circumvent a possible misused implementation detail should be way to
handle this specific issue.

For the pid/tid caching problem I see it is feasible to just get rid of
pid caching and adjust the implementation that rely on this.  The cached
pid is used basically in tgkill interface on both setxid, sigcancel
checking, pthread_{kill,cancel,sigqueue}, and getpid.  And for all
implementation it used basically as an optimization to avoid the syscall,
while 'fork' has a lot of internal control to avoid invalidate its
value.

We can just replace the cached value with a direct syscall with some
less performance (the getpid syscall), while both the {v}fork arch
defined implementation would be simpler (including getpid).  Also,
fork won't require any set/reset the cached value, which also only
adds complexity.

It also simplifies the nptl_db code as well, since no extra check would
require to validate the pid cached value between forks.

Now, about the stack argument checking, I also think it is now feasible
to remove since clone itself won't require to operate in any thread
implicit data.  What I am not sure if this semantic change would require
a compatibility symbol.

Below is a RFC patch to implement the first options (pid caching removal)
only for x86_64.  I think I covered all cases of caching PID field
(I basically renamed its field and covered all the build failures).  It
shows no regression on x86_64.

--

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 3016a2e..98a0ea2 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -438,9 +438,6 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
       SETUP_THREAD_SYSINFO (pd);
 #endif
 
-      /* The process ID is also the same as that of the caller.  */
-      pd->pid = THREAD_GETMEM (THREAD_SELF, pid);
-
       /* Don't allow setxid until cloned.  */
       pd->setxid_futex = -1;
 
@@ -577,9 +574,6 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	  /* Don't allow setxid until cloned.  */
 	  pd->setxid_futex = -1;
 
-	  /* The process ID is also the same as that of the caller.  */
-	  pd->pid = THREAD_GETMEM (THREAD_SELF, pid);
-
 	  /* Allocate the DTV for this thread.  */
 	  if (_dl_allocate_tls (TLS_TPADJ (pd)) == NULL)
 	    {
@@ -873,9 +867,6 @@ __reclaim_stacks (void)
 	  /* This marks the stack as free.  */
 	  curp->tid = 0;
 
-	  /* The PID field must be initialized for the new process.  */
-	  curp->pid = self->pid;
-
 	  /* Account for the size of the stack.  */
 	  stack_cache_actsize += curp->stackblock_size;
 
@@ -901,13 +892,6 @@ __reclaim_stacks (void)
 	}
     }
 
-  /* Reset the PIDs in any cached stacks.  */
-  list_for_each (runp, &stack_cache)
-    {
-      struct pthread *curp = list_entry (runp, struct pthread, list);
-      curp->pid = self->pid;
-    }
-
   /* Add the stack of all running threads to the cache.  */
   list_splice (&stack_used, &stack_cache);
 
@@ -1052,9 +1036,9 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
     return 0;
 
   int val;
+  pid_t pid = __getpid ();
   INTERNAL_SYSCALL_DECL (err);
-  val = INTERNAL_SYSCALL (tgkill, err, 3, THREAD_GETMEM (THREAD_SELF, pid),
-			  t->tid, SIGSETXID);
+  val = INTERNAL_SYSCALL_CALL (tgkill, err, pid, t->tid, SIGSETXID);
 
   /* If this failed, it must have had not started yet or else exited.  */
   if (!INTERNAL_SYSCALL_ERROR_P (val, err))
diff --git a/nptl/descr.h b/nptl/descr.h
index 8e4938d..17a2c9f 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -167,7 +167,7 @@ struct pthread
      therefore stack) used' flag.  */
   pid_t tid;
 
-  /* Process ID - thread group ID in kernel speak.  */
+  /* Ununsed.  */
   pid_t pid;
 
   /* List of robust mutexes the thread is holding.  */
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index bdbdfed..48fab50 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -184,18 +184,12 @@ __nptl_set_robust (struct pthread *self)
 static void
 sigcancel_handler (int sig, siginfo_t *si, void *ctx)
 {
-  /* Determine the process ID.  It might be negative if the thread is
-     in the middle of a fork() call.  */
-  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__glibc_unlikely (pid < 0))
-    pid = -pid;
-
   /* Safety check.  It would be possible to call this function for
      other signals and send a signal from another process.  This is not
      correct and might even be a security problem.  Try to catch as
      many incorrect invocations as possible.  */
   if (sig != SIGCANCEL
-      || si->si_pid != pid
+      || si->si_pid != __getpid()
       || si->si_code != SI_TKILL)
     return;
 
@@ -243,19 +237,14 @@ struct xid_command *__xidcmd attribute_hidden;
 static void
 sighandler_setxid (int sig, siginfo_t *si, void *ctx)
 {
-  /* Determine the process ID.  It might be negative if the thread is
-     in the middle of a fork() call.  */
-  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
   int result;
-  if (__glibc_unlikely (pid < 0))
-    pid = -pid;
 
   /* Safety check.  It would be possible to call this function for
      other signals and send a signal from another process.  This is not
      correct and might even be a security problem.  Try to catch as
      many incorrect invocations as possible.  */
   if (sig != SIGSETXID
-      || si->si_pid != pid
+      || si->si_pid != __getpid ()
       || si->si_code != SI_TKILL)
     return;
 
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 1419baf..89d02e1 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -22,7 +22,7 @@
 #include "pthreadP.h"
 #include <atomic.h>
 #include <sysdep.h>
-
+#include <unistd.h>
 
 int
 pthread_cancel (pthread_t th)
@@ -66,19 +66,11 @@ pthread_cancel (pthread_t th)
 #ifdef SIGCANCEL
 	  /* The cancellation handler will take care of marking the
 	     thread as canceled.  */
-	  INTERNAL_SYSCALL_DECL (err);
-
-	  /* One comment: The PID field in the TCB can temporarily be
-	     changed (in fork).  But this must not affect this code
-	     here.  Since this function would have to be called while
-	     the thread is executing fork, it would have to happen in
-	     a signal handler.  But this is no allowed, pthread_cancel
-	     is not guaranteed to be async-safe.  */
-	  int val;
-	  val = INTERNAL_SYSCALL (tgkill, err, 3,
-				  THREAD_GETMEM (THREAD_SELF, pid), pd->tid,
-				  SIGCANCEL);
+	  pid_t pid = getpid ();
 
+	  INTERNAL_SYSCALL_DECL (err);
+	  int val = INTERNAL_SYSCALL_CALL (tgkill, err, pid, pd->tid,
+					   SIGCANCEL);
 	  if (INTERNAL_SYSCALL_ERROR_P (val, err))
 	    result = INTERNAL_SYSCALL_ERRNO (val, err);
 #else
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index fb906f0..32d7484 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -68,7 +68,6 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
     {
       /* No stack information available.  This must be for the initial
 	 thread.  Get the info in some magical way.  */
-      assert (abs (thread->pid) == thread->tid);
 
       /* Stack size limit.  */
       struct rlimit rl;
diff --git a/nptl_db/td_ta_thr_iter.c b/nptl_db/td_ta_thr_iter.c
index a990fed..9e50599 100644
--- a/nptl_db/td_ta_thr_iter.c
+++ b/nptl_db/td_ta_thr_iter.c
@@ -76,48 +76,28 @@ iterate_thread_list (td_thragent_t *ta, td_thr_iter_f *callback,
       if (ps_pdread (ta->ph, addr, copy, ta->ta_sizeof_pthread) != PS_OK)
 	return TD_ERR;
 
-      /* Verify that this thread's pid field matches the child PID.
-	 If its pid field is negative, it's about to do a fork or it
-	 is the sole thread in a fork child.  */
-      psaddr_t pid;
-      err = DB_GET_FIELD_LOCAL (pid, ta, copy, pthread, pid, 0);
-      if (err == TD_OK && (pid_t) (uintptr_t) pid < 0)
-	{
-	  if (-(pid_t) (uintptr_t) pid == match_pid)
-	    /* It is about to do a fork, but is really still the parent PID.  */
-	    pid = (psaddr_t) (uintptr_t) match_pid;
-	  else
-	    /* It must be a fork child, whose new PID is in the tid field.  */
-	    err = DB_GET_FIELD_LOCAL (pid, ta, copy, pthread, tid, 0);
-	}
+      err = DB_GET_FIELD_LOCAL (schedpolicy, ta, copy, pthread,
+				schedpolicy, 0);
       if (err != TD_OK)
 	break;
+      err = DB_GET_FIELD_LOCAL (schedprio, ta, copy, pthread,
+				schedparam_sched_priority, 0);
+      if (err != TD_OK)
+	break;
+
+      /* Now test whether this thread matches the specified conditions.  */
 
-      if ((pid_t) (uintptr_t) pid == match_pid)
+      /* Only if the priority level is as high or higher.  */
+      int descr_pri = ((uintptr_t) schedpolicy == SCHED_OTHER
+		       ? 0 : (uintptr_t) schedprio);
+      if (descr_pri >= ti_pri)
 	{
-	  err = DB_GET_FIELD_LOCAL (schedpolicy, ta, copy, pthread,
-				    schedpolicy, 0);
-	  if (err != TD_OK)
-	    break;
-	  err = DB_GET_FIELD_LOCAL (schedprio, ta, copy, pthread,
-				    schedparam_sched_priority, 0);
-	  if (err != TD_OK)
-	    break;
-
-	  /* Now test whether this thread matches the specified conditions.  */
-
-	  /* Only if the priority level is as high or higher.  */
-	  int descr_pri = ((uintptr_t) schedpolicy == SCHED_OTHER
-			   ? 0 : (uintptr_t) schedprio);
-	  if (descr_pri >= ti_pri)
-	    {
-	      /* Yep, it matches.  Call the callback function.  */
-	      td_thrhandle_t th;
-	      th.th_ta_p = (td_thragent_t *) ta;
-	      th.th_unique = addr;
-	      if (callback (&th, cbdata_p) != 0)
-		return TD_DBERR;
-	    }
+	  /* Yep, it matches.  Call the callback function.  */
+	  td_thrhandle_t th;
+	  th.th_ta_p = (td_thragent_t *) ta;
+	  th.th_unique = addr;
+	  if (callback (&th, cbdata_p) != 0)
+	    return TD_DBERR;
 	}
 
       /* Get the pointer to the next element.  */
diff --git a/nptl_db/td_thr_validate.c b/nptl_db/td_thr_validate.c
index f3c8a7b..9b89fec 100644
--- a/nptl_db/td_thr_validate.c
+++ b/nptl_db/td_thr_validate.c
@@ -80,28 +80,5 @@ td_thr_validate (const td_thrhandle_t *th)
 	err = TD_OK;
     }
 
-  if (err == TD_OK)
-    {
-      /* Verify that this is not a stale element in a fork child.  */
-      pid_t match_pid = ps_getpid (th->th_ta_p->ph);
-      psaddr_t pid;
-      err = DB_GET_FIELD (pid, th->th_ta_p, th->th_unique, pthread, pid, 0);
-      if (err == TD_OK && (pid_t) (uintptr_t) pid < 0)
-	{
-	  /* This was a thread that was about to fork, or it is the new sole
-	     thread in a fork child.  In the latter case, its tid was stored
-	     via CLONE_CHILD_SETTID and so is already the proper child PID.  */
-	  if (-(pid_t) (uintptr_t) pid == match_pid)
-	    /* It is about to do a fork, but is really still the parent PID.  */
-	    pid = (psaddr_t) (uintptr_t) match_pid;
-	  else
-	    /* It must be a fork child, whose new PID is in the tid field.  */
-	    err = DB_GET_FIELD (pid, th->th_ta_p, th->th_unique,
-				pthread, tid, 0);
-	}
-      if (err == TD_OK && (pid_t) (uintptr_t) pid != match_pid)
-	err = TD_NOTHR;
-    }
-
   return err;
 }
diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c
index ea135f8..7294c08 100644
--- a/sysdeps/nptl/fork.c
+++ b/sysdeps/nptl/fork.c
@@ -132,15 +132,10 @@ __libc_fork (void)
     }
 
 #ifndef NDEBUG
-  pid_t ppid = THREAD_GETMEM (THREAD_SELF, tid);
+  INTERNAL_SYSCALL_DECL (err);
+  pid_t ppid = INTERNAL_SYSCALL_CALL (gettid, err);
 #endif
 
-  /* We need to prevent the getpid() code to update the PID field so
-     that, if a signal arrives in the child very early and the signal
-     handler uses getpid(), the value returned is correct.  */
-  pid_t parentpid = THREAD_GETMEM (THREAD_SELF, pid);
-  THREAD_SETMEM (THREAD_SELF, pid, -parentpid);
-
 #ifdef ARCH_FORK
   pid = ARCH_FORK ();
 #else
@@ -159,9 +154,6 @@ __libc_fork (void)
       if (__fork_generation_pointer != NULL)
 	*__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
 
-      /* Adjust the PID field for the new process.  */
-      THREAD_SETMEM (self, pid, THREAD_GETMEM (self, tid));
-
 #if HP_TIMING_AVAIL
       /* The CPU clock of the thread and process have to be set to zero.  */
       hp_timing_t now;
@@ -231,10 +223,7 @@ __libc_fork (void)
     }
   else
     {
-      assert (THREAD_GETMEM (THREAD_SELF, tid) == ppid);
-
-      /* Restore the PID value.  */
-      THREAD_SETMEM (THREAD_SELF, pid, parentpid);
+      assert (THREAD_GETMEM (THREAD_SELF, tid) == 0);
 
       /* Release acquired locks in the multi-threaded case.  */
       if (multiple_threads)
diff --git a/sysdeps/unix/sysv/linux/createthread.c b/sysdeps/unix/sysv/linux/createthread.c
index 6d32cec..ec86f50 100644
--- a/sysdeps/unix/sysv/linux/createthread.c
+++ b/sysdeps/unix/sysv/linux/createthread.c
@@ -128,10 +128,10 @@ create_thread (struct pthread *pd, const struct pthread_attr *attr,
 	      /* The operation failed.  We have to kill the thread.
 		 We let the normal cancellation mechanism do the work.  */
 
+	      pid_t pid = __getpid ();
 	      INTERNAL_SYSCALL_DECL (err2);
-	      (void) INTERNAL_SYSCALL (tgkill, err2, 3,
-				       THREAD_GETMEM (THREAD_SELF, pid),
-				       pd->tid, SIGCANCEL);
+	      (void) INTERNAL_SYSCALL_CALL (tgkill, err2, pid, pd->tid,
+					    SIGCANCEL);
 
 	      return INTERNAL_SYSCALL_ERRNO (res, err);
 	    }
diff --git a/sysdeps/unix/sysv/linux/getpid.c b/sysdeps/unix/sysv/linux/getpid.c
index 1124549..2bfafed 100644
--- a/sysdeps/unix/sysv/linux/getpid.c
+++ b/sysdeps/unix/sysv/linux/getpid.c
@@ -20,43 +20,11 @@
 #include <tls.h>
 #include <sysdep.h>
 
-
-#if IS_IN (libc)
-static inline __attribute__((always_inline)) pid_t really_getpid (pid_t oldval);
-
-static inline __attribute__((always_inline)) pid_t
-really_getpid (pid_t oldval)
-{
-  if (__glibc_likely (oldval == 0))
-    {
-      pid_t selftid = THREAD_GETMEM (THREAD_SELF, tid);
-      if (__glibc_likely (selftid != 0))
-	return selftid;
-    }
-
-  INTERNAL_SYSCALL_DECL (err);
-  pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
-
-  /* We do not set the PID field in the TID here since we might be
-     called from a signal handler while the thread executes fork.  */
-  if (oldval == 0)
-    THREAD_SETMEM (THREAD_SELF, tid, result);
-  return result;
-}
-#endif
-
 pid_t
 __getpid (void)
 {
-#if !IS_IN (libc)
   INTERNAL_SYSCALL_DECL (err);
-  pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
-#else
-  pid_t result = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__glibc_unlikely (result <= 0))
-    result = really_getpid (result);
-#endif
-  return result;
+  return INTERNAL_SYSCALL_CALL (getpid, err);
 }
 
 libc_hidden_def (__getpid)
diff --git a/sysdeps/unix/sysv/linux/pthread-pids.h b/sysdeps/unix/sysv/linux/pthread-pids.h
index d42bba0..618a5b1 100644
--- a/sysdeps/unix/sysv/linux/pthread-pids.h
+++ b/sysdeps/unix/sysv/linux/pthread-pids.h
@@ -26,5 +26,5 @@ static inline void
 __pthread_initialize_pids (struct pthread *pd)
 {
   INTERNAL_SYSCALL_DECL (err);
-  pd->pid = pd->tid = INTERNAL_SYSCALL (set_tid_address, err, 1, &pd->tid);
+  pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, err, &pd->tid);
 }
diff --git a/sysdeps/unix/sysv/linux/pthread_kill.c b/sysdeps/unix/sysv/linux/pthread_kill.c
index bcb3009..15c9ba6 100644
--- a/sysdeps/unix/sysv/linux/pthread_kill.c
+++ b/sysdeps/unix/sysv/linux/pthread_kill.c
@@ -21,6 +21,7 @@
 #include <pthreadP.h>
 #include <tls.h>
 #include <sysdep.h>
+#include <unistd.h>
 
 
 int
@@ -49,14 +50,15 @@ __pthread_kill (pthread_t threadid, int signo)
   /* We have a special syscall to do the work.  */
   INTERNAL_SYSCALL_DECL (err);
 
+  pid_t pid = getpid ();
+
   /* One comment: The PID field in the TCB can temporarily be changed
      (in fork).  But this must not affect this code here.  Since this
      function would have to be called while the thread is executing
      fork, it would have to happen in a signal handler.  But this is
      no allowed, pthread_kill is not guaranteed to be async-safe.  */
   int val;
-  val = INTERNAL_SYSCALL (tgkill, err, 3, THREAD_GETMEM (THREAD_SELF, pid),
-			  tid, signo);
+  val = INTERNAL_SYSCALL_CALL (tgkill, err, pid, tid, signo);
 
   return (INTERNAL_SYSCALL_ERROR_P (val, err)
 	  ? INTERNAL_SYSCALL_ERRNO (val, err) : 0);
diff --git a/sysdeps/unix/sysv/linux/pthread_sigqueue.c b/sysdeps/unix/sysv/linux/pthread_sigqueue.c
index 7694d54..642366b 100644
--- a/sysdeps/unix/sysv/linux/pthread_sigqueue.c
+++ b/sysdeps/unix/sysv/linux/pthread_sigqueue.c
@@ -49,12 +49,14 @@ pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
   if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID)
     return EINVAL;
 
+  pid_t pid = getpid ();
+
   /* Set up the siginfo_t structure.  */
   siginfo_t info;
   memset (&info, '\0', sizeof (siginfo_t));
   info.si_signo = signo;
   info.si_code = SI_QUEUE;
-  info.si_pid = THREAD_GETMEM (THREAD_SELF, pid);
+  info.si_pid = pid;
   info.si_uid = getuid ();
   info.si_value = value;
 
@@ -66,9 +68,8 @@ pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
      function would have to be called while the thread is executing
      fork, it would have to happen in a signal handler.  But this is
      no allowed, pthread_sigqueue is not guaranteed to be async-safe.  */
-  int val = INTERNAL_SYSCALL (rt_tgsigqueueinfo, err, 4,
-			      THREAD_GETMEM (THREAD_SELF, pid),
-			      tid, signo, &info);
+  int val = INTERNAL_SYSCALL_CALL (rt_tgsigqueueinfo, err, pid, tid, signo,
+				   &info);
 
   return (INTERNAL_SYSCALL_ERROR_P (val, err)
 	  ? INTERNAL_SYSCALL_ERRNO (val, err) : 0);
diff --git a/sysdeps/unix/sysv/linux/tst-clone2.c b/sysdeps/unix/sysv/linux/tst-clone2.c
index 68a7e6d..2d6369e 100644
--- a/sysdeps/unix/sysv/linux/tst-clone2.c
+++ b/sysdeps/unix/sysv/linux/tst-clone2.c
@@ -28,8 +28,14 @@
 #include <stdlib.h>
 #include <sys/types.h>
 #include <sys/wait.h>
+#include <sys/syscall.h>
 
-#include <tls.h> /* for THREAD_* macros.  */
+#include <stackinfo.h>  /* For _STACK_GROWS_{UP,DOWN}.  */
+
+static int do_test (void);
+
+#define TEST_FUNCTION do_test ()
+#include <test-skeleton.c>
 
 static int sig;
 static int pipefd[2];
@@ -39,9 +45,12 @@ f (void *a)
 {
   close (pipefd[0]);
 
-  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
-  pid_t tid = THREAD_GETMEM (THREAD_SELF, tid);
+  pid_t ppid = getppid ();
+  pid_t pid = getpid ();
+  pid_t tid = syscall (__NR_gettid);
 
+  while (write (pipefd[1], &ppid, sizeof ppid) < 0)
+    continue;
   while (write (pipefd[1], &pid, sizeof pid) < 0)
     continue;
   while (write (pipefd[1], &tid, sizeof tid) < 0)
@@ -52,26 +61,19 @@ f (void *a)
 
 
 static int
-clone_test (int clone_flags)
+do_test (void)
 {
   sig = SIGRTMIN;
   sigset_t ss;
   sigemptyset (&ss);
   sigaddset (&ss, sig);
   if (sigprocmask (SIG_BLOCK, &ss, NULL) != 0)
-    {
-      printf ("sigprocmask failed: %m\n");
-      return 1;
-    }
+    FAIL_EXIT1 ("sigprocmask failed: %m");
 
   if (pipe2 (pipefd, O_CLOEXEC))
-    {
-      printf ("sigprocmask failed: %m\n");
-      return 1;
-    }
-
-  pid_t ppid = getpid ();
+    FAIL_EXIT1 ("pipe failed: %m");
 
+  int clone_flags = 0;
 #ifdef __ia64__
   extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
 		       size_t __child_stack_size, int __flags,
@@ -88,61 +90,47 @@ clone_test (int clone_flags)
 #error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
 #endif
 #endif
+
   close (pipefd[1]);
 
   if (p == -1)
+    FAIL_EXIT1("clone failed: %m");
+
+  pid_t ppid, pid, tid;
+  if (read (pipefd[0], &ppid, sizeof pid) != sizeof pid)
     {
-      printf ("clone failed: %m\n");
-      return 1;
+      kill (p, SIGKILL);
+      FAIL_EXIT1 ("read ppid failed: %m");
     }
-
-  pid_t pid, tid;
   if (read (pipefd[0], &pid, sizeof pid) != sizeof pid)
     {
-      printf ("read pid failed: %m\n");
       kill (p, SIGKILL);
-      return 1;
+      FAIL_EXIT1 ("read pid failed: %m");
     }
   if (read (pipefd[0], &tid, sizeof tid) != sizeof tid)
     {
-      printf ("read pid failed: %m\n");
       kill (p, SIGKILL);
-      return 1;
+      FAIL_EXIT1 ("read tid failed: %m");
     }
 
   close (pipefd[0]);
 
   int ret = 0;
 
-  /* For CLONE_VM glibc clone implementation does not change the pthread
-     pid/tid field.  */
-  if ((clone_flags & CLONE_VM) == CLONE_VM)
-    {
-      if ((ppid != pid) || (ppid != tid))
-	{
-	  printf ("parent pid (%i) != received pid/tid (%i/%i)\n",
-		  (int)ppid, (int)pid, (int)tid);
-	  ret = 1;
-	}
-    }
-  /* For any other flag clone updates the new pthread pid and tid with
-     the clone return value.  */
-  else
-    {
-      if ((p != pid) || (p != tid))
-	{
-	  printf ("child pid (%i) != received pid/tid (%i/%i)\n",
-		  (int)p, (int)pid, (int)tid);
-	  ret = 1;
-	}
-    }
+  pid_t own_pid = getpid ();
+  pid_t own_tid = syscall (__NR_gettid);
+
+  /* Some sanity checks for clone syscall: returned ppid should be currernt
+     pid and both returned tid/pid should be different from current one.  */
+  if ((ppid != own_pid) || (pid == own_pid) || (tid == own_tid))
+    FAIL_RET ("ppd=%i pid=%i tid=%i | own_pid=%i own_tid=%i",
+ 	      (int)ppid, (int)pid, (int)tid, (int)own_pid, (int)own_tid);
 
   int e;
   if (waitpid (p, &e, __WCLONE) != p)
     {
-      puts ("waitpid failed");
       kill (p, SIGKILL);
-      return 1;
+      FAIL_EXIT1 ("waitpid failed");
     }
   if (!WIFEXITED (e))
     {
@@ -150,29 +138,10 @@ clone_test (int clone_flags)
 	printf ("died from signal %s\n", strsignal (WTERMSIG (e)));
       else
 	puts ("did not terminate correctly");
-      return 1;
+      exit (EXIT_FAILURE);
     }
   if (WEXITSTATUS (e) != 0)
-    {
-      printf ("exit code %d\n", WEXITSTATUS (e));
-      return 1;
-    }
+    FAIL_EXIT1 ("exit code %d", WEXITSTATUS (e));
 
   return ret;
 }
-
-int
-do_test (void)
-{
-  /* First, check that the clone implementation, without any flag, updates
-     the struct pthread to contain the new PID and TID.  */
-  int ret = clone_test (0);
-  /* Second, check that with CLONE_VM the struct pthread PID and TID fields
-     remain unmodified after the clone.  Any modifications would cause problem
-     for the parent as described in bug 19957.  */
-  ret += clone_test (CLONE_VM);
-  return ret;
-}
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S
index 66f4b11..5629aed 100644
--- a/sysdeps/unix/sysv/linux/x86_64/clone.S
+++ b/sysdeps/unix/sysv/linux/x86_64/clone.S
@@ -91,14 +91,6 @@ L(thread_start):
 	   the outermost frame obviously.  */
 	xorl	%ebp, %ebp
 
-	andq	$CLONE_VM, %rdi
-	jne	1f
-	movl	$SYS_ify(getpid), %eax
-	syscall
-	movl	%eax, %fs:PID
-	movl	%eax, %fs:TID
-1:
-
 	/* Set up arguments for the function call.  */
 	popq	%rax		/* Function to call.  */
 	popq	%rdi		/* Argument.  */
diff --git a/sysdeps/unix/sysv/linux/x86_64/vfork.S b/sysdeps/unix/sysv/linux/x86_64/vfork.S
index 8332ade..cdd2dea 100644
--- a/sysdeps/unix/sysv/linux/x86_64/vfork.S
+++ b/sysdeps/unix/sysv/linux/x86_64/vfork.S
@@ -34,16 +34,6 @@ ENTRY (__vfork)
 	cfi_adjust_cfa_offset(-8)
 	cfi_register(%rip, %rdi)
 
-	/* Save the TCB-cached PID away in %esi, and then negate the TCB
-           field.  But if it's zero, set it to 0x80000000 instead.  See
-           raise.c for the logic that relies on this value.  */
-	movl	%fs:PID, %esi
-	movl	$0x80000000, %ecx
-	movl	%esi, %edx
-	negl	%edx
-	cmove	%ecx, %edx
-	movl	%edx, %fs:PID
-
 	/* Stuff the syscall number in RAX and enter into the kernel.  */
 	movl	$SYS_ify (vfork), %eax
 	syscall
@@ -52,14 +42,6 @@ ENTRY (__vfork)
 	pushq	%rdi
 	cfi_adjust_cfa_offset(8)
 
-	/* Restore the original value of the TCB cache of the PID, if we're
-	   the parent.  But in the child (syscall return value equals zero),
-	   leave things as they are.  */
-	testq	%rax, %rax
-	je	1f
-	movl	%esi, %fs:PID
-1:
-
 	cmpl	$-4095, %eax
 	jae SYSCALL_ERROR_LABEL		/* Branch forward if it failed.  */
 
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index aeb7526..8a25c48 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
 
 RESULT			offsetof (struct pthread, result)
 TID			offsetof (struct pthread, tid)
-PID			offsetof (struct pthread, pid)
 CANCELHANDLING		offsetof (struct pthread, cancelhandling)
 CLEANUP_JMP_BUF		offsetof (struct pthread, cleanup_jmp_buf)
 CLEANUP			offsetof (struct pthread, cleanup)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-10-10 18:03         ` Adhemerval Zanella
@ 2016-11-04 15:14           ` Florian Weimer
  2016-11-04 16:03             ` Adhemerval Zanella
  0 siblings, 1 reply; 22+ messages in thread
From: Florian Weimer @ 2016-11-04 15:14 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha

On 10/10/2016 08:03 PM, Adhemerval Zanella wrote:
> +  /* Some sanity checks for clone syscall: returned ppid should be currernt

Typo: “currernt”

On its own, this approach looks okay, but I am worried that it sends a 
message that it's okay to clone processes without additional measures to 
protect PRNGs and things like that.

Florian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-11-04 15:14           ` Florian Weimer
@ 2016-11-04 16:03             ` Adhemerval Zanella
  2016-11-07 16:04               ` Florian Weimer
  0 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2016-11-04 16:03 UTC (permalink / raw)
  To: Florian Weimer, libc-alpha



On 04/11/2016 13:14, Florian Weimer wrote:
> On 10/10/2016 08:03 PM, Adhemerval Zanella wrote:
>> +  /* Some sanity checks for clone syscall: returned ppid should be currernt
> 
> Typo: “currernt”
> 
> On its own, this approach looks okay, but I am worried that it sends a message that it's okay to clone processes without additional measures to protect PRNGs and things like that.
> 
> Florian

I am not sure if you referring you to my initial RFC patch or the one 
complete I sent [1] since you replied to the original thread.

Anyway, I see that with current fixes on some algorithm (execv not
using dynamic memory allocation and various issues with posix_spawn)
clone direct usage should be really required in very specific
scanerios (mostly on new containers projects and such alike).  And 
I would expect that these very projects to know the constraints and 
limitations of using the syscall directly.

[1] https://sourceware.org/ml/libc-alpha/2016-10/msg00233.html

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Caching of PID/TID after fork
  2016-11-04 16:03             ` Adhemerval Zanella
@ 2016-11-07 16:04               ` Florian Weimer
  0 siblings, 0 replies; 22+ messages in thread
From: Florian Weimer @ 2016-11-07 16:04 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha

On 11/04/2016 05:03 PM, Adhemerval Zanella wrote:
>
>
> On 04/11/2016 13:14, Florian Weimer wrote:
>> On 10/10/2016 08:03 PM, Adhemerval Zanella wrote:
>>> +  /* Some sanity checks for clone syscall: returned ppid should be currernt
>>
>> Typo: “currernt”
>>
>> On its own, this approach looks okay, but I am worried that it sends a message that it's okay to clone processes without additional measures to protect PRNGs and things like that.
>>
>> Florian
>
> I am not sure if you referring you to my initial RFC patch or the one
> complete I sent [1] since you replied to the original thread.

Oh, I'll have a look at the updated patch then.

Florian

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2016-11-07 16:04 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-06 16:13 Caching of PID/TID after fork Robert Święcki
2016-10-06 16:34 ` Paul Pluzhnikov
2016-10-06 17:03   ` Robert Święcki
2016-10-06 18:32     ` Adhemerval Zanella
2016-10-06 17:26 ` Rich Felker
2016-10-06 17:42   ` Robert Święcki
2016-10-06 18:05     ` Rich Felker
2016-10-06 18:26       ` Robert Święcki
2016-10-06 21:35         ` Robert Święcki
2016-10-07  0:42           ` Zack Weinberg
2016-10-07  0:43             ` Zack Weinberg
2016-10-07 14:44               ` Robert Święcki
2016-10-07 18:20                 ` Adhemerval Zanella
2016-10-07 18:30               ` Adhemerval Zanella
2016-10-07 19:38 ` Florian Weimer
2016-10-07 21:23   ` Robert Święcki
2016-10-09 10:05     ` Florian Weimer
2016-10-09 14:19       ` Robert Święcki
2016-10-10 18:03         ` Adhemerval Zanella
2016-11-04 15:14           ` Florian Weimer
2016-11-04 16:03             ` Adhemerval Zanella
2016-11-07 16:04               ` Florian Weimer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).