|
Message-ID: <20190909174943.GN9017@brightrain.aerifal.cx> 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.