nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]

Message ID 87k1i6cah6.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]
Related show

Commit Message

Florian Weimer Feb. 11, 2019, 8:54 p.m.
After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
constraint for Systemtap probes [BZ #24164]"), we load pd->result into
a register in the probe below:

      /* Free the TCB.  */
      __free_tcb (pd);
    }
  else
    pd->joinid = NULL;

  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

However, at this point, the thread descriptor has been freed.  If the
thread stack does not fit into the thread stack cache, the memory will
have been unmapped, and the program will crash in the probe.

2019-02-11  Florian Weimer  <fweimer@redhat.com>

	[BZ #24211]
	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
	pd->result again after the thread descriptor has been freed.

Comments

Andreas Schwab Feb. 11, 2019, 9:11 p.m. | #1
On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..45deba6a74 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -101,7 +101,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>    else
>      pd->joinid = NULL;
>  
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

result and pd->result are not the same thing.

Andreas.
Carlos O'Donell Feb. 11, 2019, 9:13 p.m. | #2
On 2/11/19 3:54 PM, Florian Weimer wrote:
> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
> a register in the probe below:
> 
>       /* Free the TCB.  */
>       __free_tcb (pd);
>     }
>   else
>     pd->joinid = NULL;
> 
>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
> 
> However, at this point, the thread descriptor has been freed.  If the
> thread stack does not fit into the thread stack cache, the memory will
> have been unmapped, and the program will crash in the probe.
> 
> 2019-02-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24211]
> 	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
> 	pd->result again after the thread descriptor has been freed.
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..45deba6a74 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -101,7 +101,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>    else
>      pd->joinid = NULL;
>  
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

Why not move the probe up?

 77   if (block)
 78     {
 79       /* If abstime is NULL we switch to asynchronous cancellation.  If we
 80          are cancelled then the thread we are waiting for must be marked as
 81          un-wait-ed for again.  */
 82       pthread_cleanup_push (cleanup, &pd->joinid);
 83 
 84       result = lll_wait_tid (pd->tid, abstime);
 85 
 86       pthread_cleanup_pop (0);
 87     }
 88 

LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
^^^^

At this point the join is complete.

We have the threadid (constant).

We have the result (just returned from lll_wait_tid).

We have pd->result (the thread returned).

We aren't waiting for anything else?

 89   if (__glibc_likely (result == 0))
 90     {
 91       /* We mark the thread as terminated and as joined.  */
 92       pd->tid = -1;
 93 
 94       /* Store the return value if the caller is interested.  */
 95       if (thread_return != NULL)
 96         *thread_return = pd->result;
 97 
 98       /* Free the TCB.  */
 99       __free_tcb (pd);
100     }
101   else
102     pd->joinid = NULL;
103 
104   LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);


>  
>    return result;
>  }
> 

As an orthogonal issue the probe needs documenting in manual/probes.texi
under a new "POSIX Thread Probes" section ;-)
Florian Weimer Feb. 11, 2019, 9:22 p.m. | #3
* Andreas Schwab:

> On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
>> index ecb78ffba5..45deba6a74 100644
>> --- a/nptl/pthread_join_common.c
>> +++ b/nptl/pthread_join_common.c
>> @@ -101,7 +101,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>>    else
>>      pd->joinid = NULL;
>>  
>> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
>
> result and pd->result are not the same thing.

Oops.  What about this?

Or should I write essentially the same probe twice?  I don't want to
introduce a new probe name because I want to backport this (along with
the fix for bug 24164).

Thanks,
Florian

nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]

After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
constraint for Systemtap probes [BZ #24164]"), we load pd->result into
a register in the probe below:

      /* Free the TCB.  */
      __free_tcb (pd);
    }
  else
    pd->joinid = NULL;

  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

However, at this point, the thread descriptor has been freed.  If the
thread stack does not fit into the thread stack cache, the memory will
have been unmapped, and the program will crash in the probe.

2019-02-11  Florian Weimer  <fweimer@redhat.com>

	[BZ #24211]
	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
	pd->result after the thread descriptor has been freed.

diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index ecb78ffba5..366feb376b 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -86,6 +86,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
       pthread_cleanup_pop (0);
     }
 
+  void *pd_result = pd->result;
   if (__glibc_likely (result == 0))
     {
       /* We mark the thread as terminated and as joined.  */
@@ -93,7 +94,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
 
       /* Store the return value if the caller is interested.  */
       if (thread_return != NULL)
-	*thread_return = pd->result;
+	*thread_return = pd_result;
 
       /* Free the TCB.  */
       __free_tcb (pd);
@@ -101,7 +102,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
   else
     pd->joinid = NULL;
 
-  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
+  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);
 
   return result;
 }
Florian Weimer Feb. 11, 2019, 9:26 p.m. | #4
* Carlos O'Donell:

> Why not move the probe up?
>
>  77   if (block)
>  78     {
>  79       /* If abstime is NULL we switch to asynchronous cancellation.  If we
>  80          are cancelled then the thread we are waiting for must be marked as
>  81          un-wait-ed for again.  */
>  82       pthread_cleanup_push (cleanup, &pd->joinid);
>  83 
>  84       result = lll_wait_tid (pd->tid, abstime);
>  85 
>  86       pthread_cleanup_pop (0);
>  87     }
>  88 
>
> LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> ^^^^

I think it should come after the stack has been freed.  It's explicitly
documented as a return probe (“probe for pthread_join return”).

> At this point the join is complete.
>
> We have the threadid (constant).
>
> We have the result (just returned from lll_wait_tid).
>
> We have pd->result (the thread returned).
>
> We aren't waiting for anything else?

Something might be interested in the fact that the stack is actually
gone.  I'm not sure if this is a change we should make when backporting.

Thanks,
Florian
Carlos O'Donell Feb. 11, 2019, 9:31 p.m. | #5
On 2/11/19 4:22 PM, Florian Weimer wrote:
> * Andreas Schwab:
> 
>> On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
>>> index ecb78ffba5..45deba6a74 100644
>>> --- a/nptl/pthread_join_common.c
>>> +++ b/nptl/pthread_join_common.c
>>> @@ -101,7 +101,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>>>    else
>>>      pd->joinid = NULL;
>>>  
>>> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>>> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
>>
>> result and pd->result are not the same thing.
> 
> Oops.  What about this?
> 
> Or should I write essentially the same probe twice?  I don't want to
> introduce a new probe name because I want to backport this (along with
> the fix for bug 24164).
> 
> Thanks,
> Florian
> 
> nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]
> 
> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
> a register in the probe below:
> 
>       /* Free the TCB.  */
>       __free_tcb (pd);
>     }
>   else
>     pd->joinid = NULL;
> 
>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
> 
> However, at this point, the thread descriptor has been freed.  If the
> thread stack does not fit into the thread stack cache, the memory will
> have been unmapped, and the program will crash in the probe.
> 
> 2019-02-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24211]
> 	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
> 	pd->result after the thread descriptor has been freed.
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..366feb376b 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -86,6 +86,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>        pthread_cleanup_pop (0);
>      }
>  
> +  void *pd_result = pd->result;
>    if (__glibc_likely (result == 0))
>      {
>        /* We mark the thread as terminated and as joined.  */
> @@ -93,7 +94,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>  
>        /* Store the return value if the caller is interested.  */
>        if (thread_return != NULL)
> -	*thread_return = pd->result;
> +	*thread_return = pd_result;
>  
>        /* Free the TCB.  */
>        __free_tcb (pd);
> @@ -101,7 +102,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>    else
>      pd->joinid = NULL;
>  
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);

What prevents the compiler from optimizing away pd_result and using
pd->result directly again?

The actions of __free_tcb() and their consequences are not visible
to the compiler.

Do we have to make pd_result volatile to avoid optimizations that
move that read? Or do we need a compiler barrier to ensure that
the read is complete before we free the stack?
  
>    return result;
>  }
>
Florian Weimer Feb. 11, 2019, 9:40 p.m. | #6
* Carlos O'Donell:

>> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);
>
> What prevents the compiler from optimizing away pd_result and using
> pd->result directly again?

The __free_tcb call acts as an implicit optimization barrier, like any
function call that doesn't have attributes that say otherwise.

> The actions of __free_tcb() and their consequences are not visible
> to the compiler.

Exactly.  The compiler cannot rematerialize the value from pd->result
because it has no way of knowing if the value is still the same.

Thanks,
Florian
Carlos O'Donell Feb. 12, 2019, 12:34 a.m. | #7
On 2/11/19 4:26 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> Why not move the probe up?
>>
>>  77   if (block)
>>  78     {
>>  79       /* If abstime is NULL we switch to asynchronous cancellation.  If we
>>  80          are cancelled then the thread we are waiting for must be marked as
>>  81          un-wait-ed for again.  */
>>  82       pthread_cleanup_push (cleanup, &pd->joinid);
>>  83 
>>  84       result = lll_wait_tid (pd->tid, abstime);
>>  85 
>>  86       pthread_cleanup_pop (0);
>>  87     }
>>  88 
>>
>> LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>> ^^^^
> 
> I think it should come after the stack has been freed.  It's explicitly
> documented as a return probe (“probe for pthread_join return”).

Can you tell the difference?

The compiler might move all sorts of things after the probe that will
happen before the final return insruction.

>> At this point the join is complete.
>>
>> We have the threadid (constant).
>>
>> We have the result (just returned from lll_wait_tid).
>>
>> We have pd->result (the thread returned).
>>
>> We aren't waiting for anything else?
> 
> Something might be interested in the fact that the stack is actually
> gone.  I'm not sure if this is a change we should make when backporting.

Nobody guarantees the stack is gone, and in fact with the stack cache,
and a future configurable stack cache, the cache may be around forever
until the program exits.

The probes are explicitly off the list of things which are ABI/API unless
we document them as such. Therefore I think they can reasonable change
in their exact triggering position. If you change anything else, like
semantics or arguments then you need a new name, and you can do that if
you want, because this master is development for glibc.
Carlos O'Donell Feb. 12, 2019, 12:42 a.m. | #8
On 2/11/19 4:40 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>>> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);
>>
>> What prevents the compiler from optimizing away pd_result and using
>> pd->result directly again?
> 
> The __free_tcb call acts as an implicit optimization barrier, like any
> function call that doesn't have attributes that say otherwise.

Sorry, yes, that's right, global memory accesses should not move around
that call, so I guess that means that the original pd_result store wont
move past __free_tcb() (and we don't do IPO/LTO yet).

>> The actions of __free_tcb() and their consequences are not visible
>> to the compiler.
> 
> Exactly.  The compiler cannot rematerialize the value from pd->result
> because it has no way of knowing if the value is still the same.

Yes, OK, let me review this again.
Carlos O'Donell Feb. 12, 2019, 1:31 a.m. | #9
On 2/11/19 4:22 PM, Florian Weimer wrote:
> nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]
> 
> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
> a register in the probe below:
> 
>       /* Free the TCB.  */
>       __free_tcb (pd);
>     }
>   else
>     pd->joinid = NULL;
> 
>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
> 
> However, at this point, the thread descriptor has been freed.  If the
> thread stack does not fit into the thread stack cache, the memory will
> have been unmapped, and the program will crash in the probe.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-02-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24211]
> 	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
> 	pd->result after the thread descriptor has been freed.
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..366feb376b 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -86,6 +86,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>        pthread_cleanup_pop (0);
>      }
>  
> +  void *pd_result = pd->result;

OK. Store result into pd_result local.

>    if (__glibc_likely (result == 0))
>      {
>        /* We mark the thread as terminated and as joined.  */

OK. In between the store and the access we have either not freed the stack,
or we have freed the stack but already read pd->result. The compiler might
generate two paths and optimize both, but can't optimize the read of global
memory past __free_tcb() which is in another CU.

> @@ -93,7 +94,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>  
>        /* Store the return value if the caller is interested.  */
>        if (thread_return != NULL)
> -	*thread_return = pd->result;
> +	*thread_return = pd_result;

OK. Micro-optimization.

>  
>        /* Free the TCB.  */
>        __free_tcb (pd);
> @@ -101,7 +102,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>    else
>      pd->joinid = NULL;
>  
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);

OK. Read of pd_result is always valid regardless of stack state.

>  
>    return result;
>  }
>
Andreas Schwab Feb. 12, 2019, 8:23 a.m. | #10
On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
> a register in the probe below:
>
>       /* Free the TCB.  */
>       __free_tcb (pd);
>     }
>   else
>     pd->joinid = NULL;
>
>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

That's not the original nor the new line.

Andreas.
Florian Weimer Feb. 12, 2019, 11:15 a.m. | #11
* Andreas Schwab:

> On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
>> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
>> a register in the probe below:
>>
>>       /* Free the TCB.  */
>>       __free_tcb (pd);
>>     }
>>   else
>>     pd->joinid = NULL;
>>
>>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
>
> That's not the original nor the new line.

Thanks, clearly I was too tired yesterday.

Do you think this patch is okay, or should I duplicate the probe?  But
even then, I won't be able to avoid the pd_return variable because
thread_return can be NULL, so a separate place for the return value is
needed.

Thanks,
Florian

Patch

diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index ecb78ffba5..45deba6a74 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -101,7 +101,7 @@  __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
   else
     pd->joinid = NULL;
 
-  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
+  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
 
   return result;
 }