[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-kernel
Subject:    Re: [PATCH] Fix queued SIGIO
From:       Jamie Lokier <lk () tantalophile ! demon ! co ! uk>
Date:       2000-09-18 18:56:58
[Download RAW message or body]

Linus, please think this over before applying Andi's patch.

Andi Kleen wrote:
> The problem is really that SI_SIGIO is negative, but it should be positive
> to make SI_FROMUSER return false on it. 

This is an old problem.  There was a thread on this topic last March.
Look for "accept() improvements for rt signals".

SI_SIGIO is not the only signal that's broken: SI_ASYNCIO, SI_TIMER and
SI_MESGQ have the same problem.  When those signals are used you'll be
adding more and more exceptions to the SI_FROMUSER macro.

(For example the POSIX timers patch actually does exactly the same as
Andi's patch, but for SI_TIMER instead of SI_SIGIO).

It's obvious that SI_SIGIO, SI_ASYNCIO, SI_TIMER and SI_MESGQ should all
return false from SI_FROMUSER, assuming SI_FROMUSER is the right thing
to use in any case.

> Changing it would unfortunately break binary compatibility. This patch
> instead changes the definition of SI_FROMUSER/KERNEL to check explicitely
> for SI_SIGIO and make it appear like a kernel generated signal type. This
> prevents send_sig_info from looking at current .

That looks like major band-aid.  What does SI_FROMUSER mean anyway?

I looked it up on the web.  It doesn't appear in manual pages on the web
in general, and I didn't find it in Single Unix.  Let's investigate.
/*
 * si_code values
 * Digital reserves positive values for kernel-generated signals.
 */

Hmm...  Inherited from Digital Unix perhaps?
Let's try Digital UNIX V4.0F... /usr/include/sys/siginfo.h:

/* negative si_codes are reserved for user-generated signals */
#define SI_QUEUE        -1      /* sent by sigqueue */
#define SI_USER         0       /* sent by kill, sigsend, raise, etc. */
#define SI_NOINFO       1       /* unknown source */
#define SI_TIMER        0x10    /* sent by timer expiration */
#define SI_ASYNCIO      0x20    /* sent by AIO completion */
#define SI_MESGQ        0x40    /* sent by realtime mesq state transition */

#define SI_FROMUSER(siptr)      ((siptr)->si_code <= 0)
#define SI_FROMKERNEL(siptr)    ((siptr)->si_code > 0)

Looks like DEC got this right, Linux blindly copied some of the
definitions and made up some of the others (by setting SI_TIMER etc. to
negative values but keeping the same definition of SI_FROMUSER).

_Something_ of binary compatibility will have to be broken to fix this
problem.  Either change the SI_TIMER etc. signal numbers, or change the
SI_FROMUSER macro.  Both of can potentially break applications.
Changing SI_SIGINFO would obviously be the most serious.

I'd posit that changing SI_FROMUSER is the right fix.
But changed to include SI_TIMER, SI_MESGQ and SI_ASYNCIO as well.

Andi says:

> It'll break programs that try to send SI_SIGIO (=-5) signals from userspace,
> but I think that is ok.

Actually rt_sigqueueinfo has this test hard-coded in it:

	if (info.si_code >= 0)
		return -EPERM;

with a comment "not even root is allowed to send signals from the
kernel".  Changing SI_FROMUSER won't affect this.

Perhaps it should.  Do we consider SI_SIGIO, SI_TIMER etc. to be in the
"not even root is allowed" category or not?

have a nice day,
-- Jamie
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

[prev in list] [next in list] [prev in thread] [next in thread]