[patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Thomas Gleixner]

sys_kill has a counterpart sys_tgkill which allows to send signals to
a particular thread. sys_rt_sigqueueinfo is lacking such a counterpart.

Aside of the asymetry it is a show stopper for migrating applications
from other unix-alike RTOSes.

The following patch series implements rt_tgsigqueueinfo and hooks it
up for x86.

Find below the raw documentation.

Thanks,

        tglx
----

NAME
       rt_tgsigqueueinfo - Send signal information to a signal to a thread

SYNOPSIS
       long sys_rt_tgsigqueueinfo (int tgid, int tid, int sig, siginfo_t *uinfo);

DESCRIPTION

       rt_tgsigqueueinfo sends signal sig information uinfo to the
       thread with the thread ID tid in the thread group tgid.  (By
       contrast, rt_sigqueueinfo(2) can only be used to send a signal
       info to a process (i.e., thread group) as a whole, and the
       signal will be delivered to an arbitrary thread within that
       process.)

RETURN VALUE

       rt_tgsigqueueinfo returns 0 on success; otherwise,
       rt_sigqueueinfo returns one of the errors listed in the
       "Errors" section.

ERRORS
       -EFAULT
              An invalid value for uinfo was specified.

       -EINVAL
              An invalid TID, TGID or signal was specified.

       -EPERM
              Permission denied.  For the required permissions,
              see rt_sigqueueinfo(2).

       -ESRCH
              No process with the specified thread ID and thread group
              ID exists. 

[patch 1/3] signals: split do_tkill

[Thomas Gleixner]

[-- Attachment #1: signals-split-do-tkill.patch --]
[-- Type: text/plain, Size: 2074 bytes --]

Split out the code from do_tkill to make it reusable by the follow up
patch which implements sys_rt_tgsigqueueinfo

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/signal.c |   29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Index: linux-2.6-tip/kernel/signal.c
===================================================================
--- linux-2.6-tip.orig/kernel/signal.c
+++ linux-2.6-tip/kernel/signal.c
@@ -2235,24 +2235,20 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, s
 	return kill_something_info(sig, &info, pid);
 }
 
-static int do_tkill(pid_t tgid, pid_t pid, int sig)
+static int
+do_send_specific(pid_t tgid, pid_t pid, int sig, struct siginfo *info)
 {
-	int error;
-	struct siginfo info;
 	struct task_struct *p;
 	unsigned long flags;
+	int error = -ESRCH;
 
-	error = -ESRCH;
-	info.si_signo = sig;
-	info.si_errno = 0;
-	info.si_code = SI_TKILL;
-	info.si_pid = task_tgid_vnr(current);
-	info.si_uid = current_uid();
+	info->si_pid = task_tgid_vnr(current);
+	info->si_uid = current_uid();
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
 	if (p && (tgid <= 0 || task_tgid_vnr(p) == tgid)) {
-		error = check_kill_permission(sig, &info, p);
+		error = check_kill_permission(sig, info, p);
 		/*
 		 * The null signal is a permissions and process existence
 		 * probe.  No signal is actually delivered.
@@ -2262,7 +2258,7 @@ static int do_tkill(pid_t tgid, pid_t pi
 		 * signal is private anyway.
 		 */
 		if (!error && sig && lock_task_sighand(p, &flags)) {
-			error = specific_send_sig_info(sig, &info, p);
+			error = specific_send_sig_info(sig, info, p);
 			unlock_task_sighand(p, &flags);
 		}
 	}
@@ -2271,6 +2267,17 @@ static int do_tkill(pid_t tgid, pid_t pi
 	return error;
 }
 
+static int do_tkill(pid_t tgid, pid_t pid, int sig)
+{
+	struct siginfo info;
+
+	info.si_signo = sig;
+	info.si_errno = 0;
+	info.si_code = SI_TKILL;
+
+	return do_send_specific(tgid, pid, sig, &info);
+}
+
 /**
  *  sys_tgkill - send signal to one specific thread
  *  @tgid: the thread group ID of the thread

[patch 2/3] signals: implement sys_rt_tgsigqueueinfo

[Thomas Gleixner]

[-- Attachment #1: signals-implement-rt-tgsigqueueinfo.patch --]
[-- Type: text/plain, Size: 3630 bytes --]

sys_kill has the per thread counterpart sys_tgkill. sigqueueinfo is
missing a thread directed counterpart. Such an interface is important
for migrating applications from other OSes which have the per thread
delivery implemented.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/compat.h |    2 ++
 include/linux/signal.h |    2 ++
 kernel/compat.c        |   11 +++++++++++
 kernel/signal.c        |   26 ++++++++++++++++++++++++++
 4 files changed, 41 insertions(+)

Index: linux-2.6-tip/include/linux/compat.h
===================================================================
--- linux-2.6-tip.orig/include/linux/compat.h
+++ linux-2.6-tip/include/linux/compat.h
@@ -208,6 +208,8 @@ int copy_siginfo_from_user32(siginfo_t *
 int copy_siginfo_to_user32(struct compat_siginfo __user *to, siginfo_t *from);
 int get_compat_sigevent(struct sigevent *event,
 		const struct compat_sigevent __user *u_event);
+long compat_sys_rt_tgsigqueueinfo(compat_pid_t tgid, compat_pid_t pid, int sig,
+				  struct compat_siginfo __user *uinfo);
 
 static inline int compat_timeval_compare(struct compat_timeval *lhs,
 					struct compat_timeval *rhs)
Index: linux-2.6-tip/include/linux/signal.h
===================================================================
--- linux-2.6-tip.orig/include/linux/signal.h
+++ linux-2.6-tip/include/linux/signal.h
@@ -235,6 +235,8 @@ static inline int valid_signal(unsigned 
 extern int next_signal(struct sigpending *pending, sigset_t *mask);
 extern int group_send_sig_info(int sig, struct siginfo *info, struct task_struct *p);
 extern int __group_send_sig_info(int, struct siginfo *, struct task_struct *);
+extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
+				 siginfo_t *info);
 extern long do_sigpending(void __user *, unsigned long);
 extern int sigprocmask(int, sigset_t *, sigset_t *);
 extern int show_unhandled_signals;
Index: linux-2.6-tip/kernel/compat.c
===================================================================
--- linux-2.6-tip.orig/kernel/compat.c
+++ linux-2.6-tip/kernel/compat.c
@@ -882,6 +882,17 @@ compat_sys_rt_sigtimedwait (compat_sigse
 
 }
 
+asmlinkage long
+compat_sys_rt_tgsigqueueinfo(compat_pid_t tgid, compat_pid_t pid, int sig,
+			     struct compat_siginfo __user *uinfo)
+{
+	siginfo_t info;
+
+	if (copy_siginfo_from_user32(&info, uinfo))
+		return -EFAULT;
+	return do_rt_tgsigqueueinfo(tgid, pid, sig, &info);
+}
+
 #ifdef __ARCH_WANT_COMPAT_SYS_TIME
 
 /* compat_time_t is a 32 bit "long" and needs to get converted. */
Index: linux-2.6-tip/kernel/signal.c
===================================================================
--- linux-2.6-tip.orig/kernel/signal.c
+++ linux-2.6-tip/kernel/signal.c
@@ -2327,6 +2327,32 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, 
 	return kill_proc_info(sig, &info, pid);
 }
 
+long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
+{
+	/* This is only valid for single tasks */
+	if (pid <= 0 || tgid <= 0)
+		return -EINVAL;
+
+	/* Not even root can pretend to send signals from the kernel.
+	   Nor can they impersonate a kill(), which adds source info.  */
+	if (info->si_code >= 0)
+		return -EPERM;
+	info->si_signo = sig;
+
+	return do_send_specific(tgid, pid, sig, info);
+}
+
+SYSCALL_DEFINE4(rt_tgsigqueueinfo, pid_t, tgid, pid_t, pid, int, sig,
+		siginfo_t __user *, uinfo)
+{
+	siginfo_t info;
+
+	if (copy_from_user(&info, uinfo, sizeof(siginfo_t)))
+		return -EFAULT;
+
+	return do_rt_tgsigqueueinfo(tgid, pid, sig, &info);
+}
+
 int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
 {
 	struct task_struct *t = current;

[patch 3/3] x86: hookup sys_rt_tgsigqueueinfo

[Thomas Gleixner]

[-- Attachment #1: x86-hookup-rt-tgsigqueueinfo.patch --]
[-- Type: text/plain, Size: 1994 bytes --]

Make the new sys_rt_tgsigqueueinfo available for x86.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/ia32/ia32entry.S          |    1 +
 arch/x86/include/asm/unistd_32.h   |    1 +
 arch/x86/include/asm/unistd_64.h   |    2 ++
 arch/x86/kernel/syscall_table_32.S |    1 +
 4 files changed, 5 insertions(+)

Index: linux-2.6-tip/arch/x86/ia32/ia32entry.S
===================================================================
--- linux-2.6-tip.orig/arch/x86/ia32/ia32entry.S
+++ linux-2.6-tip/arch/x86/ia32/ia32entry.S
@@ -828,4 +828,5 @@ ia32_sys_call_table:
 	.quad sys_dup3			/* 330 */
 	.quad sys_pipe2
 	.quad sys_inotify_init1
+	.quad compat_sys_rt_tgsigqueueinfo
 ia32_syscall_end:
Index: linux-2.6-tip/arch/x86/include/asm/unistd_32.h
===================================================================
--- linux-2.6-tip.orig/arch/x86/include/asm/unistd_32.h
+++ linux-2.6-tip/arch/x86/include/asm/unistd_32.h
@@ -338,6 +338,7 @@
 #define __NR_dup3		330
 #define __NR_pipe2		331
 #define __NR_inotify_init1	332
+#define __NR_rt_tgsigqueueinfo	333
 
 #ifdef __KERNEL__
 
Index: linux-2.6-tip/arch/x86/include/asm/unistd_64.h
===================================================================
--- linux-2.6-tip.orig/arch/x86/include/asm/unistd_64.h
+++ linux-2.6-tip/arch/x86/include/asm/unistd_64.h
@@ -653,6 +653,8 @@ __SYSCALL(__NR_dup3, sys_dup3)
 __SYSCALL(__NR_pipe2, sys_pipe2)
 #define __NR_inotify_init1			294
 __SYSCALL(__NR_inotify_init1, sys_inotify_init1)
+#define __NR_rt_tgsigqueueinfo			295
+__SYSCALL(__NR_rt_tgsigqueueinfo, sys_rt_tgsigqueueinfo)
 
 
 #ifndef __NO_STUBS
Index: linux-2.6-tip/arch/x86/kernel/syscall_table_32.S
===================================================================
--- linux-2.6-tip.orig/arch/x86/kernel/syscall_table_32.S
+++ linux-2.6-tip/arch/x86/kernel/syscall_table_32.S
@@ -332,3 +332,4 @@ ENTRY(sys_call_table)
 	.long sys_dup3			/* 330 */
 	.long sys_pipe2
 	.long sys_inotify_init1
+	.long sys_rt_tgsigqueueinfo

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Michael Kerrisk]

Thomas,

Please CC linux-api@ (http://thread.gmane.org/gmane.linux.ltp/5658/)
on patches that change the k-u interface...

Further comments below.

On 2/27/09, Thomas Gleixner <tglx@linutronix.de> wrote:
> sys_kill has a counterpart sys_tgkill which allows to send signals to
> a particular thread. sys_rt_sigqueueinfo is lacking such a counterpart.
>
> Aside of the asymetry it is a show stopper for migrating applications
> from other unix-alike RTOSes.
>
> The following patch series implements rt_tgsigqueueinfo and hooks it
> up for x86.
>
> Find below the raw documentation.

I haven't looked at this iteration of the patch, to check if it is
different from the version you posted a few months ago.  I assume it
isn't different, since you didn't mention any differences.  In that
case, there are still the questions I raised back then, so I'll just
repeat them now.

Back in October, I did some testing of this interface.  Two potential
issues that I saw, both of which relate to inconsistencies with
rt_sigqueueinfo():

1) With rt_siqueueinfo(), we can get the PID (TGID) of the sender,
which enables the receiver of the signal to know who the sender was,
and perhaps use that information to (for example) send a signal in the
other direction.

With rt_tgsigqueueinfo(), we don't quite have that ability: all that
the receiver gets is the TGID of the sender, not the TID.  This means
that we can't (for example) send a signal back to the precise thread
that signaled us. I'm not sure if this matters or not (but perhaps it
might when sender and receiver are in the same process?).  I'm also
not sure whether we want to do anything about this (i.e., extending
siginfo_t to include a si_tid field), but I wanted to point out this
assymetry w.r.t. to rt_sigqueueinfo(), in case you had not considered
it.

2) With rt_sigqueueinfo(), we can specify the si_pid and and si_uid
fields that should be sent to the receiver.  This is not possible with
rt_tgsigqueueinfo(), which always supplied the caller';s PID and UID
in the si_pid and si_uid fields sent to the receiver.  See the
following, created using my test programs below (the 111 & 222
arguments to t_*sigqueueinfo set the si_pid and si_uid fields in the
siginfo_t given to the *sigqueueinfo() syscall):

$ ./multithread_sig_receiver 0
Main: TGID = 11711; TID = 11711
Main: blocked signals
Thread 1: PID = 11711; TID = 11712
Thread 1: about to wait for signals
Thread 0: about to wait for signals
                        $ ./t_rt_sigqueueinfo 11711 44 1 -1 111 222
                        My PID is 11714
Thread 0: got signal: si_signo=44 si_code=-1 si_pid=111 si_uid=222
si_value.sival_int=1
                        $ ./t_rt_tgsigqueueinfo 11711 11712 44 1 -1 111 222
                        My PID is 11715
Thread 1: got signal: si_signo=44 si_code=-1 si_pid=11715 si_uid=1000
si_value.sival_int=1

The problem is the different code paths taken by
rt_tgsigqueueinfo()/tgkill() versus rt_sigqueueinfo():

sys_rt_sigqueueinfo()  sys_tgkill()                sys_rt_sigqueueinfo()
        |                   |                              |
        v                   v                              v
do_rt_sigqueueinfo()   do_tkill()                  kill_proc_info()
        |                   |                              |
        |                   v                              v
        +------------> do_send_specific()          kill_pid_info()
                            |                              |
                            v                              v
                       specific_send_sig_info()    group_send_sig_info()
                            |                              |
                            |                              v
                            |                      __group_send_sig_info()
                            |                              |
                            |                              v
                            +--------------------> send_signal()

And in do_send_specifc() there are the lines:

        info->si_pid = task_tgid_vnr(current);
        info->si_uid = current->uid;

I suspect this should be fixed to be consistent with sigqueueinfo().

Cheers,

Michael

=====

/*#* multithread_sig_receiver.c

   Copyright 2008, Linux Foundation;
   written by Michael Kerrisk <mtk.manpages@gmail.com>
*/
#define _GNU_SOURCE
#include <sys/syscall.h>
#include <ctype.h>
#include <unistd.h>
#include <signal.h>
#include <pthread.h>
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)

#define errExitEN(en, msg) \
                        do { errno = en; perror(msg); \
                             exit(EXIT_FAILURE); } while (0)

static sigset_t test_sigs;

struct thread_arg {
    int   thread_num;
    char *argv_string;
};

static pid_t
gettid(void)
{
    return syscall(SYS_gettid);
}

#if 0
static void
handler(int sig, siginfo_t *si, void *uc)
{
    printf("PID = %ld; TID = %ld; caught signal %d: \n",
            (long) getpid(), (long) gettid(), sig);
    printf("%p\n", si);
    printf("si_signo=%d ", si->si_signo);
    printf("si_code=%d ", si->si_code);
    printf("si_pid=%ld ", (long) si->si_pid);
    printf("si_uid=%ld ", (long) si->si_uid);
    printf("si_value.sival_int=%d\n", si->si_value.sival_int);
}
#endif

static void
wait_for_signals(int tnum)
{
    siginfo_t si;

    printf("Thread %d: about to wait for signals\n", tnum);

    for (;;) {
        if (sigwaitinfo(&test_sigs, &si) == -1)
            errExit("sigwaitinfo");
        printf("Thread %d: got signal: ", tnum);
        printf("si_signo=%d ", si.si_signo);
        printf("si_code=%d ", si.si_code);
        printf("si_pid=%ld ", (long) si.si_pid);
        printf("si_uid=%ld ", (long) si.si_uid);
        printf("si_value.sival_int=%d\n", si.si_value.sival_int);
    }
} /* wait_for_signals */


static void *
threadFunc(void *arg)
{
    struct thread_arg *ta = (struct thread_arg *) arg;
    int nsecs;

    printf("Thread %d: PID = %ld; TID = %ld\n", ta->thread_num,
            (long) getpid(), (long) gettid());

    nsecs = atoi(ta->argv_string);

    if (nsecs > 0) {
        printf("Thread %d: sleeping\n", ta->thread_num);
        sleep(nsecs);
        printf("Thread %d: finished sleeping\n", ta->thread_num);
    }

    wait_for_signals(ta->thread_num);
    return NULL;        /* NOTREACHED */
} /* threadFunc */


#define MAX_ARGS 1000

int
main(int argc, char *argv[])
{
    pthread_t t;
    int s, j;
//    struct sigaction sa;
    struct thread_arg ta[MAX_ARGS];
    sigset_t new;

    if (argc < 2) {
        fprintf(stderr, "Usage: %s <nsecs>...\n", argv[0]);
        fprintf(stderr, "Each <nsecs> argument causes a separate "
                "thread to be created;\n");
        fprintf(stderr, "The thread sleeps <nsecs> seconds before "
                "accepting signals with sigwaitinfo()\n");

        fprintf(stderr, "\n");
        exit(EXIT_SUCCESS);
    }

    if (argc > MAX_ARGS) {
        fprintf(stderr, "Too many arguments\n");
        exit(EXIT_SUCCESS);
    }

    sigemptyset(&test_sigs);
    for (s = 1; s < NSIG; s++)
        if (s != SIGINT && s != SIGQUIT && s != SIGTERM)
            sigaddset(&test_sigs, s);

    sigemptyset(&new);
    if (sigprocmask(SIG_SETMASK, &new, NULL) == -1)
        errExit("sigprocmask");

#if 0
    for (s = 1; s < NSIG; s++) {
        if (sigismember(&test_sigs, s)) {
            sa.sa_flags = SA_SIGINFO;
            sa.sa_sigaction = handler;
            sigemptyset(&sa.sa_mask);
            if (sigaction(s, &sa, NULL) == -1 && errno != EINVAL)
                errExit("sigaction");
        }
    }
    printf("Established handler for signals\n");
#endif

    printf("Main: TGID = %ld; TID = %ld\n",
            (long) getpid(), (long) gettid());

    s = pthread_sigmask(SIG_SETMASK, &test_sigs, NULL);
    if (s != 0)
        errExit("pthread_sigmask");
    printf("Main: blocked signals\n");

    for (j = 0; j + 1 < argc; j++) {
        ta[j].thread_num = j + 1;
        ta[j].argv_string = argv[j + 1];

        s = pthread_create(&t, NULL, threadFunc, &ta[j]);
        if (s != 0)
            errExitEN(s, "pthread_create");
    }

    wait_for_signals(0);
} /* main */

=====

/*#* t_rt_tgsigqueueinfo.c

   Copyright 2008, Linux Foundation;
   written by Michael Kerrisk <mtk.manpages@gmail.com>
*/
#define _GNU_SOURCE
#include <unistd.h>
#include <sys/syscall.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <signal.h>
#include <sys/types.h>

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)

#if defined(__i386__)
#define SYS_rt_tgsigqueueinfo 333
#endif

static int
rt_tgsigqueueinfo(pid_t tgid, pid_t tid, int sig, siginfo_t *si)
{
    return syscall(SYS_rt_tgsigqueueinfo, tgid, tid, sig, si);
}


int
main(int argc, char *argv[])
{
    pid_t tgid, tid;
    siginfo_t si;
    int sig, val;

    if (argc < 5) {
        fprintf(stderr, "Usage: %s <tgid> <tid> <sig> <val> "
                "[<si_code> [<si_pid [<si_uid>]]]\n", argv[0]);
        exit(EXIT_SUCCESS);
    }

    printf("My PID is %ld\n", (long) getpid());

    tgid = atoi(argv[1]);
    tid = atoi(argv[2]);
    sig = atoi(argv[3]);
    val = atoi(argv[4]);

    si.si_signo = sig + 1;
    si.si_code = (argc > 5) ? atoi(argv[5]) : SI_QUEUE;
    si.si_pid = (argc > 6) ? atoi(argv[6]) : getpid();
    si.si_uid = (argc > 7) ? atoi(argv[7]) : getuid();
    si.si_value.sival_int = val;

    /*
    printf("Sending: si.si_signo = %d, si.si_code = %d, "
            "si.si_pid = %ld, si.si_uid = %ld\n",
            si.si_signo, si.si_code, (long) si.si_pid, (long) si.si_uid);
    */
    if (rt_tgsigqueueinfo(tgid, tid, sig, &si) == -1)
        errExit("rt_tgsigqueueinfo");

    exit(EXIT_SUCCESS);
} /* main */

=====

/*#* t_rt_sigqueueinfo.c

   Copyright 2008, Linux Foundation;
   written by Michael Kerrisk <mtk.manpages@gmail.com>
*/
#define _GNU_SOURCE
#include <unistd.h>
#include <sys/syscall.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <signal.h>
#include <sys/types.h>

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)

static int
rt_sigqueueinfo(pid_t tgid, int sig, siginfo_t *si)
{
    return syscall(SYS_rt_sigqueueinfo, tgid, sig, si);
}


int
main(int argc, char *argv[])
{
    pid_t tgid;
    siginfo_t si;
    int sig, val;

    if (argc < 4) {
        fprintf(stderr, "Usage: %s <tgid> <sig> <val> "
                "[<si_code> [<si_pid [<si_uid>]]]\n", argv[0]);
        exit(EXIT_SUCCESS);
    }

    printf("My PID is %ld\n", (long) getpid());

    tgid = atoi(argv[1]);
    sig = atoi(argv[2]);
    val = atoi(argv[3]);

    si.si_signo = sig + 1;
    si.si_code = (argc > 4) ? atoi(argv[4]) : SI_QUEUE;
    si.si_pid = (argc > 5) ? atoi(argv[5]) : getpid();
    si.si_uid = (argc > 6) ? atoi(argv[6]) : getuid();
    si.si_value.sival_int = val;

    /*
    printf("Sending: si.si_signo = %d, si.si_code = %d, "
            "si.si_pid = %ld, si.si_uid = %ld\n",
            si.si_signo, si.si_code, (long) si.si_pid, (long) si.si_uid);
    */
    if (rt_sigqueueinfo(tgid, sig, &si) == -1)
        errExit("rt_sigqueueinfo");

    exit(EXIT_SUCCESS);
} /* main */

=====



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Thomas Gleixner]

Michael,

On Fri, 27 Feb 2009, Michael Kerrisk wrote:
> I haven't looked at this iteration of the patch, to check if it is
> different from the version you posted a few months ago.  I assume it
> isn't different, since you didn't mention any differences.  In that
> case, there are still the questions I raised back then, so I'll just
> repeat them now.

Right, no fundamental changes.
 
> Back in October, I did some testing of this interface.  Two potential
> issues that I saw, both of which relate to inconsistencies with
> rt_sigqueueinfo():
> 
> 1) With rt_siqueueinfo(), we can get the PID (TGID) of the sender,
> which enables the receiver of the signal to know who the sender was,
> and perhaps use that information to (for example) send a signal in the
> other direction.
> 
> With rt_tgsigqueueinfo(), we don't quite have that ability: all that
> the receiver gets is the TGID of the sender, not the TID.  This means
> that we can't (for example) send a signal back to the precise thread
> that signaled us. I'm not sure if this matters or not (but perhaps it
> might when sender and receiver are in the same process?).  I'm also
> not sure whether we want to do anything about this (i.e., extending
> siginfo_t to include a si_tid field), but I wanted to point out this
> assymetry w.r.t. to rt_sigqueueinfo(), in case you had not considered
> it.
> 
> 2) With rt_sigqueueinfo(), we can specify the si_pid and and si_uid
> fields that should be sent to the receiver.  This is not possible with
> rt_tgsigqueueinfo(), which always supplied the caller';s PID and UID
> in the si_pid and si_uid fields sent to the receiver.  See the
> following, created using my test programs below (the 111 & 222
> arguments to t_*sigqueueinfo set the si_pid and si_uid fields in the
> siginfo_t given to the *sigqueueinfo() syscall):

I can see your concern, but I have no strong opinion in either
direction. 

Roland ??

Thanks,

	tglx

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Michael Kerrisk]

Thomas,

On 2/27/09, Thomas Gleixner <tglx@linutronix.de> wrote:
> Michael,
>
> On Fri, 27 Feb 2009, Michael Kerrisk wrote:
>> I haven't looked at this iteration of the patch, to check if it is
>> different from the version you posted a few months ago.  I assume it
>> isn't different, since you didn't mention any differences.  In that
>> case, there are still the questions I raised back then, so I'll just
>> repeat them now.
>
> Right, no fundamental changes.
>
>> Back in October, I did some testing of this interface.  Two potential
>> issues that I saw, both of which relate to inconsistencies with
>> rt_sigqueueinfo():
>>
>> 1) With rt_siqueueinfo(), we can get the PID (TGID) of the sender,
>> which enables the receiver of the signal to know who the sender was,
>> and perhaps use that information to (for example) send a signal in the
>> other direction.
>>
>> With rt_tgsigqueueinfo(), we don't quite have that ability: all that
>> the receiver gets is the TGID of the sender, not the TID.  This means
>> that we can't (for example) send a signal back to the precise thread
>> that signaled us. I'm not sure if this matters or not (but perhaps it
>> might when sender and receiver are in the same process?).  I'm also
>> not sure whether we want to do anything about this (i.e., extending
>> siginfo_t to include a si_tid field), but I wanted to point out this
>> assymetry w.r.t. to rt_sigqueueinfo(), in case you had not considered
>> it.
>>
>> 2) With rt_sigqueueinfo(), we can specify the si_pid and and si_uid
>> fields that should be sent to the receiver.  This is not possible with
>> rt_tgsigqueueinfo(), which always supplied the caller';s PID and UID
>> in the si_pid and si_uid fields sent to the receiver.  See the
>> following, created using my test programs below (the 111 & 222
>> arguments to t_*sigqueueinfo set the si_pid and si_uid fields in the
>> siginfo_t given to the *sigqueueinfo() syscall):
>
> I can see your concern, but I have no strong opinion in either
> direction.
>
> Roland ??

The more I think about these two points, the more it seems to me they
both should be fixed.  Roland seems to concur on at least the second
point.  (Probably Ulrich should also be CCed for input -- done now.)
With respect to the first point, it seems to me reasonably likely that
there would be use cases where the receiving thread wants to know the
thread ID of the sender -- especially when sender and receiver are in
the same process.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Roland McGrath]

> With respect to the first point, it seems to me reasonably likely that
> there would be use cases where the receiving thread wants to know the
> thread ID of the sender -- especially when sender and receiver are in
> the same process.

But expecting si_pid to play that role is bizarre.  It's never what any
POSIX-like program would do, both since POSIX says si_pid is a process ID,
and because there are no POSIX-like interfaces at all that use Linux TIDs
to refer to threads.

Wanting this only seems plausible within one process.  In that case, sender
and recipient know they share memory.  The normal thing to do (and what
POSIX applications will do) is to store that info somewhere pointed to by
the sigval.


Thanks,
Roland

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Michael Kerrisk]

On 2/28/09, Roland McGrath <roland@redhat.com> wrote:
>> With respect to the first point, it seems to me reasonably likely that
>> there would be use cases where the receiving thread wants to know the
>> thread ID of the sender -- especially when sender and receiver are in
>> the same process.
>
> But expecting si_pid to play that role is bizarre.

Oh -- I wasn't suggeting that si_pid do that task; I was instead
wondering if we needed an additional si_threadid field (or some such).

> It's never what any
> POSIX-like program would do, both since POSIX says si_pid is a process ID,
> and because there are no POSIX-like interfaces at all that use Linux TIDs
> to refer to threads.

(Nod.)

> Wanting this only seems plausible within one process.

We are in agreement.

> In that case, sender
> and recipient know they share memory.  The normal thing to do (and what
> POSIX applications will do) is to store that info somewhere pointed to by
> the sigval.

Good point.  That would be another of dealing with point I wondered
about, and probably better than my idea.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Roland McGrath]

I don't see any rationale for rt_tgsigqueueinfo and rt_sigqueueinfo to
differ in their treatment of si_pid/si_uid (whatever that is).  It just
seems like common sense that they would match.

Oleg and/or Sukadev have some patches floating around (maybe all in -mm?)
that relate to setting those.

I notice that POSIX says that si_pid and si_uid have reliable values
whenever si_code <= 0 (what we call SI_FROMUSER() in asm/siginfo.h).
(POSIX only has sigqueue() and kill() et al to send these, so a POSIX
application never explicitly supplies the values.  libc/libpthread do.
The {t,tg,}kill syscalls all set si_pid to tgid already.)
This means a POSIX-conformant application might check si_pid and si_uid and
rely on them not being forged by some other process/user.  Firstly this
means that si_pid must be the POSIX PID, i.e. tgid, not the Linux TID
(which is not a useful value in POSIX interfaces).  Secondly it means the
kernel should guarantee the correctness of these values (at least when
crossing processes, might as well do always).

I don't recall if the pending changes already use tgid, I think they do.

I think what both calls should do is set si_pid to tgid and si_uid to uid
whenever SI_FROMUSER().  This satisfies the POSIX and security trust
concern, and makes them uniform.  (In Sukadev's version, what si_pid value
they fill in here depends on pid_ns details of sender and recipient.)

Vis a vis Sukadev's changes, I also notice that si_uid ought to be
translated for the recipient user_ns.


Thanks,
Roland

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Thomas Gleixner]

Roland,

On Thu, 26 Feb 2009, Roland McGrath wrote:

> I don't see any rationale for rt_tgsigqueueinfo and rt_sigqueueinfo to
> differ in their treatment of si_pid/si_uid (whatever that is).  It just
> seems like common sense that they would match.
> 
> Oleg and/or Sukadev have some patches floating around (maybe all in -mm?)
> that relate to setting those.

You mean having the same logic as we have in {t,tg,}kill syscalls for
rt_sigqueueinfo as well ?

Is there anything else which stands in the way of getting the
rt_tgsigqueueinfo interface merged ?

Thanks,

	tglx

Re: [patch 0/3] add rt_tgsigqueueinfo syscall [RESEND]

[Roland McGrath]

> > I don't see any rationale for rt_tgsigqueueinfo and rt_sigqueueinfo to
> > differ in their treatment of si_pid/si_uid (whatever that is).  It just
> > seems like common sense that they would match.
> > 
> > Oleg and/or Sukadev have some patches floating around (maybe all in -mm?)
> > that relate to setting those.
> 
> You mean having the same logic as we have in {t,tg,}kill syscalls for
> rt_sigqueueinfo as well ?

I mean just what I said: rt_tgsigqueueinfo's treatment of the siginfo_t
should match rt_sigqueueinfo's.  Make it match rt_sigqueueinfo today and
make sure that if rt_sigqueueinfo changes, then rt_tgsigqueueinfo will
change to match.

> Is there anything else which stands in the way of getting the
> rt_tgsigqueueinfo interface merged ?

I don't see any problem.  But you should at least make sure that Ulrich
likes the syscall interface for implementing pthread_sigqueue or whatever
it will be.


Thanks,
Roland