Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Hi, On Wed, Aug 31, 2016 at 02:16:12PM +0200, Samuel Thibault wrote: > Linux' notion of nice values is already not > really POSIX for root :) (POSIX doesn't define negative nice values). It's been a while; but I'm almost confident that according to my last reading of the POSIX man pages, the Linux implementation is actually perfectly correct -- the POSIX description is just very confusing... Note that this is a very old known issue, and there was previous discussion. (Which I'm too lazy to try digging up...) My conclusion was that trying to implement it correctly on top of the existing Mach interfaces would be tricky, for no real benefit -- we should just change the kernel interface instead to directly provide the semantics needed for POSIX behaviour. -antrik-
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Svante Signell, on Thu 01 Sep 2016 12:44:04 +0200, wrote:
> On Wed, 2016-08-31 at 14:16 +0200, Samuel Thibault wrote:
> > Svante Signell, on Wed 31 Aug 2016 14:10:22 +0200, wrote:
> > >
> > >
> > > I would expect values [-20,19] to be OK converted to [5,44] with
> > > #define NICE_TO_MACH_PRIORITY(nice) ((nice) + 25) from
> > > hurd/hurd/resource.h
> > > and
> > > #define invalid_pri(pri) (((pri) < 0) || ((pri) >= NRQS)), where NRQS = 50
> > > from kern/sched.h.
> > > to work.
> > Yes, but see the code changing thread priorities (since that's what is
> > posing problem)
>
> Digging a little further shows that tasks and threads are created with
> BASEPRI_USER=25 (NICE_TO_MACH_PRIORITY(0)=25). On the other hand kernel
> threads
> has a max priority of BASEPRI_SYSTEM=6 (NICE_TO_MACH_PRIORITY(-19)=6).
> (is -20 not possible as for Linux??)
These are just defaults, they could be changed to do -20, but POSIX
doesn't require that, so we don't need to care.
> In the code kern/task.c:task_priority() we have
> if (change_threads) {
> ...
> if (thread_priority(thread, priority, TRUE)!= KERN_SUCCESS) ret =
> KERN_FAILURE;
> and in kern/thread.c:thread_priority():
> if (priority < thread->max_priority) {ret = KERN_FAILURE;}
> irrespective of being root or not.
So that's the issue.
> Should code be added to check for root(how)
> in task-priority() and call kern/thread.c:thread_max_priority() for all
> threads
> in the task? Or are tasks created by root different in gnumach?
They aren't created differently. For mach, being root means having
a right on the privileged host port. Check how it is done in glibc's
sysdeps/mach/hurd/mlock.c and gnumach's vm/vm_user.c which checks for
ip_kotype(port) == IKOT_HOST_PRIV.
So the idea could be that we add to gnumach an RPC which additionally
takes a host port, and can thus check whether it's privileged or not.
Samuel
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
On Wed, 2016-08-31 at 14:16 +0200, Samuel Thibault wrote:
> Svante Signell, on Wed 31 Aug 2016 14:10:22 +0200, wrote:
> >
> >
> > I would expect values [-20,19] to be OK converted to [5,44] with
> > #define NICE_TO_MACH_PRIORITY(nice) ((nice) + 25) from hurd/hurd/resource.h
> > and
> > #define invalid_pri(pri) (((pri) < 0) || ((pri) >= NRQS)), where NRQS = 50
> > from kern/sched.h.
> > to work.
> Yes, but see the code changing thread priorities (since that's what is
> posing problem)
Digging a little further shows that tasks and threads are created with
BASEPRI_USER=25 (NICE_TO_MACH_PRIORITY(0)=25). On the other hand kernel threads
has a max priority of BASEPRI_SYSTEM=6 (NICE_TO_MACH_PRIORITY(-19)=6).
(is -20 not possible as for Linux??)
In the code kern/task.c:task_priority() we have
if (change_threads) {
...
if (thread_priority(thread, priority, TRUE)!= KERN_SUCCESS) ret = KERN_FAILURE;
and in kern/thread.c:thread_priority():
if (priority < thread->max_priority) {ret = KERN_FAILURE;}
irrespective of being root or not. Should code be added to check for root(how)
in task-priority() and call kern/thread.c:thread_max_priority() for all threads
in the task? Or are tasks created by root different in gnumach?
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Svante Signell, on Wed 31 Aug 2016 14:10:22 +0200, wrote: > On Wed, 2016-08-31 at 13:51 +0200, Samuel Thibault wrote: > > Svante Signell, on Wed 31 Aug 2016 13:42:58 +0200, wrote: > > > > > > > > > Which is the original bug then? > > You didn't say what application you are actually trying to fix, but the > > issue you have shown is that task_priority returns permission denied > > when change_threads is true (and I guessed you want that to work as > > normal user). I just said that the test was expected to have issues > > since the nice value is negative. > > The application is openntpd, which I'm working on porting and ntpd where this > call is made requires you to be root. Ok. Please always provide such information from the beginning, so we don't have to divine it. > I would expect values [-20,19] to be OK converted to [5,44] with > #define NICE_TO_MACH_PRIORITY(nice) ((nice) + 25) from hurd/hurd/resource.h > and > #define invalid_pri(pri) (((pri) < 0) || ((pri) >= NRQS)), where NRQS = 50 > from kern/sched.h. > to work. Yes, but see the code changing thread priorities (since that's what is posing problem) > In fact, according to invalid_pri(pri) the range could be [-26,25]. but that > would not make sense, right? It does not really matter. Linux' notion of nice values is already not really POSIX for root :) (POSIX doesn't define negative nice values). Samuel
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
On Wed, 2016-08-31 at 13:51 +0200, Samuel Thibault wrote: > Svante Signell, on Wed 31 Aug 2016 13:42:58 +0200, wrote: > > > > > > Which is the original bug then? > You didn't say what application you are actually trying to fix, but the > issue you have shown is that task_priority returns permission denied > when change_threads is true (and I guessed you want that to work as > normal user). I just said that the test was expected to have issues > since the nice value is negative. The application is openntpd, which I'm working on porting and ntpd where this call is made requires you to be root. > Now, it's not normal that a root process can't use a negative number. So > investigation is needed there. You are right: Working values are [0, 24], <0 returns EACCESS, >24 returns ESRCH (irrespective if being root or not). I would expect values [-20,19] to be OK converted to [5,44] with #define NICE_TO_MACH_PRIORITY(nice) ((nice) + 25) from hurd/hurd/resource.h and #define invalid_pri(pri) (((pri) < 0) || ((pri) >= NRQS)), where NRQS = 50 from kern/sched.h. to work. In fact, according to invalid_pri(pri) the range could be [-26,25]. but that would not make sense, right?
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Svante Signell, on Wed 31 Aug 2016 13:42:58 +0200, wrote: > On Wed, 2016-08-31 at 13:28 +0200, Samuel Thibault wrote: > > Svante Signell, on Wed 31 Aug 2016 13:24:34 +0200, wrote: > > > > Re-read your test again: it requests nice -19, i.e. something which is > > > > reserved to root. No wonder you are getting a permission denied. > > > Explain please, I get the same output also for running as root: > > > check_setpriority: can't set priority: Permission denied > > Then there is probably also a bug about not letting root do it. But > > that's *another* bug. > > Which is the original bug then? You didn't say what application you are actually trying to fix, but the issue you have shown is that task_priority returns permission denied when change_threads is true (and I guessed you want that to work as normal user). I just said that the test was expected to have issues since the nice value is negative. > Doing some more tests the priority can be set to > a number>=0, but not negative, irrespective of being root or not... Which is expected. So there is actually no bug for non-root. Now, it's not normal that a root process can't use a negative number. So investigation is needed there. Samuel
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
On Wed, 2016-08-31 at 13:28 +0200, Samuel Thibault wrote: > Svante Signell, on Wed 31 Aug 2016 13:24:34 +0200, wrote: > > > > > > > > Re-read your test again: it requests nice -19, i.e. something which is > > > reserved to root. No wonder you are getting a permission denied. > > Explain please, I get the same output also for running as root: > > check_setpriority: can't set priority: Permission denied > Then there is probably also a bug about not letting root do it. But > that's *another* bug. Which is the original bug then? Doing some more tests the priority can be set to a number>=0, but not negative, irrespective of being root or not...
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Svante Signell, on Wed 31 Aug 2016 13:24:34 +0200, wrote: > On Wed, 2016-08-31 at 12:58 +0200, Samuel Thibault wrote: > > Svante Signell, on Wed 31 Aug 2016 12:35:26 +0200, wrote: > > > > > > The attached patch changes this fixing the previous: > > > check_setpriority: can't set priority: Permission denied > > > > > > - prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 1); > > > + prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 0); > > Err, but then that makes change_threads false, i.e. the task_priority() > > call will not change the priorities of all threads of the task, which as > > you say is the POSIX behavior: > > So a task is equal to a thread, not a process? No, a task is a process. See the documentation: “The priority of a task is used only for creation of new threads; a new thread's priority is set to the enclosing task's priority.” > Leading to change_threads must be TRUE (to change all threads = > process?) It must be true, yes. See the source code: otherwise it doesn't touch existing threads. > > > According to setpriority(2) and POSIX the nice value should be > > > per-process not per-thread. > > So this "fix" your testcase by making the function not do what it is > > supposed to do... > > > > Re-read your test again: it requests nice -19, i.e. something which is > > reserved to root. No wonder you are getting a permission denied. > > Explain please, I get the same output also for running as root: > check_setpriority: can't set priority: Permission denied Then there is probably also a bug about not letting root do it. But that's *another* bug. > > The > > actual bug here is that task_priority seems not to check whether the > > priority is allowed when change_threads is false. > > Tracing the call from setpriority() results in KERN_FAILURE from kern/task.c: > > if (thread_priority(thread, priority, FALSE) != KERN_SUCCESS) > ret = KERN_FAILURE; > > which falls back to the implementation of thread_priority() in kern/thread.c That's what I said, yes: task_priority seems to not do things appropriately. Samuel
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
On Wed, 2016-08-31 at 12:58 +0200, Samuel Thibault wrote: > Svante Signell, on Wed 31 Aug 2016 12:35:26 +0200, wrote: > > > > The attached patch changes this fixing the previous: > > check_setpriority: can't set priority: Permission denied > > > > - prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 1); > > + prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 0); > Err, but then that makes change_threads false, i.e. the task_priority() > call will not change the priorities of all threads of the task, which as > you say is the POSIX behavior: So a task is equal to a thread, not a process? Leading to change_threads must be TRUE (to change all threads = process?) > > > > According to setpriority(2) and POSIX the nice value should be > > per-process not per-thread. > So this "fix" your testcase by making the function not do what it is > supposed to do... > > Re-read your test again: it requests nice -19, i.e. something which is > reserved to root. No wonder you are getting a permission denied. Explain please, I get the same output also for running as root: check_setpriority: can't set priority: Permission denied > The > actual bug here is that task_priority seems not to check whether the > priority is allowed when change_threads is false. Tracing the call from setpriority() results in KERN_FAILURE from kern/task.c: if (thread_priority(thread, priority, FALSE) != KERN_SUCCESS) ret = KERN_FAILURE; which falls back to the implementation of thread_priority() in kern/thread.c
Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.
Svante Signell, on Wed 31 Aug 2016 12:35:26 +0200, wrote: > The attached patch changes this fixing the previous: > check_setpriority: can't set priority: Permission denied > > - prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 1); > + prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 0); Err, but then that makes change_threads false, i.e. the task_priority() call will not change the priorities of all threads of the task, which as you say is the POSIX behavior: > According to setpriority(2) and POSIX the nice value should be > per-process not per-thread. So this "fix" your testcase by making the function not do what it is supposed to do... Re-read your test again: it requests nice -19, i.e. something which is reserved to root. No wonder you are getting a permission denied. The actual bug here is that task_priority seems not to check whether the priority is allowed when change_threads is false. Samuel
