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

[Svante Signell]

Hi,

Attached is a patch for the call of __task_priority() in
sysdeps/mach/hurd/setpriority.c. According to setpriority(2) and POSIX the nice
value should be per-process not per-thread.
According to: http://pubs.opengroup.org/onlinepubs/009695399/functions/getpriori
ty.html
The nice value set with setpriority() shall be applied to the process. If the
process is multi-threaded, the nice value shall affect all system scope threads
in the process.

Unfortunately the call to __task_priority() is set with the change_threads
parameter true in gnumach/kern/task.c making a simple test program fail.
cat check_setpriority.c
#include 
#include 
#include 

int main ()
{
  if (setpriority(PRIO_PROCESS, 0, -20) == -1)
{
  warn("can't set priority");
  return -1;
}

  return 0;
}

The attached patch changes this fixing the previous:
check_setpriority: can't set priority: Permission denied


Thanks!--- a/sysdeps/mach/hurd/setpriority.c.orig	2016-02-18 18:54:00.0 +0100
+++ b/sysdeps/mach/hurd/setpriority.c		2016-08-31 10:05:20.0 +0200
@@ -54,7 +54,7 @@
 0, 1);
 	  }
 #else
-	  prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 1);
+	  prierr = __task_priority (task, NICE_TO_MACH_PRIORITY (prio), 0);
 #endif
 	  __mach_port_deallocate (__mach_task_self (), task);
 	  switch (prierr)

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

[Samuel Thibault]

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

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

[Svante Signell]

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.

[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

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

[Svante Signell]

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.

[Samuel Thibault]

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.

[Svante Signell]

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.

[Samuel Thibault]

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: [PATCH] drop the deprecated malloc/free hooks in hurd/mach-defpager

[Samuel Thibault]

Samuel Thibault, on Tue 16 Aug 2016 22:57:04 +0200, wrote:
> Thomas Schwinge, on Fri 05 Aug 2016 09:27:00 +0200, wrote:
> > On Mon, 30 May 2016 23:59:33 +0200, Samuel Thibault 
> >  wrote:
> > > Justus Winter, on Wed 18 May 2016 13:27:13 +0200, wrote:
> > > > As Richard said in #hurd, implement mlockall and MCL_FUTURE and just
> > > > use the default allocator.
> > > 
> > > Indeed.
> > 
> > (Conceptually ACK.)
> > 
> > > > or is the removal of that deprecated interface imminent?
> > 
> > After quite some discussion, it now actually has gotten removed in glibc
> > 2.24:
> 
> Debian will use 2.24 quite soon, so we need to implement something quite
> soon.

Debian is uploading 2.24, so we need to implement something now.

Samuel

[bug #48456] mig-generated user code does not destroy invalid reply

[Kalle Olavi Niemitalo]

Follow-up Comment #1, bug #48456 (project hurd):

I wrote: "This could perhaps be used for denial of service, if a long-lived
process calls a less-trusted one."

The exec server is a long-lived process, and it calls several RPC routines on
the FILE and OLDTASK passed to exec_exec, which can be called by anyone.  That
makes it vulnerable to the DoS.  I don't currently have a test case for that.

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/

problems with a subhurd

[Brent W. Baccala]

Aloha -

While testing the exec server, I setup a very minimalist subhurd using just
the most essential files, as opposed to copying the entire filesystem, and
uncovered a number of bugs.

I've refined the process into a shell script (attached) which creates the
subhurd on a ramdisk and then boots it.

At least three bugs become apparent:

1. /hurd/startup doesn't fallback on /bin/sh if it can't exec
/etc/hurd/runsystem.  This is easy to fix - just a missing increment.
Patch attached.

2. /hurd/startup naively assumes that SIGCHLD and waitpid() both work on
init (PID 1), but they don't.

I've been able to patch this up by introducing special cases to check for
HURD_PID_INIT in proc/wait.c's alert_parent (if PID is HURD_PID_INIT then
ignore the p_parent field and treat startup_proc as the parent) and
S_proc_wait (if we're called from procserver, make a special attempt to
reap(init_proc)), but I hesitate to submit this as a patch.  I'm not sure
how we want to do this.  Introduce special cases for init everywhere we've
got a problem with it?  Also, after fixing bug #1, this screws up startup's
attempt to start a new shell if the old one dies.  proc doesn't like having
a second init process started after the first one has died and been
reaped.  Maybe startup shouldn't try to start a second init, even if the
first one dies.  And startup still should have some way to detect if init
dies.

Our current setup is that PID 5 (ext2fs) runs first, then starts PID 2
(startup), which starts PID 1 (init).  Weird.  The cleanest solution, of
course, would be to have proc actually respect these parenting
relationships, then SIGCHLD and waitpid() would work normally.

3. Booting the subhurd, then running "halt -f" from its shell crashes the
parent Hurd.  Here's what the subhurd displays:

# halt -f
startup: notifying ext2fs.static pseudo-root of halt...done
startup: Killing pid 1
startup: Killing pid 3

...and here's what I see on the parent's console:

panic: thread_invoke: thread 9fcbfc80 has unexpected state 86
Debugger invoked: panic
Kernel Breakpoint trap, eip 0x810200f4
Stoppedat  Debugger+0x13:int$3
Debugger(810e015e,7,0,81a2fc90,9fcc6960)+0x13
panic(810e4740,810d9666,9fcbfc80,86,9fcbfc80)+0x79
state_panic(81051d8f,9bbb48dc,0,9fcbfc80,81029300)+0x17
thread_invoke(9fcbfc80,81029300,9b58f698,810292b3)+0x258
thread_run(81029300,9b58f698,0,81029375)+0x49
idle_thread_continue(9bb92568,81028a70,9c092fe4,0,9bd15488)+0x125
db>

Thread 0x9fcbfc80 is a kernel thread, and state 86 is TH_RUN | TH_SUSP |
TH_IDLE.  Not sure how it gets there.

agape
brent


subhurd
Description: Binary data


patch
Description: Binary data

Re: [PATCH] i386: Omit pmap workaround on i486 or later.

[Samuel Thibault]

Hello,

Masanori Ogino, on Sat 09 Jul 2016 17:00:35 +0900, wrote:
> (I couldn't find any page describing the i386 bug, though. Probably
> there were certain revisions with the bug but the others worked fine.)

See test_wp_bit() on Linux:

“It isn't supported on 386's and also on some strange 486's. All
586+'s are OK.”

> I will share the v2 patch after checking the code again.

I haven't seen such v2 yet?

Thanks,
Samuel

Re: problems with a subhurd

[Samuel Thibault]

Hello,

Brent W. Baccala, on Wed 31 Aug 2016 13:35:07 -1000, wrote:
>   Also, after fixing bug #1, this screws up startup's attempt to start a new
> shell if the old one dies.

Note that even on success, "try" is incremented, i.e. /bin/shd is tried
instead of the shell.

>  Maybe startup shouldn't
> try to start a second init, even if the first one dies.

No, we probably do want to run a shell in case the runsystem dies
unexpectedly.

Samuel