[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 9 Sep 2019 13:49:43 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Subject: [PATCH] pthread: Fix bug that pthread_create may
cause priority inversion
On Mon, Sep 09, 2019 at 04:54:29PM +0200, Szabolcs Nagy wrote:
> * zhaohang (F) <zhaohang14@...wei.com> [2019-09-09 13:57:36 +0000]:
> > diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
> > index 7d4dc2e..ae08c0f 100644
> > --- a/src/thread/pthread_create.c
> > +++ b/src/thread/pthread_create.c
> > @@ -181,15 +181,8 @@ static int start(void *p)
> > {
> > struct start_args *args = p;
> > if (args->attr) {
> > - pthread_t self = __pthread_self();
> > - int ret = -__syscall(SYS_sched_setscheduler, self->tid,
> > - args->attr->_a_policy, &args->attr->_a_prio);
> > - if (a_swap(args->perr, ret)==-2)
> > - __wake(args->perr, 1, 1);
> > - if (ret) {
> > - self->detach_state = DT_DETACHED;
> > - __pthread_exit(0);
> > - }
> > + if (a_cas(args->perr, -1, -2) == -1)
> > + __wait(args->perr, 0, -2, 1);
> > }
> > __syscall(SYS_rt_sigprocmask, SIG_SETMASK, &args->sig_mask, 0, _NSIG/8);
> > __pthread_exit(args->start_func(args->start_arg));
> > @@ -367,10 +360,14 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
> > }
> >
> > if (attr._a_sched) {
> > - if (a_cas(&err, -1, -2)==-1)
> > - __wait(&err, 0, -2, 1);
> > - ret = err;
> > - if (ret) return ret;
> > + ret = -__syscall(SYS_sched_setscheduler, new->tid, attr._a_policy, &attr._a_prio);
> > + if (ret) {
> > + new->detach_state = DT_DETACHED;
> > + pthread_cancel(new);
> > + return ret;
>
> the child has the cancel signal blocked so it will never act on the signal.
Also, pthread_create should not pull in cancellation. Aside from being
unnecessary amounts of code that increases lots of costs in static
linking (for example, cancellable syscall paths have to be used),
there's no reason to use cancellation for something like this where
it's not trying to work with arbitrary application code, just a fixed
piece of code that admits explicit negotiation of how to continue.
> but even if that's fixed, the detached child may not get scheduled to
> handle the signal for a long time and will take up stack/tid resources.
That's the side issue I noted which my third patch fixes.
> i think Rich already has a solution that will deal with these issues.
Yes, sorry for not posting it sooner. Attached are the drafts that I
plan to push soon. (If you see something wrong and they've already
been pushed, just let me know and I'll fix it.) Patch 2 is the one
that addresses the issue reported here.
Rich
View attachment "0001-fix-unsynchronized-decrement-of-thread-count-on-pthr.patch" of type "text/plain" (1128 bytes)
View attachment "0002-set-explicit-scheduling-for-new-thread-from-calling-.patch" of type "text/plain" (3297 bytes)
View attachment "0003-synchronously-clean-up-pthread_create-failure-due-to.patch" of type "text/plain" (3259 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
