Message ID | 875ztqc96q.fsf@oldenburg2.str.redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 33079 invoked by alias); 11 Feb 2019 21:22:11 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 33063 invoked by uid 89); 11 Feb 2019 21:22:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com From: Florian Weimer <fweimer@redhat.com> To: Andreas Schwab <schwab@linux-m68k.org> Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211] References: <87k1i6cah6.fsf@oldenburg2.str.redhat.com> <87lg2m81yw.fsf@igel.home> Date: Mon, 11 Feb 2019 22:22:05 +0100 In-Reply-To: <87lg2m81yw.fsf@igel.home> (Andreas Schwab's message of "Mon, 11 Feb 2019 22:11:35 +0100") Message-ID: <875ztqc96q.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Florian Weimer
Feb. 11, 2019, 9:22 p.m. UTC
* 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.
Comments
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; > } >
* 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
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.
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; > } >
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.
* 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
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; }