Re: RFC: [PATCH] Fix setpriority calling __task_priority() for processes instead of threads.

[Samuel Thibault]

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