| Message ID | 20141118205213.90CF92C3B21@topped-with-meat.com |
|---|---|
| State | Committed |
| Headers |
Received: (qmail 28917 invoked by alias); 18 Nov 2014 20:52:17 -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 28893 invoked by uid 89); 18 Nov 2014 20:52:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit From: Roland McGrath <roland@hack.frob.com> To: "GNU C. Library" <libc-alpha@sourceware.org> Subject: [PATCH roland/nptl] NPTL: Don't (re)validate sched_priority in pthread_create. Message-Id: <20141118205213.90CF92C3B21@topped-with-meat.com> Date: Tue, 18 Nov 2014 12:52:13 -0800 (PST) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=SvUDtp+0 c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=h_25X6ZIMyge40B-PgwA:9 a=CjuIK1q_8ugA:10 |
Commit Message
Roland McGrath
Nov. 18, 2014, 8:52 p.m. UTC
Now that pthread_attr_setschedparam validates the value, it should no longer be possible to see an invalid value in pthread_create. I've verified that in Linux the min/max values depend only on the policy selection and not on any dynamic factors that might have changed between pthread_attr_setschedparam and pthread_create. The one wrinkle is if the user called pthread_attr_setschedparam before pthread_attr_setschedpolicy and the sched_priority value was valid for the old policy but invalid for the new one. POSIX does not seem to provide for pthread_attr_setschedpolicy being able to return an error in this case, or for it to do something else like reset the sched_param bits to defaults on pthread_attr_setschedpolicy if the old bits don't make sense for the new policy. So in this situation, pthread_create failing with EINVAL appears to be the best we can do--and that's what we do now. But removing this check in pthread_create doesn't change that behavior observably: the thread will be started up and then create_thread will cancel it when the sched_setscheduler syscall fails, propagating the error code (presumably still EINVAL) back to pthread_create. So this user misuse of the API is detected with significantly more overhead, but I don't think that matters. Tested x86_64-linux-gnu. If nobody objects soon, I'll commit this. Thanks, Roland * nptl/pthread_create.c (__pthread_create_2_1): Don't try to validate the sched_priority value here. It was already checked when the user called pthread_attr_setschedparam.
Comments
On 11/18/2014 03:52 PM, Roland McGrath wrote: > Now that pthread_attr_setschedparam validates the value, it should no > longer be possible to see an invalid value in pthread_create. I've > verified that in Linux the min/max values depend only on the policy > selection and not on any dynamic factors that might have changed between > pthread_attr_setschedparam and pthread_create. > > The one wrinkle is if the user called pthread_attr_setschedparam before > pthread_attr_setschedpolicy and the sched_priority value was valid for the > old policy but invalid for the new one. POSIX does not seem to provide for > pthread_attr_setschedpolicy being able to return an error in this case, or > for it to do something else like reset the sched_param bits to defaults on > pthread_attr_setschedpolicy if the old bits don't make sense for the new > policy. So in this situation, pthread_create failing with EINVAL appears > to be the best we can do--and that's what we do now. But removing this > check in pthread_create doesn't change that behavior observably: the thread > will be started up and then create_thread will cancel it when the > sched_setscheduler syscall fails, propagating the error code (presumably > still EINVAL) back to pthread_create. So this user misuse of the API is > detected with significantly more overhead, but I don't think that matters. > > Tested x86_64-linux-gnu. > > If nobody objects soon, I'll commit this. Failure is certainly not what we want to optimize, and removing this code is certain to make pthread_create faster, and I agree that EINVAL is the best we can do here. What would like to know is: Have you tested the failure mode? If you haven't, please do, and if it works sanely, then LGTM. What is the impetus behind this change? Cheers, Carlos.
> What would like to know is: Have you tested the failure mode? > If you haven't, please do, and if it works sanely, then LGTM. I've written up a test for the expected EINVAL diagnosis. It depends on constructing a pthread_attr_t with invalid scheduling parameters, which AFAIK is possible only by using pthread_attr_setschedparam while the pthread_attr_t is set to one scheduling policy where a given struct sched_param is valid and then using pthread_attr_setschedpolicy to switch it to a policy where those values are invalid. This requires some assumptions about the policies and their parameters, but those are predictable enough at least for Linux. I'll (post and) put the new test in first, then verify this change doesn't regress it and commit. > What is the impetus behind this change? As I've been doing for some time now, I'm moving errant unconditional Linuxisms (such as direct INTERNAL_SYSCALL use) out of NPTL code. Mostly I'm just refactoring things to have Linuxisms in appropriate sysdeps files, but I'm not doing that blindly. This was a case where what the Linuxism was doing didn't seem worth keeping. Thanks, Roland
I've committed the change (after committing the new test and verifying it passes both before and after). If you'd like to review the one change remaining on roland/nptl (freshly rebased) soon, that would be great. After a day or two, I'll take silence as assent. :-) Thanks, Roland
On 11/19/2014 08:24 PM, Roland McGrath wrote: >> What would like to know is: Have you tested the failure mode? >> If you haven't, please do, and if it works sanely, then LGTM. > > I've written up a test for the expected EINVAL diagnosis. It depends > on constructing a pthread_attr_t with invalid scheduling parameters, > which AFAIK is possible only by using pthread_attr_setschedparam while > the pthread_attr_t is set to one scheduling policy where a given > struct sched_param is valid and then using pthread_attr_setschedpolicy > to switch it to a policy where those values are invalid. This > requires some assumptions about the policies and their parameters, but > those are predictable enough at least for Linux. Should this even be possible? > I'll (post and) put the new test in first, then verify this change > doesn't regress it and commit. Awesome, I couldn't ask for anything more. >> What is the impetus behind this change? > > As I've been doing for some time now, I'm moving errant unconditional > Linuxisms (such as direct INTERNAL_SYSCALL use) out of NPTL code. > Mostly I'm just refactoring things to have Linuxisms in appropriate > sysdeps files, but I'm not doing that blindly. This was a case where > what the Linuxism was doing didn't seem worth keeping. Makes perfect sense. c.
> > I've written up a test for the expected EINVAL diagnosis. It depends > > on constructing a pthread_attr_t with invalid scheduling parameters, > > which AFAIK is possible only by using pthread_attr_setschedparam while > > the pthread_attr_t is set to one scheduling policy where a given > > struct sched_param is valid and then using pthread_attr_setschedpolicy > > to switch it to a policy where those values are invalid. This > > requires some assumptions about the policies and their parameters, but > > those are predictable enough at least for Linux. > > Should this even be possible? I can't see how it could be ruled out. POSIX doesn't put any requirements on the order in which you call pthread_attr_setschedparam and pthread_attr_setschedpolicy.
On 11/20/2014 01:59 PM, Roland McGrath wrote: >>> I've written up a test for the expected EINVAL diagnosis. It depends >>> on constructing a pthread_attr_t with invalid scheduling parameters, >>> which AFAIK is possible only by using pthread_attr_setschedparam while >>> the pthread_attr_t is set to one scheduling policy where a given >>> struct sched_param is valid and then using pthread_attr_setschedpolicy >>> to switch it to a policy where those values are invalid. This >>> requires some assumptions about the policies and their parameters, but >>> those are predictable enough at least for Linux. >> >> Should this even be possible? > > I can't see how it could be ruled out. > POSIX doesn't put any requirements on the order in which you call > pthread_attr_setschedparam and pthread_attr_setschedpolicy. Good point. c.
--- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -605,6 +605,7 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg) } if (iattr->flags & ATTR_FLAG_SCHED_SET) + /* The values were validated in pthread_attr_setschedparam. */ memcpy (&pd->schedparam, &iattr->schedparam, sizeof (struct sched_param)); else if ((pd->flags & ATTR_FLAG_SCHED_SET) == 0) @@ -612,26 +613,6 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg) INTERNAL_SYSCALL (sched_getparam, scerr, 2, 0, &pd->schedparam); pd->flags |= ATTR_FLAG_SCHED_SET; } - - /* Check for valid priorities. */ - int minprio = INTERNAL_SYSCALL (sched_get_priority_min, scerr, 1, - iattr->schedpolicy); - int maxprio = INTERNAL_SYSCALL (sched_get_priority_max, scerr, 1, - iattr->schedpolicy); - if (pd->schedparam.sched_priority < minprio - || pd->schedparam.sched_priority > maxprio) - { - /* Perhaps a thread wants to change the IDs and if waiting - for this stillborn thread. */ - if (__builtin_expect (atomic_exchange_acq (&pd->setxid_futex, 0) - == -2, 0)) - lll_futex_wake (&pd->setxid_futex, 1, LLL_PRIVATE); - - __deallocate_stack (pd); - - retval = EINVAL; - goto out; - } } /* Pass the descriptor to the caller. */