[PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Andrey Vagin]

This patch set adds ability to choose a signal queue and
to read signals without dequeuing them.

Three new flags are added:
SFD_SHARED_QUEUE     -- reads will be from process-wide shared signal queue
SFD_PER_THREAD_QUEUE -- reads will be from per-thread signal queue
SFD_PEEK	     -- don't dequeue signals

Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>

-- 
1.7.11.7

[PATCH 1/3] signal: add a helper for dequeuing signals from a specified queue

[Andrey Vagin]

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 include/linux/sched.h | 13 ++++++++++++-
 kernel/signal.c       | 10 ++++++----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 206bb08..45871f1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2221,7 +2221,18 @@ extern void flush_signals(struct task_struct *);
 extern void __flush_signals(struct task_struct *);
 extern void ignore_signals(struct task_struct *);
 extern void flush_signal_handlers(struct task_struct *, int force_default);
-extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info);
+
+#define SIGQUEUE_SHARED		1
+#define SIGQUEUE_PRIVATE	2
+extern int do_dequeue_signal(struct task_struct *tsk,
+				sigset_t *mask, siginfo_t *info, int qmask);
+
+static inline int dequeue_signal(struct task_struct *tsk,
+					sigset_t *mask, siginfo_t *info)
+{
+	return do_dequeue_signal(tsk, mask, info,
+					SIGQUEUE_SHARED | SIGQUEUE_PRIVATE);
+}
 
 static inline int dequeue_signal_lock(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
 {
diff --git a/kernel/signal.c b/kernel/signal.c
index ac5f5e7..1412f6a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -600,15 +600,17 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
  *
  * All callers have to hold the siglock.
  */
-int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
+int do_dequeue_signal(struct task_struct *tsk, sigset_t *mask,
+					siginfo_t *info, int qmask)
 {
-	int signr;
+	int signr = 0;
 
 	/* We only dequeue private signals from ourselves, we don't let
 	 * signalfd steal them
 	 */
-	signr = __dequeue_signal(&tsk->pending, mask, info);
-	if (!signr) {
+	if (qmask & SIGQUEUE_PRIVATE)
+		signr = __dequeue_signal(&tsk->pending, mask, info);
+	if (!signr && (qmask & SIGQUEUE_SHARED)) {
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
 					 mask, info);
 		/*
-- 
1.7.11.7

[PATCH 1/3] signal: add a helper for dequeuing signals from a specified queue (v2)

[Andrey Vagin]

v2: export the symbol do_dequeue_signal

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 include/linux/sched.h | 13 ++++++++++++-
 kernel/signal.c       | 12 +++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 206bb08..45871f1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2221,7 +2221,18 @@ extern void flush_signals(struct task_struct *);
 extern void __flush_signals(struct task_struct *);
 extern void ignore_signals(struct task_struct *);
 extern void flush_signal_handlers(struct task_struct *, int force_default);
-extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info);
+
+#define SIGQUEUE_SHARED		1
+#define SIGQUEUE_PRIVATE	2
+extern int do_dequeue_signal(struct task_struct *tsk,
+				sigset_t *mask, siginfo_t *info, int qmask);
+
+static inline int dequeue_signal(struct task_struct *tsk,
+					sigset_t *mask, siginfo_t *info)
+{
+	return do_dequeue_signal(tsk, mask, info,
+					SIGQUEUE_SHARED | SIGQUEUE_PRIVATE);
+}
 
 static inline int dequeue_signal_lock(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
 {
diff --git a/kernel/signal.c b/kernel/signal.c
index ac5f5e7..9d1cfc6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -600,15 +600,17 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
  *
  * All callers have to hold the siglock.
  */
-int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
+int do_dequeue_signal(struct task_struct *tsk, sigset_t *mask,
+					siginfo_t *info, int qmask)
 {
-	int signr;
+	int signr = 0;
 
 	/* We only dequeue private signals from ourselves, we don't let
 	 * signalfd steal them
 	 */
-	signr = __dequeue_signal(&tsk->pending, mask, info);
-	if (!signr) {
+	if (qmask & SIGQUEUE_PRIVATE)
+		signr = __dequeue_signal(&tsk->pending, mask, info);
+	if (!signr && (qmask & SIGQUEUE_SHARED)) {
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
 					 mask, info);
 		/*
@@ -2479,7 +2481,7 @@ out:
 }
 
 EXPORT_SYMBOL(recalc_sigpending);
-EXPORT_SYMBOL_GPL(dequeue_signal);
+EXPORT_SYMBOL_GPL(do_dequeue_signal);
 EXPORT_SYMBOL(flush_signals);
 EXPORT_SYMBOL(force_sig);
 EXPORT_SYMBOL(send_sig);
-- 
1.7.11.7

[PATCH 2/3] signalfd: add ability to choose a private or shared queue

[Andrey Vagin]

This patch is added two flags SFD_SHARED_QUEUE and SFD_PER_THREAD_QUEUE
SFD_SHARED_QUEUE - read signals from a shared (process wide) queue
SFD_PER_THREAD_QUEUE - read signals from a per-thread queue

Without these flags or with both flags signals are read from both queues.

This functionality is required for dumping pending signals.

Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 fs/signalfd.c                 | 30 ++++++++++++++++++++++--------
 include/uapi/linux/signalfd.h |  4 ++++
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index c723b5d..8019ec9 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -61,13 +61,17 @@ static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 {
 	struct signalfd_ctx *ctx = file->private_data;
 	unsigned int events = 0;
+	unsigned int flags = file->f_flags;
 
 	poll_wait(file, &current->sighand->signalfd_wqh, wait);
 
 	spin_lock_irq(&current->sighand->siglock);
-	if (next_signal(&current->pending, &ctx->sigmask) ||
-	    next_signal(&current->signal->shared_pending,
-			&ctx->sigmask))
+	if ((flags & SFD_PER_THREAD_QUEUE) &&
+	     next_signal(&current->pending, &ctx->sigmask))
+		events |= POLLIN;
+
+	if ((flags & SFD_SHARED_QUEUE) &&
+	     next_signal(&current->signal->shared_pending, &ctx->sigmask))
 		events |= POLLIN;
 	spin_unlock_irq(&current->sighand->siglock);
 
@@ -189,13 +193,13 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
 }
 
 static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
-				int nonblock)
+				int nonblock, int qmask)
 {
 	ssize_t ret;
 	DECLARE_WAITQUEUE(wait, current);
 
 	spin_lock_irq(&current->sighand->siglock);
-	ret = dequeue_signal(current, &ctx->sigmask, info);
+	ret = do_dequeue_signal(current, &ctx->sigmask, info, qmask);
 	switch (ret) {
 	case 0:
 		if (!nonblock)
@@ -209,7 +213,7 @@ static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
 	add_wait_queue(&current->sighand->signalfd_wqh, &wait);
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
-		ret = dequeue_signal(current, &ctx->sigmask, info);
+		ret = do_dequeue_signal(current, &ctx->sigmask, info, qmask);
 		if (ret != 0)
 			break;
 		if (signal_pending(current)) {
@@ -240,16 +244,23 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 	struct signalfd_siginfo __user *siginfo;
 	int nonblock = file->f_flags & O_NONBLOCK;
 	bool raw = file->f_flags & SFD_RAW;
+	int qmask = 0;
 	ssize_t ret, total = 0;
 	siginfo_t info;
 
+	if (file->f_flags & SFD_PER_THREAD_QUEUE)
+		qmask |= SIGQUEUE_PRIVATE;
+	if (file->f_flags & SFD_SHARED_QUEUE)
+		qmask |= SIGQUEUE_SHARED;
+
 	count /= sizeof(struct signalfd_siginfo);
 	if (!count)
 		return -EINVAL;
 
 	siginfo = (struct signalfd_siginfo __user *) buf;
 	do {
-		ret = signalfd_dequeue(ctx, &info, nonblock);
+		ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
+
 		if (unlikely(ret <= 0))
 			break;
 
@@ -292,6 +303,8 @@ static const struct file_operations signalfd_fops = {
 	.llseek		= noop_llseek,
 };
 
+#define SFD_QUEUES (SFD_SHARED_QUEUE | SFD_PER_THREAD_QUEUE)
+
 SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 		size_t, sizemask, int, flags)
 {
@@ -302,7 +315,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW))
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW | SFD_QUEUES))
 		return -EINVAL;
 
 	if (sizemask != sizeof(sigset_t) ||
@@ -338,6 +351,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 			goto out;
 		}
 
+		file->f_flags |= (flags & SFD_QUEUES) ? : SFD_QUEUES;
 		file->f_flags |= flags & SFD_RAW;
 
 		fd_install(ufd, file);
diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
index bc31849..481b658 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -16,6 +16,10 @@
 #define SFD_CLOEXEC O_CLOEXEC
 #define SFD_NONBLOCK O_NONBLOCK
 #define SFD_RAW O_DIRECT
+/* Read signals from a shared (process wide) queue */
+#define SFD_SHARED_QUEUE O_DIRECTORY
+/* Read signals from a per-thread queue */
+#define SFD_PER_THREAD_QUEUE O_EXCL
 
 struct signalfd_siginfo {
 	__u32 ssi_signo;
-- 
1.7.11.7

Re: [PATCH 2/3] signalfd: add ability to choose a private or shared queue

[Oleg Nesterov]

AOn 01/22, Andrey Vagin wrote:
>
> This patch is added two flags SFD_SHARED_QUEUE and SFD_PER_THREAD_QUEUE
> SFD_SHARED_QUEUE

I can't believe I am going to blame the naming ;) but these 2 do not look
"symmetrical"....

SFD_PRIVATE_QUEUE looks a little bit better to me. At least it is simpler
to type. But I am fine either way.

Oleg.

Re: [PATCH 2/3] signalfd: add ability to choose a private or shared queue

[Michael Kerrisk (man-pages)]

On Thu, Feb 7, 2013 at 7:17 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> AOn 01/22, Andrey Vagin wrote:
>>
>> This patch is added two flags SFD_SHARED_QUEUE and SFD_PER_THREAD_QUEUE
>> SFD_SHARED_QUEUE
>
> I can't believe I am going to blame the naming ;) but these 2 do not look
> "symmetrical"....
>
> SFD_PRIVATE_QUEUE looks a little bit better to me. At least it is simpler
> to type. But I am fine either way.

At the risk of bikeshedding.... The point is that there are two
queues, the process-wide queue or the per-thread queue. "Private" does
not come into. And "Shared" is a little confusing. Adding those two
terms into the mix is just confusing: it doesn't fit with the
user-space view of signals.

I'd greatly prefer names that really say what these things are. Thus:

SFD_PROCESS_QUEUE + SFD_THREAD_QUEUE
or
SFD_PROCESS_WIDE_QUEUE + SFD_PER_THREAD_QUEUE
or
SFD_PROCESS_Q + SFD_THREAD_Q
or
[some consistent variation of the above]

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [PATCH 2/3] signalfd: add ability to choose a private or shared queue

[Oleg Nesterov]

On 02/08, Michael Kerrisk (man-pages) wrote:
>
> I'd greatly prefer names that really say what these things are. Thus:
>
> SFD_PROCESS_QUEUE + SFD_THREAD_QUEUE
> or
> SFD_PROCESS_WIDE_QUEUE + SFD_PER_THREAD_QUEUE
> or
> SFD_PROCESS_Q + SFD_THREAD_Q
> or
> [some consistent variation of the above]

I am fine either way.

But again, perhaps ioctl/ptrace is better.

Oleg.

[PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals

[Andrey Vagin]

If signalfd is created with the flag SFD_PEEK, it reads siginfo-s
without dequeuing signals.

For reading not first siginfo pread(fd, buf, size, pos) can be used,
where ppos is a sequence number of a signal in a queue.

This functionality is required for checkpointing pending signals.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Andrey Vagin <avagin@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 fs/signalfd.c                 | 52 ++++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/signalfd.h |  2 ++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 8019ec9..a160a7c 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -51,6 +51,47 @@ struct signalfd_ctx {
 	sigset_t sigmask;
 };
 
+static int peek_signal(struct sigpending *pending, sigset_t *mask,
+				siginfo_t *info, loff_t *pseq)
+{
+	struct sigqueue *q;
+	int ret = 0;
+
+	spin_lock_irq(&current->sighand->siglock);
+
+	list_for_each_entry(q, &pending->list, list) {
+		if (sigismember(mask, q->info.si_signo))
+			continue;
+
+		if ((*pseq)-- == 0) {
+			copy_siginfo(info, &q->info);
+			ret = info->si_signo;
+			break;
+		}
+	}
+
+	spin_unlock_irq(&current->sighand->siglock);
+
+	return ret;
+}
+
+static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
+				siginfo_t *info, loff_t *ppos, int queue_mask)
+{
+	loff_t seq = *ppos;
+	int signr = 0;
+
+	if (queue_mask & SIGQUEUE_PRIVATE)
+		signr = peek_signal(&current->pending,
+					&ctx->sigmask, info, &seq);
+	if (!signr && (queue_mask & SIGQUEUE_SHARED))
+		signr = peek_signal(&current->signal->shared_pending,
+					 &ctx->sigmask, info, &seq);
+	(*ppos)++;
+
+	return signr;
+}
+
 static int signalfd_release(struct inode *inode, struct file *file)
 {
 	kfree(file->private_data);
@@ -259,7 +300,10 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 
 	siginfo = (struct signalfd_siginfo __user *) buf;
 	do {
-		ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
+		if (file->f_flags & SFD_PEEK)
+			ret = signalfd_peek(ctx, &info, ppos, qmask);
+		else
+			ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
 
 		if (unlikely(ret <= 0))
 			break;
@@ -315,7 +359,8 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW | SFD_QUEUES))
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK |
+			SFD_RAW | SFD_PEEK | SFD_QUEUES))
 		return -EINVAL;
 
 	if (sizemask != sizeof(sigset_t) ||
@@ -352,7 +397,8 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 		}
 
 		file->f_flags |= (flags & SFD_QUEUES) ? : SFD_QUEUES;
-		file->f_flags |= flags & SFD_RAW;
+		file->f_flags |= flags & (SFD_RAW | SFD_PEEK);
+		file->f_mode |= FMODE_PREAD;
 
 		fd_install(ufd, file);
 	} else {
diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
index 481b658..24c5d2d 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -20,6 +20,8 @@
 #define SFD_SHARED_QUEUE O_DIRECTORY
 /* Read signals from a per-thread queue */
 #define SFD_PER_THREAD_QUEUE O_EXCL
+/* Read signals without removing them from a queue */
+#define SFD_PEEK O_APPEND
 
 struct signalfd_siginfo {
 	__u32 ssi_signo;
-- 
1.7.11.7

[PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrey Vagin]

If signalfd is created with the flag SFD_PEEK, it reads siginfo-s
without dequeuing signals.

For reading not first siginfo pread(fd, buf, size, pos) can be used,
where ppos / sizeof(signalfd_siginfo) is a sequence number of a signal
in a queue.

This functionality is required for checkpointing pending signals.

v2: * signals can be dumped only from one queue.
    * treat pos as offset in bytes, not in elements, so pos should be
      aligned to the size of signalfd_siginfo.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Andrey Vagin <avagin@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 fs/signalfd.c                 | 61 ++++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/signalfd.h |  2 ++
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 8019ec9..0da6a30 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -51,6 +51,47 @@ struct signalfd_ctx {
 	sigset_t sigmask;
 };
 
+static int peek_signal(struct sigpending *pending, sigset_t *mask,
+				siginfo_t *info, loff_t *pseq)
+{
+	struct sigqueue *q;
+	int ret = 0;
+
+	spin_lock_irq(&current->sighand->siglock);
+
+	list_for_each_entry(q, &pending->list, list) {
+		if (sigismember(mask, q->info.si_signo))
+			continue;
+
+		if ((*pseq)-- == 0) {
+			copy_siginfo(info, &q->info);
+			ret = info->si_signo;
+			break;
+		}
+	}
+
+	spin_unlock_irq(&current->sighand->siglock);
+
+	return ret;
+}
+
+static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
+				siginfo_t *info, loff_t *ppos, int queue_mask)
+{
+	loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
+	int signr = 0;
+
+	if (queue_mask & SIGQUEUE_PRIVATE)
+		signr = peek_signal(&current->pending,
+					&ctx->sigmask, info, &seq);
+	else if (queue_mask & SIGQUEUE_SHARED)
+		signr = peek_signal(&current->signal->shared_pending,
+					 &ctx->sigmask, info, &seq);
+	(*ppos) += sizeof(struct signalfd_siginfo);
+
+	return signr;
+}
+
 static int signalfd_release(struct inode *inode, struct file *file)
 {
 	kfree(file->private_data);
@@ -257,9 +298,15 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 	if (!count)
 		return -EINVAL;
 
+	if (*ppos % sizeof(struct signalfd_siginfo))
+		return -EINVAL;
+
 	siginfo = (struct signalfd_siginfo __user *) buf;
 	do {
-		ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
+		if (file->f_flags & SFD_PEEK)
+			ret = signalfd_peek(ctx, &info, ppos, qmask);
+		else
+			ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
 
 		if (unlikely(ret <= 0))
 			break;
@@ -315,7 +362,12 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW | SFD_QUEUES))
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK |
+			SFD_RAW | SFD_PEEK | SFD_QUEUES))
+		return -EINVAL;
+
+	/* SFD_PEEK can be used for one queue only */
+	if ((flags & SFD_PEEK) && ((flags & SFD_QUEUES) == SFD_QUEUES))
 		return -EINVAL;
 
 	if (sizemask != sizeof(sigset_t) ||
@@ -352,7 +404,10 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 		}
 
 		file->f_flags |= (flags & SFD_QUEUES) ? : SFD_QUEUES;
-		file->f_flags |= flags & SFD_RAW;
+		file->f_flags |= flags & (SFD_RAW | SFD_PEEK);
+
+		if (file->f_flags & SFD_PEEK)
+			file->f_mode |= FMODE_PREAD;
 
 		fd_install(ufd, file);
 	} else {
diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
index 481b658..24c5d2d 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -20,6 +20,8 @@
 #define SFD_SHARED_QUEUE O_DIRECTORY
 /* Read signals from a per-thread queue */
 #define SFD_PER_THREAD_QUEUE O_EXCL
+/* Read signals without removing them from a queue */
+#define SFD_PEEK O_APPEND
 
 struct signalfd_siginfo {
 	__u32 ssi_signo;
-- 
1.7.11.7

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrey Wagin]

2013/2/2 Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>:
>
> On Jan 30, 2013 8:05 AM, "Andrey Vagin" <avagin@openvz.org> wrote:
>>
>> If signalfd is created with the flag SFD_PEEK, it reads siginfo-s
>> without dequeuing signals.
>>
>> For reading not first siginfo pread(fd, buf, size, pos) can be used,
>> where ppos / sizeof(signalfd_siginfo) is a sequence number of a signal
>> in a queue.
>
> Andrey,
>
> Is it perhaps worth erroring (EINVAL) if  ((ppos % sizeof
> (signalfd_siginfo)) != 0) ?

Yes, it's already in the patch.

@@ -257,9 +298,15 @@ static ssize_t signalfd_read(struct file *file,
char __user *buf, size_t count,
        if (!count)
                return -EINVAL;

+       if (*ppos % sizeof(struct signalfd_siginfo))
+               return -EINVAL;
+


>
> Cheers,
>
> Michael
>
>> This functionality is required for checkpointing pending signals.
>>
>> v2: * signals can be dumped only from one queue.
>>     * treat pos as offset in bytes, not in elements, so pos should be
>>       aligned to the size of signalfd_siginfo.
>>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> Cc: David Howells <dhowells@redhat.com>
>> Cc: Dave Jones <davej@redhat.com>
>> Cc: Andrey Vagin <avagin@openvz.org>
>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>> Cc: Pavel Emelyanov <xemul@parallels.com>
>> CC: Cyrill Gorcunov <gorcunov@openvz.org>
>> Signed-off-by: Andrey Vagin <avagin@openvz.org>
>> ---
>>  fs/signalfd.c                 | 61
>> ++++++++++++++++++++++++++++++++++++++++---
>>  include/uapi/linux/signalfd.h |  2 ++
>>  2 files changed, 60 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/signalfd.c b/fs/signalfd.c
>> index 8019ec9..0da6a30 100644
>> --- a/fs/signalfd.c
>> +++ b/fs/signalfd.c
>> @@ -51,6 +51,47 @@ struct signalfd_ctx {
>>         sigset_t sigmask;
>>  };
>>
>> +static int peek_signal(struct sigpending *pending, sigset_t *mask,
>> +                               siginfo_t *info, loff_t *pseq)
>> +{
>> +       struct sigqueue *q;
>> +       int ret = 0;
>> +
>> +       spin_lock_irq(&current->sighand->siglock);
>> +
>> +       list_for_each_entry(q, &pending->list, list) {
>> +               if (sigismember(mask, q->info.si_signo))
>> +                       continue;
>> +
>> +               if ((*pseq)-- == 0) {
>> +                       copy_siginfo(info, &q->info);
>> +                       ret = info->si_signo;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       spin_unlock_irq(&current->sighand->siglock);
>> +
>> +       return ret;
>> +}
>> +
>> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
>> +                               siginfo_t *info, loff_t *ppos, int
>> queue_mask)
>> +{
>> +       loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
>> +       int signr = 0;
>> +
>> +       if (queue_mask & SIGQUEUE_PRIVATE)
>> +               signr = peek_signal(&current->pending,
>> +                                       &ctx->sigmask, info, &seq);
>> +       else if (queue_mask & SIGQUEUE_SHARED)
>> +               signr = peek_signal(&current->signal->shared_pending,
>> +                                        &ctx->sigmask, info, &seq);
>> +       (*ppos) += sizeof(struct signalfd_siginfo);
>> +
>> +       return signr;
>> +}
>> +
>>  static int signalfd_release(struct inode *inode, struct file *file)
>>  {
>>         kfree(file->private_data);
>> @@ -257,9 +298,15 @@ static ssize_t signalfd_read(struct file *file, char
>> __user *buf, size_t count,
>>         if (!count)
>>                 return -EINVAL;
>>
>> +       if (*ppos % sizeof(struct signalfd_siginfo))
>> +               return -EINVAL;
>> +
>>         siginfo = (struct signalfd_siginfo __user *) buf;
>>         do {
>> -               ret = signalfd_dequeue(ctx, &info, nonblock, qmask);
>> +               if (file->f_flags & SFD_PEEK)
>> +                       ret = signalfd_peek(ctx, &info, ppos, qmask);
>> +               else
>> +                       ret = signalfd_dequeue(ctx, &info, nonblock,
>> qmask);
>>
>>                 if (unlikely(ret <= 0))
>>                         break;
>> @@ -315,7 +362,12 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user
>> *, user_mask,
>>         BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
>>         BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
>>
>> -       if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW | SFD_QUEUES))
>> +       if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK |
>> +                       SFD_RAW | SFD_PEEK | SFD_QUEUES))
>> +               return -EINVAL;
>> +
>> +       /* SFD_PEEK can be used for one queue only */
>> +       if ((flags & SFD_PEEK) && ((flags & SFD_QUEUES) == SFD_QUEUES))
>>                 return -EINVAL;
>>
>>         if (sizemask != sizeof(sigset_t) ||
>> @@ -352,7 +404,10 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user
>> *, user_mask,
>>                 }
>>
>>                 file->f_flags |= (flags & SFD_QUEUES) ? : SFD_QUEUES;
>> -               file->f_flags |= flags & SFD_RAW;
>> +               file->f_flags |= flags & (SFD_RAW | SFD_PEEK);
>> +
>> +               if (file->f_flags & SFD_PEEK)
>> +                       file->f_mode |= FMODE_PREAD;
>>
>>                 fd_install(ufd, file);
>>         } else {
>> diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
>> index 481b658..24c5d2d 100644
>> --- a/include/uapi/linux/signalfd.h
>> +++ b/include/uapi/linux/signalfd.h
>> @@ -20,6 +20,8 @@
>>  #define SFD_SHARED_QUEUE O_DIRECTORY
>>  /* Read signals from a per-thread queue */
>>  #define SFD_PER_THREAD_QUEUE O_EXCL
>> +/* Read signals without removing them from a queue */
>> +#define SFD_PEEK O_APPEND
>>
>>  struct signalfd_siginfo {
>>         __u32 ssi_signo;
>> --
>> 1.7.11.7
>>

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Oleg Nesterov]

Andrey, sorry for delay.

As for API, I leave this to you and Michael. Not that I like these
new flags, but I agree that pread() hack was not pretty too.

On 01/29, Andrey Vagin wrote:
> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
> +				siginfo_t *info, loff_t *ppos, int queue_mask)
> +{
> +	loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
> +	int signr = 0;
> +
> +	if (queue_mask & SIGQUEUE_PRIVATE)
> +		signr = peek_signal(&current->pending,
> +					&ctx->sigmask, info, &seq);
> +	else if (queue_mask & SIGQUEUE_SHARED)
> +		signr = peek_signal(&current->signal->shared_pending,
> +					 &ctx->sigmask, info, &seq);
> +	(*ppos) += sizeof(struct signalfd_siginfo);

Now that this can work even with normal read(), we will actually change
f_pos. Then perhaps signalfd_fops->llseek() should work too. But this
is minor...

Hmm. but since it works with read(), we shouldn't increment *ppos unless
signalfd_copyinfo() succeeds?

Btw, why do you pass seq by reference? Looks unneeded.

Oleg.

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrey Wagin]

2013/2/7 Oleg Nesterov <oleg@redhat.com>:
> Andrey, sorry for delay.
>
> As for API, I leave this to you and Michael. Not that I like these
> new flags, but I agree that pread() hack was not pretty too.
>
> On 01/29, Andrey Vagin wrote:
>> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
>> +                             siginfo_t *info, loff_t *ppos, int queue_mask)
>> +{
>> +     loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
>> +     int signr = 0;
>> +
>> +     if (queue_mask & SIGQUEUE_PRIVATE)
>> +             signr = peek_signal(&current->pending,
>> +                                     &ctx->sigmask, info, &seq);
>> +     else if (queue_mask & SIGQUEUE_SHARED)
>> +             signr = peek_signal(&current->signal->shared_pending,
>> +                                      &ctx->sigmask, info, &seq);
>> +     (*ppos) += sizeof(struct signalfd_siginfo);
>
> Now that this can work even with normal read(), we will actually change
> f_pos. Then perhaps signalfd_fops->llseek() should work too. But this
> is minor...

lseek works only if FMODE_LSEEK is set.

You have explained why read&lseek have strange semantics for SIGNALFD_PEEK.

>Damn. But after I wrote this email I realized that llseek() probably can't
> work. Because peek_offset/f_pos/whatever has to be shared with all processes
> which have this file opened.
>
> Suppose that the task forks after sys_signalfd(). Now if parent or child
> do llseek this affects them both. This is insane because signalfd is
> "strange" to say at least, fork/dup/etc inherits signalfd_ctx but not the
> "source" of the data.

So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
file->f_pos can be initialized to -1. read() returns EINVAL in this
case. In a man page we will write that signals can be dumped only with
help pread(). Is it overload or too ugly?

>
> Hmm. but since it works with read(), we shouldn't increment *ppos unless
> signalfd_copyinfo() succeeds?

No, we shouldn't.

>
> Btw, why do you pass seq by reference? Looks unneeded.

You are right. I created this code for reading signals from both
queues, but then we decided to forbid using SIGNALFD_PEEK for both
queues simultaneously.


Oleg, thank you for the comments. I'm waiting an answer on the
question and after that I'm going to send a final version.

>
> Oleg.
>

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Michael Kerrisk (man-pages)]

On Thu, Feb 7, 2013 at 10:13 PM, Andrey Wagin <avagin@gmail.com> wrote:
> 2013/2/7 Oleg Nesterov <oleg@redhat.com>:
>> Andrey, sorry for delay.
>>
>> As for API, I leave this to you and Michael. Not that I like these
>> new flags, but I agree that pread() hack was not pretty too.
>>
>> On 01/29, Andrey Vagin wrote:

[...]

>>Damn. But after I wrote this email I realized that llseek() probably can't
>> work. Because peek_offset/f_pos/whatever has to be shared with all processes
>> which have this file opened.
>>
>> Suppose that the task forks after sys_signalfd(). Now if parent or child
>> do llseek this affects them both. This is insane because signalfd is
>> "strange" to say at least, fork/dup/etc inherits signalfd_ctx but not the
>> "source" of the data.

(Good catch, Oleg.)

> So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
> file->f_pos can be initialized to -1. read() returns EINVAL in this
> case. In a man page we will write that signals can be dumped only with
> help pread(). Is it overload or too ugly?

>From an interface perspective I have no problem with limiting the API
to allow just pread(). If we later decide that there is some way that
the semantics using read() + lseek() could be sensible (which seems
unlikely), we could relax things and allow read().

[...]

> Oleg, thank you for the comments. I'm waiting an answer on the
> question and after that I'm going to send a final version.

It would be nice if the new patch series has a changelog of the
changes to date, and also includes a fairly detailed description of
the user-space API. Would that be possible?

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Oleg Nesterov]

On 02/08, Andrey Wagin wrote:
>
> 2013/2/7 Oleg Nesterov <oleg@redhat.com>:
> > Andrey, sorry for delay.
> >
> > As for API, I leave this to you and Michael. Not that I like these
> > new flags, but I agree that pread() hack was not pretty too.
> >
> > On 01/29, Andrey Vagin wrote:
> >> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
> >> +                             siginfo_t *info, loff_t *ppos, int queue_mask)
> >> +{
> >> +     loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
> >> +     int signr = 0;
> >> +
> >> +     if (queue_mask & SIGQUEUE_PRIVATE)
> >> +             signr = peek_signal(&current->pending,
> >> +                                     &ctx->sigmask, info, &seq);
> >> +     else if (queue_mask & SIGQUEUE_SHARED)
> >> +             signr = peek_signal(&current->signal->shared_pending,
> >> +                                      &ctx->sigmask, info, &seq);
> >> +     (*ppos) += sizeof(struct signalfd_siginfo);
> >
> > Now that this can work even with normal read(), we will actually change
> > f_pos. Then perhaps signalfd_fops->llseek() should work too. But this
> > is minor...
>
> lseek works only if FMODE_LSEEK is set.
>
> You have explained why read&lseek have strange semantics for SIGNALFD_PEEK.
>
> >Damn. But after I wrote this email I realized that llseek() probably can't
> > work. Because peek_offset/f_pos/whatever has to be shared with all processes
> > which have this file opened.

Yes. but I thought you decided to ignore this oddity ;)

> So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
> file->f_pos can be initialized to -1. read() returns EINVAL in this
> case. In a man page we will write that signals can be dumped only with
> help pread(). Is it overload or too ugly?

Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.

Oleg.

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Michael Kerrisk (man-pages)]

On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/08, Andrey Wagin wrote:
>>
>> 2013/2/7 Oleg Nesterov <oleg@redhat.com>:
>> > Andrey, sorry for delay.
>> >
>> > As for API, I leave this to you and Michael. Not that I like these
>> > new flags, but I agree that pread() hack was not pretty too.
>> >
>> > On 01/29, Andrey Vagin wrote:
>> >> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
>> >> +                             siginfo_t *info, loff_t *ppos, int queue_mask)
>> >> +{
>> >> +     loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
>> >> +     int signr = 0;
>> >> +
>> >> +     if (queue_mask & SIGQUEUE_PRIVATE)
>> >> +             signr = peek_signal(&current->pending,
>> >> +                                     &ctx->sigmask, info, &seq);
>> >> +     else if (queue_mask & SIGQUEUE_SHARED)
>> >> +             signr = peek_signal(&current->signal->shared_pending,
>> >> +                                      &ctx->sigmask, info, &seq);
>> >> +     (*ppos) += sizeof(struct signalfd_siginfo);
>> >
>> > Now that this can work even with normal read(), we will actually change
>> > f_pos. Then perhaps signalfd_fops->llseek() should work too. But this
>> > is minor...
>>
>> lseek works only if FMODE_LSEEK is set.
>>
>> You have explained why read&lseek have strange semantics for SIGNALFD_PEEK.
>>
>> >Damn. But after I wrote this email I realized that llseek() probably can't
>> > work. Because peek_offset/f_pos/whatever has to be shared with all processes
>> > which have this file opened.
>
> Yes. but I thought you decided to ignore this oddity ;)
>
>> So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
>> file->f_pos can be initialized to -1. read() returns EINVAL in this
>> case. In a man page we will write that signals can be dumped only with
>> help pread(). Is it overload or too ugly?
>
> Well. I do not know. Up to you and Michael.
>
> But honestly, I can't say this all looks really nice. And why do we
> need SIGNALFD_PEEK then?

It surely is no beauty. The hope is at least to make it less ugly than it was.

> Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> And much simpler/straightforward.
>
> But I am not going to argue.

I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Oleg Nesterov]

On 02/08, Michael Kerrisk (man-pages) wrote:
>
> On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Well. I do not know. Up to you and Michael.
> >
> > But honestly, I can't say this all looks really nice. And why do we
> > need SIGNALFD_PEEK then?
>
> It surely is no beauty. The hope is at least to make it less ugly than it was.

This is subjective, but I am not sure about "less" ;) Yes, we avoid the
magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
dequeue_signal plus other complications. And for what?

> > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> > And much simpler/straightforward.
> >
> > But I am not going to argue.
>
> I suppose I had wondered along similar lines, but in a slightly
> different direction: would the use of a /proc interface to get the
> queued signals make some sense?

(Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
 which dumps almost everything c/r needs without need to add a lot of
 cr code everywhere).

Perhaps, but I am not sure about the textual representation.

And to me, the best solution is the simplest solution. Please look
at the patch below. It is trivial. And we can also drop the SFD_RAW
patch in -mm.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
 	return error;
 }
 
+static int ptrace_peek_signal(struct task_struct *child,
+				unsigned long addr, siginfo_t __user *uinfo)
+{
+	siginfo_t info;
+	struct sigpending *pending;
+	int ret = -ESOMETHING;
+
+	pending = &child->pending;
+	if (addr & PTRACE_PEEK_SHARED) {
+		addr &= ~PTRACE_PEEK_SHARED;
+		pending = &child->signal->shared_pending;
+	}
+
+	spin_lock_irq(&child->sighand->siglock);
+	list_for_each_entry(q, &pending->list, list) {
+		if (!addr--) {
+			copy_siginfo(info, &q->info);
+			ret = 0;
+			break;
+		}
+	}
+	spin_lock_irq(&child->sighand->siglock);
+
+	if (!ret)
+		ret = copy_siginfo_to_user(uinfo, info);
+	if (!ret)
+		ret = __put_user(info, si_code);
+	return ret;
+}
 
 #ifdef PTRACE_SINGLESTEP
 #define is_singlestep(request)		((request) == PTRACE_SINGLESTEP)
@@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
 		ret = put_user(child->ptrace_message, datalp);
 		break;
 
+	case PTRACE_PEEKSIGNAL:
+		ret = ptrace_peek_signal(child, addr, datavp);
+		break;
+
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Michael Kerrisk (man-pages)]

On Sat, Feb 9, 2013 at 7:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/08, Michael Kerrisk (man-pages) wrote:
>>
>> On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > Well. I do not know. Up to you and Michael.
>> >
>> > But honestly, I can't say this all looks really nice. And why do we
>> > need SIGNALFD_PEEK then?
>>
>> It surely is no beauty. The hope is at least to make it less ugly than it was.
>
> This is subjective, but I am not sure about "less" ;) Yes, we avoid the
> magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
> dequeue_signal plus other complications. And for what?
>
>> > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
>> > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
>> > And much simpler/straightforward.
>> >
>> > But I am not going to argue.
>>
>> I suppose I had wondered along similar lines, but in a slightly
>> different direction: would the use of a /proc interface to get the
>> queued signals make some sense?
>
> (Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
>  which dumps almost everything c/r needs without need to add a lot of
>  cr code everywhere).
>
> Perhaps, but I am not sure about the textual representation.
>
> And to me, the best solution is the simplest solution. Please look
> at the patch below. It is trivial. And we can also drop the SFD_RAW
> patch in -mm.

Oleg,

This looks promising, but I am not sure I understand the user-space
API. Could you explain how it would look to (say) pull all per-thread
signals from user space?

Thanks,

Michael


> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
>         return error;
>  }
>
> +static int ptrace_peek_signal(struct task_struct *child,
> +                               unsigned long addr, siginfo_t __user *uinfo)
> +{
> +       siginfo_t info;
> +       struct sigpending *pending;
> +       int ret = -ESOMETHING;
> +
> +       pending = &child->pending;
> +       if (addr & PTRACE_PEEK_SHARED) {
> +               addr &= ~PTRACE_PEEK_SHARED;
> +               pending = &child->signal->shared_pending;
> +       }
> +
> +       spin_lock_irq(&child->sighand->siglock);
> +       list_for_each_entry(q, &pending->list, list) {
> +               if (!addr--) {
> +                       copy_siginfo(info, &q->info);
> +                       ret = 0;
> +                       break;
> +               }
> +       }
> +       spin_lock_irq(&child->sighand->siglock);
> +
> +       if (!ret)
> +               ret = copy_siginfo_to_user(uinfo, info);
> +       if (!ret)
> +               ret = __put_user(info, si_code);
> +       return ret;
> +}
>
>  #ifdef PTRACE_SINGLESTEP
>  #define is_singlestep(request)         ((request) == PTRACE_SINGLESTEP)
> @@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
>                 ret = put_user(child->ptrace_message, datalp);
>                 break;
>
> +       case PTRACE_PEEKSIGNAL:
> +               ret = ptrace_peek_signal(child, addr, datavp);
> +               break;
> +
>         case PTRACE_GETSIGINFO:
>                 ret = ptrace_getsiginfo(child, &siginfo);
>                 if (!ret)
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrew Vagin]

On Sat, Feb 09, 2013 at 11:53:04PM +0100, Michael Kerrisk (man-pages) wrote:
> On Sat, Feb 9, 2013 at 7:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 02/08, Michael Kerrisk (man-pages) wrote:
> >>
> >> On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> >
> >> > Well. I do not know. Up to you and Michael.
> >> >
> >> > But honestly, I can't say this all looks really nice. And why do we
> >> > need SIGNALFD_PEEK then?
> >>
> >> It surely is no beauty. The hope is at least to make it less ugly than it was.
> >
> > This is subjective, but I am not sure about "less" ;) Yes, we avoid the
> > magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
> > dequeue_signal plus other complications. And for what?
> >
> >> > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> >> > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> >> > And much simpler/straightforward.
> >> >
> >> > But I am not going to argue.
> >>
> >> I suppose I had wondered along similar lines, but in a slightly
> >> different direction: would the use of a /proc interface to get the
> >> queued signals make some sense?
> >
> > (Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
> >  which dumps almost everything c/r needs without need to add a lot of
> >  cr code everywhere).
> >
> > Perhaps, but I am not sure about the textual representation.
> >
> > And to me, the best solution is the simplest solution. Please look
> > at the patch below. It is trivial. And we can also drop the SFD_RAW
> > patch in -mm.
> 
> Oleg,
> 
> This looks promising, but I am not sure I understand the user-space
> API. Could you explain how it would look to (say) pull all per-thread
> signals from user space?
>

	siginfo_t *signals = malloc(...);
	...

	ret = ptrace(PTRACE_ATTACH, pid, NULL, NULL);
	if (ret == -1)
		goto err;

	for (i = 0; ; i++) {
		ret = ptrace(PTRACE_PEEKSIGNAL, pid, i, signals + i);
		if (ret == -1) {
			if (errno == ESOMETHING)
				break;
			else
				goto err_detach;
		}
	}
...
err_detach:
	ret = ptrace(PTRACE_DETACH, pid, NULL, NULL);
err:
...

For shared (per-process) signals one line should be changed:
 ret = ptrace(PTRACE_PEEKSIGNAL, pid, PTRACE_PEEK_SHARED + i, signals + i);

> Thanks,
> 
> Michael
> 
> 
> > --- x/kernel/ptrace.c
> > +++ x/kernel/ptrace.c
> > @@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
> >         return error;
> >  }
> >
> > +static int ptrace_peek_signal(struct task_struct *child,
> > +                               unsigned long addr, siginfo_t __user *uinfo)
> > +{
> > +       siginfo_t info;
> > +       struct sigpending *pending;
> > +       int ret = -ESOMETHING;
> > +
> > +       pending = &child->pending;
> > +       if (addr & PTRACE_PEEK_SHARED) {
> > +               addr &= ~PTRACE_PEEK_SHARED;
> > +               pending = &child->signal->shared_pending;
> > +       }
> > +
> > +       spin_lock_irq(&child->sighand->siglock);
> > +       list_for_each_entry(q, &pending->list, list) {
> > +               if (!addr--) {
> > +                       copy_siginfo(info, &q->info);
> > +                       ret = 0;
> > +                       break;
> > +               }
> > +       }
> > +       spin_lock_irq(&child->sighand->siglock);
> > +
> > +       if (!ret)
> > +               ret = copy_siginfo_to_user(uinfo, info);
> > +       if (!ret)
> > +               ret = __put_user(info, si_code);
> > +       return ret;
> > +}
> >
> >  #ifdef PTRACE_SINGLESTEP
> >  #define is_singlestep(request)         ((request) == PTRACE_SINGLESTEP)
> > @@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
> >                 ret = put_user(child->ptrace_message, datalp);
> >                 break;
> >
> > +       case PTRACE_PEEKSIGNAL:
> > +               ret = ptrace_peek_signal(child, addr, datavp);
> > +               break;
> > +
> >         case PTRACE_GETSIGINFO:
> >                 ret = ptrace_getsiginfo(child, &siginfo);
> >                 if (!ret)
> >
> 
> 
> 
> -- 
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Author of "The Linux Programming Interface"; http://man7.org/tlpi/
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Oleg Nesterov]

On 02/10, Andrew Vagin wrote:
>
> On Sat, Feb 09, 2013 at 11:53:04PM +0100, Michael Kerrisk (man-pages) wrote:
> >
> > This looks promising, but I am not sure I understand the user-space
> > API. Could you explain how it would look to (say) pull all per-thread
> > signals from user space?
> >
>
> 	siginfo_t *signals = malloc(...);
> 	...
>
> 	ret = ptrace(PTRACE_ATTACH, pid, NULL, NULL);

Yes. I assume this should not be a problem since c/r needs to ptrace
the task anyway?

> For shared (per-process) signals one line should be changed:
>  ret = ptrace(PTRACE_PEEKSIGNAL, pid, PTRACE_PEEK_SHARED + i, signals + i);

Yes. Except I'd say "PTRACE_PEEK_SHARED | i" to clarify that
PTRACE_PEEK_SHARED is "flag". Note that we can more flags, say,
PTRACE_PEEK_DEQUEUE which obviously means "dequeue signal" (but in
this case we should filter out SIGKILL/SIGSTOP).

And note that either way (I mean ptrace or signalfd) this can not
actually peek a signal which doesn't have the sigqueue (say,
SEND_SIG_FORCED). I guess this is not the problem, c/r can look
at /proc/pid/status#*Pnd aditionally.

OK, you seem to agree with this approach, I'll try to make the
working patch tomorrow. At least ptrace_peek_signal() needs another
is_compat argument for compat_ptrace_request().

Oleg.

> > > --- x/kernel/ptrace.c
> > > +++ x/kernel/ptrace.c
> > > @@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
> > >         return error;
> > >  }
> > >
> > > +static int ptrace_peek_signal(struct task_struct *child,
> > > +                               unsigned long addr, siginfo_t __user *uinfo)
> > > +{
> > > +       siginfo_t info;
> > > +       struct sigpending *pending;
> > > +       int ret = -ESOMETHING;
> > > +
> > > +       pending = &child->pending;
> > > +       if (addr & PTRACE_PEEK_SHARED) {
> > > +               addr &= ~PTRACE_PEEK_SHARED;
> > > +               pending = &child->signal->shared_pending;
> > > +       }
> > > +
> > > +       spin_lock_irq(&child->sighand->siglock);
> > > +       list_for_each_entry(q, &pending->list, list) {
> > > +               if (!addr--) {
> > > +                       copy_siginfo(info, &q->info);
> > > +                       ret = 0;
> > > +                       break;
> > > +               }
> > > +       }
> > > +       spin_lock_irq(&child->sighand->siglock);
> > > +
> > > +       if (!ret)
> > > +               ret = copy_siginfo_to_user(uinfo, info);
> > > +       if (!ret)
> > > +               ret = __put_user(info, si_code);
> > > +       return ret;
> > > +}
> > >
> > >  #ifdef PTRACE_SINGLESTEP
> > >  #define is_singlestep(request)         ((request) == PTRACE_SINGLESTEP)
> > > @@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
> > >                 ret = put_user(child->ptrace_message, datalp);
> > >                 break;
> > >
> > > +       case PTRACE_PEEKSIGNAL:
> > > +               ret = ptrace_peek_signal(child, addr, datavp);
> > > +               break;
> > > +
> > >         case PTRACE_GETSIGINFO:
> > >                 ret = ptrace_getsiginfo(child, &siginfo);
> > >                 if (!ret)
> > >
> >
> >
> >
> > --
> > Michael Kerrisk
> > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> > Author of "The Linux Programming Interface"; http://man7.org/tlpi/
> > _______________________________________________
> > CRIU mailing list
> > CRIU@openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrew Vagin]

On Sat, Feb 09, 2013 at 07:22:39PM +0100, Oleg Nesterov wrote:
> On 02/08, Michael Kerrisk (man-pages) wrote:
> >
> > On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > Well. I do not know. Up to you and Michael.
> > >
> > > But honestly, I can't say this all looks really nice. And why do we
> > > need SIGNALFD_PEEK then?
> >
> > It surely is no beauty. The hope is at least to make it less ugly than it was.
> 
> This is subjective, but I am not sure about "less" ;) Yes, we avoid the
> magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
> dequeue_signal plus other complications. And for what?
> 
> > > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> > > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> > > And much simpler/straightforward.
> > >
> > > But I am not going to argue.
> >
> > I suppose I had wondered along similar lines, but in a slightly
> > different direction: would the use of a /proc interface to get the
> > queued signals make some sense?
> 
> (Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
>  which dumps almost everything c/r needs without need to add a lot of
>  cr code everywhere).
> 
> Perhaps, but I am not sure about the textual representation.
> 
> And to me, the best solution is the simplest solution. Please look
> at the patch below. It is trivial. And we can also drop the SFD_RAW
> patch in -mm.

I'm agree with you and I will ask Andrew to drop the SFD_RAW patch.

Thanks.

> 
> Oleg.
> 
> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
>  	return error;
>  }
>  
> +static int ptrace_peek_signal(struct task_struct *child,
> +				unsigned long addr, siginfo_t __user *uinfo)
> +{
> +	siginfo_t info;
> +	struct sigpending *pending;
> +	int ret = -ESOMETHING;
> +
> +	pending = &child->pending;
> +	if (addr & PTRACE_PEEK_SHARED) {
> +		addr &= ~PTRACE_PEEK_SHARED;
> +		pending = &child->signal->shared_pending;
> +	}
> +
> +	spin_lock_irq(&child->sighand->siglock);
> +	list_for_each_entry(q, &pending->list, list) {
> +		if (!addr--) {
> +			copy_siginfo(info, &q->info);
> +			ret = 0;
> +			break;
> +		}
> +	}
> +	spin_lock_irq(&child->sighand->siglock);
> +
> +	if (!ret)
> +		ret = copy_siginfo_to_user(uinfo, info);
> +	if (!ret)
> +		ret = __put_user(info, si_code);
> +	return ret;
> +}
>  
>  #ifdef PTRACE_SINGLESTEP
>  #define is_singlestep(request)		((request) == PTRACE_SINGLESTEP)
> @@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
>  		ret = put_user(child->ptrace_message, datalp);
>  		break;
>  
> +	case PTRACE_PEEKSIGNAL:
> +		ret = ptrace_peek_signal(child, addr, datavp);
> +		break;
> +
>  	case PTRACE_GETSIGINFO:
>  		ret = ptrace_getsiginfo(child, &siginfo);
>  		if (!ret)
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Denys Vlasenko]

On Friday 08 February 2013 21:15, Michael Kerrisk (man-pages) wrote:
> >> >Damn. But after I wrote this email I realized that llseek() probably can't
> >> > work. Because peek_offset/f_pos/whatever has to be shared with all processes
> >> > which have this file opened.
> >
> > Yes. but I thought you decided to ignore this oddity ;)
> >
> >> So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
> >> file->f_pos can be initialized to -1. read() returns EINVAL in this
> >> case. In a man page we will write that signals can be dumped only with
> >> help pread(). Is it overload or too ugly?
> >
> > Well. I do not know. Up to you and Michael.
> >
> > But honestly, I can't say this all looks really nice. And why do we
> > need SIGNALFD_PEEK then?
> 
> It surely is no beauty. The hope is at least to make it less ugly than it was.
> 
> > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> > And much simpler/straightforward.
> >
> > But I am not going to argue.

ptrace interface might find some use in debuggers.
Not that any of them expressed such desires so far,
but just maybe.

However, it needs coding in C to read it,
which brings me to:


> I suppose I had wondered along similar lines, but in a slightly
> different direction: would the use of a /proc interface to get the
> queued signals make some sense?

I think that /proc interface beats adding magic flags and magic semantic
to [p]read.

It also has the benefit of being human-readable. You don't need
to write a special C program to "cat /proc/$$/foo".

Andrey, I know that it is hard to let go of the code you invested time
and efforts in creating. But this isn't the last patch, is it?
You will need to retrieve yet more data for process checkpointing.
When you start working on the next patch for it, consider trying
/proc approach.

-- 
vda

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Andrew Vagin]

On Mon, Feb 11, 2013 at 10:29:50AM +0100, Denys Vlasenko wrote:
> On Friday 08 February 2013 21:15, Michael Kerrisk (man-pages) wrote:
> > >> >Damn. But after I wrote this email I realized that llseek() probably can't
> > >> > work. Because peek_offset/f_pos/whatever has to be shared with all processes
> > >> > which have this file opened.
> > >
> > > Yes. but I thought you decided to ignore this oddity ;)
> > >
> > >> So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
> > >> file->f_pos can be initialized to -1. read() returns EINVAL in this
> > >> case. In a man page we will write that signals can be dumped only with
> > >> help pread(). Is it overload or too ugly?
> > >
> > > Well. I do not know. Up to you and Michael.
> > >
> > > But honestly, I can't say this all looks really nice. And why do we
> > > need SIGNALFD_PEEK then?
> > 
> > It surely is no beauty. The hope is at least to make it less ugly than it was.
> > 
> > > Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
> > > Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
> > > And much simpler/straightforward.
> > >
> > > But I am not going to argue.
> 
> ptrace interface might find some use in debuggers.
> Not that any of them expressed such desires so far,
> but just maybe.
> 
> However, it needs coding in C to read it,
> which brings me to:
> 
> 
> > I suppose I had wondered along similar lines, but in a slightly
> > different direction: would the use of a /proc interface to get the
> > queued signals make some sense?
> 
> I think that /proc interface beats adding magic flags and magic semantic
> to [p]read.
> 
> It also has the benefit of being human-readable. You don't need
> to write a special C program to "cat /proc/$$/foo".
> 
> Andrey, I know that it is hard to let go of the code you invested time
> and efforts in creating. But this isn't the last patch, is it?
> You will need to retrieve yet more data for process checkpointing.
> When you start working on the next patch for it, consider trying
> /proc approach.

I don't think that we need to convert siginfo into a human readable format
in kernel. For example siginfo with a negative si_code contains binary data.
A kernel already has got signalfd_siginfo and we can see, that it's not
good.
I think would be better to print siginfo in a human readable format from
userspace (e.g. gdb) and implement a generic interface in kernel (e.g.
ptrace).

> > -- 
> vda
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Denys Vlasenko]

On Mon, Feb 11, 2013 at 11:59 AM, Andrew Vagin <avagin@parallels.com> wrote:
>> > I suppose I had wondered along similar lines, but in a slightly
>> > different direction: would the use of a /proc interface to get the
>> > queued signals make some sense?
>>
>> I think that /proc interface beats adding magic flags and magic semantic
>> to [p]read.
>>
>> It also has the benefit of being human-readable. You don't need
>> to write a special C program to "cat /proc/$$/foo".
>>
>> Andrey, I know that it is hard to let go of the code you invested time
>> and efforts in creating. But this isn't the last patch, is it?
>> You will need to retrieve yet more data for process checkpointing.
>> When you start working on the next patch for it, consider trying
>> /proc approach.
>
> I don't think that we need to convert siginfo into a human readable format
> in kernel.

My point is that bolting hacks onto various bits of kernel API
in order to support process checkpointing makes those APIs
(their in-kernel implementation) ridden with special cases
and harder to support in the future.

Process checkpointing needs to bite the bullet and
create its own API instead.

Whether it would be a /proc/PID/checkpoint or a
ptrace(PTRACE_GET_CHKPOINT_DATA) is another question.

-- 
vda

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Pavel Emelyanov]

On 02/11/2013 06:46 PM, Denys Vlasenko wrote:
> On Mon, Feb 11, 2013 at 11:59 AM, Andrew Vagin <avagin@parallels.com> wrote:
>>>> I suppose I had wondered along similar lines, but in a slightly
>>>> different direction: would the use of a /proc interface to get the
>>>> queued signals make some sense?
>>>
>>> I think that /proc interface beats adding magic flags and magic semantic
>>> to [p]read.
>>>
>>> It also has the benefit of being human-readable. You don't need
>>> to write a special C program to "cat /proc/$$/foo".
>>>
>>> Andrey, I know that it is hard to let go of the code you invested time
>>> and efforts in creating. But this isn't the last patch, is it?
>>> You will need to retrieve yet more data for process checkpointing.
>>> When you start working on the next patch for it, consider trying
>>> /proc approach.
>>
>> I don't think that we need to convert siginfo into a human readable format
>> in kernel.
> 
> My point is that bolting hacks onto various bits of kernel API
> in order to support process checkpointing makes those APIs
> (their in-kernel implementation) ridden with special cases
> and harder to support in the future.
> 
> Process checkpointing needs to bite the bullet and
> create its own API instead.

This is bad approach as well. What we should do is come up with a sane
API that makes sense without the checkpoint-restore project _when_ _possible_.

> Whether it would be a /proc/PID/checkpoint or a
> ptrace(PTRACE_GET_CHKPOINT_DATA) is another question.

Thanks,
Pavel

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Denys Vlasenko]

On Mon, Feb 11, 2013 at 3:53 PM, Pavel Emelyanov <xemul@parallels.com> wrote:
> On 02/11/2013 06:46 PM, Denys Vlasenko wrote:
>> On Mon, Feb 11, 2013 at 11:59 AM, Andrew Vagin <avagin@parallels.com> wrote:
>>>>> I suppose I had wondered along similar lines, but in a slightly
>>>>> different direction: would the use of a /proc interface to get the
>>>>> queued signals make some sense?
>>>>
>>>> I think that /proc interface beats adding magic flags and magic semantic
>>>> to [p]read.
>>>>
>>>> It also has the benefit of being human-readable. You don't need
>>>> to write a special C program to "cat /proc/$$/foo".
>>>>
>>>> Andrey, I know that it is hard to let go of the code you invested time
>>>> and efforts in creating. But this isn't the last patch, is it?
>>>> You will need to retrieve yet more data for process checkpointing.
>>>> When you start working on the next patch for it, consider trying
>>>> /proc approach.
>>>
>>> I don't think that we need to convert siginfo into a human readable format
>>> in kernel.
>>
>> My point is that bolting hacks onto various bits of kernel API
>> in order to support process checkpointing makes those APIs
>> (their in-kernel implementation) ridden with special cases
>> and harder to support in the future.
>>
>> Process checkpointing needs to bite the bullet and
>> create its own API instead.
>
> This is bad approach as well. What we should do is come up with a sane
> API that makes sense without the checkpoint-restore project _when_ _possible_.

Coming up with a sane API in general isn't easy.

Consider numerous blunders enshrined in the Unix API,
such as O_NONBLOCK being a file's flag instead of being
a flag of read(), or waitpid, or sigwait,
(had to be fds which one can feed to select/poll)...

If you have your own playground in /proc/PID/foo,
you can mature your API without touching many other areas
of kernel, and without making mistakes permanent.
Later, when other people are interested, they can factor out
your code.


You are planning to use signalfd to extract pending signals
from the process being checkpointed.

This must be a quite convoluted method already, since you
need to create a signalfd and then read from it *in the context
of the process you are checkpointing*.

I presume you are ptrace-attaching to the process and then
play games with setting registers and injecting syscalls.
This does not look particularly sane to me, I'm afraid.

Compared to this, ptrace-attaching to the process
and then reading from /proc or issuing a new ptrace request
looks much cleaner. My opinion, of course.

-- 
vda

Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

[Pavel Emelyanov]

>>> Process checkpointing needs to bite the bullet and
>>> create its own API instead.
>>
>> This is bad approach as well. What we should do is come up with a sane
>> API that makes sense without the checkpoint-restore project _when_ _possible_.
> 
> Coming up with a sane API in general isn't easy.

That's what we've put the linux-api@ in Cc for ;)

[snip]

> Compared to this, ptrace-attaching to the process
> and then reading from /proc or issuing a new ptrace request
> looks much cleaner.

Sure it does! Also note, that it also looks much cleaner than the
fancy /proc/pid/checkpoint thing you propose.

> My opinion, of course.

Thanks,
Pavel

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Michael Kerrisk (man-pages)]

Hi Andrey,

On Tue, Jan 22, 2013 at 11:15 AM, Andrey Vagin <avagin@openvz.org> wrote:
> This patch set adds ability to choose a signal queue and
> to read signals without dequeuing them.
>
> Three new flags are added:
> SFD_SHARED_QUEUE     -- reads will be from process-wide shared signal queue
> SFD_PER_THREAD_QUEUE -- reads will be from per-thread signal queue
> SFD_PEEK             -- don't dequeue signals

A fuller description of the patch, including information that was in
previous versions of this patch would be helpful. Let me see if I can
summarize/fill out the API side of things, and ask a few questions
along the way (yes, I could answer some of the questions by checking
the code, but I want to know what the *intended* behavior is).

The patch series adds a total of 4 flags to signalfd(). In addition to
those you list above, the other is

SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo

The intention is that these flags be used in conjunction with pread(),
to peek at queued signals. The 'offset' argument is treated as a
position. Thus, for example, to non-destructively read all of the
per-thread signals in raw form from the per-thread queue, one would
write

fd = signalfd(-1, SFD_PER_THREAD_QUEUE | SFD_RAW | SFD_PEEK)
for (j = 0; ; j++) {
    s = pread(fd, buf, ocunt, j)
    if (s == 0) /* No more signals */
        break;
}

Right?

Now some questions. I don't require all of the following, but I'm
wanting to know what's possible, for documentation purposes.

Q1: with this patch series, is it permissible to specify
SFD_PER_THREAD_QUEUE or SFD_SHARED_QUEUE without specifying either
SFD_PEEK or SFD_QUEUE? In other words, can one do traditional
signalfd_siginfo reads, but selecting from a specific queue.

Q2: Is it possible to specify SFD_PEEK without SFD_RAW, so that one
can peek at siginfo structs rather than signalfd_siginfo structs?

Q3: Is it possible to specify SFD_RAW without SFD_PEEK, so that one
can destructively read signalfd_siginfo structs? Can that be done
using any read interface (read(), pread(), etc.)?

Q4: Is it possible to specify both SFD_PER_THREAD_QUEUE and
SFD_SHARED_QUEUE? In that case, in what order are signals read from
the two queues?

Thanks,

Michael

> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> CC: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>
> --
> 1.7.11.7
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Andrew Vagin]

On Wed, Jan 23, 2013 at 05:19:24AM +0100, Michael Kerrisk (man-pages) wrote:
> Hi Andrey,
> 
> On Tue, Jan 22, 2013 at 11:15 AM, Andrey Vagin <avagin@openvz.org> wrote:
> > This patch set adds ability to choose a signal queue and
> > to read signals without dequeuing them.
> >
> > Three new flags are added:
> > SFD_SHARED_QUEUE     -- reads will be from process-wide shared signal queue
> > SFD_PER_THREAD_QUEUE -- reads will be from per-thread signal queue
> > SFD_PEEK             -- don't dequeue signals

> 
> A fuller description of the patch, including information that was in
> previous versions of this patch would be helpful. Let me see if I can
> summarize/fill out the API side of things, and ask a few questions
> along the way (yes, I could answer some of the questions by checking
> the code, but I want to know what the *intended* behavior is).
> 
> The patch series adds a total of 4 flags to signalfd(). In addition to
> those you list above, the other is

In additional we can say, that this patch series adds three orthogonal,
independent groups of flags.
* SFD_RAW
* SFD_PEEK
* SFD_SHARED_QUEUE, SFD_PER_THREAD_QUEUE

> 
> SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo
> 
> The intention is that these flags be used in conjunction with pread(),
> to peek at queued signals. The 'offset' argument is treated as a
> position. Thus, for example, to non-destructively read all of the
> per-thread signals in raw form from the per-thread queue, one would
> write
> 

siginfo_t *buf;

> fd = signalfd(-1, SFD_PER_THREAD_QUEUE | SFD_RAW | SFD_PEEK)
> for (j = 0; ; j++) {
>     s = pread(fd, buf, ocunt, j)
      s = pread(fd, buf + j, sizeof(siginfo_t), j);
>     if (s <= 0) /* No more signals */
>         break;
> }

This examples reads signals one by one

or

  siginfo_t *buf = NULL;
  unsigned long buf_size = 0, nr = 0;
  int ret;

  while (1) {
	bug_size += PAGE_SIZ;
	buf = realloc(buf, buf_size);
	if (buf == NULL)
		goto err;
	ret = pread(fd, buf + nr, sizeof(siginfo_t), nr);
	if (ret == -1)
		goto err;
	nr += ret / sizeof(siginfo_t);
	if (ret < PAGE_SIZE) /* No more signals */
		break;
  }

pread() can read more than one signal.

* The interface of signalfd could be a bit more predictable,
  if we will treat pos as offset in bytes, not in elements.

  pread(fd, buf, sizeof(siginfo_t), i * sizeof(siginfo_t)) -
               reads a signal with a sequence number i in a queue.
> 
> Right?
> 
> Now some questions. I don't require all of the following, but I'm
> wanting to know what's possible, for documentation purposes.
> 
> Q1: with this patch series, is it permissible to specify
> SFD_PER_THREAD_QUEUE or SFD_SHARED_QUEUE without specifying either
> SFD_PEEK or SFD_QUEUE? In other words, can one do traditional
> signalfd_siginfo reads, but selecting from a specific queue.

Yes, we can
> 
> Q2: Is it possible to specify SFD_PEEK without SFD_RAW, so that one
> can peek at siginfo structs rather than signalfd_siginfo structs?

Yes, it is possible. read() and pread() returns signalfd_siginfo structs
in this case.

> 
> Q3: Is it possible to specify SFD_RAW without SFD_PEEK, so that one
> can destructively read signalfd_siginfo structs? Can that be done
> using any read interface (read(), pread(), etc.)?
Yes, it is possible too. read() will return siginfo structs.

> 
> Q4: Is it possible to specify both SFD_PER_THREAD_QUEUE and
> SFD_SHARED_QUEUE? In that case, in what order are signals read from
> the two queues?
> 

It is equal to the case, when none of these flags are not specified.
And it is equal to what we had before this patches.
signalfd() reads signals from a private queue, then from a shared queue.

ps: A good analogy:
signalfd looks like a pipe if SFD_PEEK are not specified, otherwise it looks
like a file

> Thanks,
> 
> Michael
> 
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Pavel Emelyanov <xemul@parallels.com>
> > CC: Cyrill Gorcunov <gorcunov@openvz.org>
> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> >
> > --
> > 1.7.11.7
> >
> 
> 
> 
> -- 
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Michael Kerrisk (man-pages)]

Hi Andrey,

On Wed, Jan 23, 2013 at 12:03 PM, Andrew Vagin <avagin@parallels.com> wrote:
> On Wed, Jan 23, 2013 at 05:19:24AM +0100, Michael Kerrisk (man-pages) wrote:
>> Hi Andrey,
>>
>> On Tue, Jan 22, 2013 at 11:15 AM, Andrey Vagin <avagin@openvz.org> wrote:
>> > This patch set adds ability to choose a signal queue and
>> > to read signals without dequeuing them.
>> >
>> > Three new flags are added:
>> > SFD_SHARED_QUEUE     -- reads will be from process-wide shared signal queue
>> > SFD_PER_THREAD_QUEUE -- reads will be from per-thread signal queue
>> > SFD_PEEK             -- don't dequeue signals
>
>>
>> A fuller description of the patch, including information that was in
>> previous versions of this patch would be helpful. Let me see if I can
>> summarize/fill out the API side of things, and ask a few questions
>> along the way (yes, I could answer some of the questions by checking
>> the code, but I want to know what the *intended* behavior is).
>>
>> The patch series adds a total of 4 flags to signalfd(). In addition to
>> those you list above, the other is
>
> In additional we can say, that this patch series adds three orthogonal,
> independent groups of flags.
> * SFD_RAW
> * SFD_PEEK
> * SFD_SHARED_QUEUE, SFD_PER_THREAD_QUEUE

Thanks. Nice summary.

>> SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo
>>
>> The intention is that these flags be used in conjunction with pread(),
>> to peek at queued signals. The 'offset' argument is treated as a
>> position. Thus, for example, to non-destructively read all of the
>> per-thread signals in raw form from the per-thread queue, one would
>> write
>>
>
> siginfo_t *buf;
>
>> fd = signalfd(-1, SFD_PER_THREAD_QUEUE | SFD_RAW | SFD_PEEK)
>> for (j = 0; ; j++) {
>>     s = pread(fd, buf, ocunt, j)
>       s = pread(fd, buf + j, sizeof(siginfo_t), j);
>>     if (s <= 0) /* No more signals */
>>         break;
>> }
>
> This examples reads signals one by one
>
> or
>
>   siginfo_t *buf = NULL;
>   unsigned long buf_size = 0, nr = 0;
>   int ret;
>
>   while (1) {
>         bug_size += PAGE_SIZ;
>         buf = realloc(buf, buf_size);
>         if (buf == NULL)
>                 goto err;
>         ret = pread(fd, buf + nr, sizeof(siginfo_t), nr);
>         if (ret == -1)
>                 goto err;
>         nr += ret / sizeof(siginfo_t);
>         if (ret < PAGE_SIZE) /* No more signals */
>                 break;
>   }
>
> pread() can read more than one signal.

(Thanks for the reminder on that last point.)

> * The interface of signalfd could be a bit more predictable,
>   if we will treat pos as offset in bytes, not in elements.
>
>   pread(fd, buf, sizeof(siginfo_t), i * sizeof(siginfo_t)) -
>                reads a signal with a sequence number i in a queue.

Can you explain what you mean by "more predictable"? It's not clear to me.

>> Right?
>>
>> Now some questions. I don't require all of the following, but I'm
>> wanting to know what's possible, for documentation purposes.
>>
>> Q1: with this patch series, is it permissible to specify
>> SFD_PER_THREAD_QUEUE or SFD_SHARED_QUEUE without specifying either
>> SFD_PEEK or SFD_QUEUE? In other words, can one do traditional
>> signalfd_siginfo reads, but selecting from a specific queue.
>
> Yes, we can
>>
>> Q2: Is it possible to specify SFD_PEEK without SFD_RAW, so that one
>> can peek at siginfo structs rather than signalfd_siginfo structs?
>
> Yes, it is possible. read() and pread() returns signalfd_siginfo structs
> in this case.
>
>>
>> Q3: Is it possible to specify SFD_RAW without SFD_PEEK, so that one
>> can destructively read signalfd_siginfo structs? Can that be done
>> using any read interface (read(), pread(), etc.)?
> Yes, it is possible too. read() will return siginfo structs.

3 * yes is nice!

For which of the above 3 questions was the answer "No" with the
previous version of these patches (the version that specified queue
selection in pread())?


>> Q4: Is it possible to specify both SFD_PER_THREAD_QUEUE and
>> SFD_SHARED_QUEUE? In that case, in what order are signals read from
>> the two queues?
>>
>
> It is equal to the case, when none of these flags are not specified.
> And it is equal to what we had before this patches.
> signalfd() reads signals from a private queue, then from a shared queue.

So, the 'offset' argument of pread() is interpreted by considering the
per-thread and shared queue as one concatenated list, right?

If yes to the previous question, then from an API design point of view
that seems odd: it exposes an implementation detail. Is specifying
both SFD_PER_THREAD_QUEUE and SFD_SHARED_QUEUE usefule for
checkpoint/restore? I almost wonder if, when SFD_PEEK is specified, a
requirement should be enforced that SFD_PER_THREAD_QUEUE or
SFD_SHARED_QUEUE, but not both, must be specified. What do you think?

Thanks,

Michael

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Andrew Vagin]

On Wed, Jan 23, 2013 at 01:11:42PM +0100, Michael Kerrisk (man-pages) wrote:
> Hi Andrey,
> 
> On Wed, Jan 23, 2013 at 12:03 PM, Andrew Vagin <avagin@parallels.com> wrote:
> > On Wed, Jan 23, 2013 at 05:19:24AM +0100, Michael Kerrisk (man-pages) wrote:
> >> Hi Andrey,
> >>
> >> On Tue, Jan 22, 2013 at 11:15 AM, Andrey Vagin <avagin@openvz.org> wrote:
> >> > This patch set adds ability to choose a signal queue and
> >> > to read signals without dequeuing them.
> >> >
> >> > Three new flags are added:
> >> > SFD_SHARED_QUEUE     -- reads will be from process-wide shared signal queue
> >> > SFD_PER_THREAD_QUEUE -- reads will be from per-thread signal queue
> >> > SFD_PEEK             -- don't dequeue signals
> >
> >>
> >> A fuller description of the patch, including information that was in
> >> previous versions of this patch would be helpful. Let me see if I can
> >> summarize/fill out the API side of things, and ask a few questions
> >> along the way (yes, I could answer some of the questions by checking
> >> the code, but I want to know what the *intended* behavior is).
> >>
> >> The patch series adds a total of 4 flags to signalfd(). In addition to
> >> those you list above, the other is
> >
> > In additional we can say, that this patch series adds three orthogonal,
> > independent groups of flags.
> > * SFD_RAW
> > * SFD_PEEK
> > * SFD_SHARED_QUEUE, SFD_PER_THREAD_QUEUE
> 
> Thanks. Nice summary.
> 
> >> SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo
> >>
> >> The intention is that these flags be used in conjunction with pread(),
> >> to peek at queued signals. The 'offset' argument is treated as a
> >> position. Thus, for example, to non-destructively read all of the
> >> per-thread signals in raw form from the per-thread queue, one would
> >> write
> >>
> >
> > siginfo_t *buf;
> >
> >> fd = signalfd(-1, SFD_PER_THREAD_QUEUE | SFD_RAW | SFD_PEEK)
> >> for (j = 0; ; j++) {
> >>     s = pread(fd, buf, ocunt, j)
> >       s = pread(fd, buf + j, sizeof(siginfo_t), j);
> >>     if (s <= 0) /* No more signals */
> >>         break;
> >> }
> >
> > This examples reads signals one by one
> >
> > or
> >
> >   siginfo_t *buf = NULL;
> >   unsigned long buf_size = 0, nr = 0;
> >   int ret;
> >
> >   while (1) {
> >         bug_size += PAGE_SIZ;
> >         buf = realloc(buf, buf_size);
> >         if (buf == NULL)
> >                 goto err;
> >         ret = pread(fd, buf + nr, sizeof(siginfo_t), nr);
> >         if (ret == -1)
> >                 goto err;
> >         nr += ret / sizeof(siginfo_t);
> >         if (ret < PAGE_SIZE) /* No more signals */
> >                 break;
> >   }
> >
> > pread() can read more than one signal.
> 
> (Thanks for the reminder on that last point.)
> 
> > * The interface of signalfd could be a bit more predictable,
> >   if we will treat pos as offset in bytes, not in elements.
> >
> >   pread(fd, buf, sizeof(siginfo_t), i * sizeof(siginfo_t)) -
> >                reads a signal with a sequence number i in a queue.
> 
> Can you explain what you mean by "more predictable"? It's not clear to me.

offset is usual in bytes.

Lets imagine that we have a file, which contains siginfo-s.
If "pos" is offset in bytes, the same code can reads siginfo-s from the
file and from signalfd.

> 
> >> Right?
> >>
> >> Now some questions. I don't require all of the following, but I'm
> >> wanting to know what's possible, for documentation purposes.
> >>
> >> Q1: with this patch series, is it permissible to specify
> >> SFD_PER_THREAD_QUEUE or SFD_SHARED_QUEUE without specifying either
> >> SFD_PEEK or SFD_QUEUE? In other words, can one do traditional
> >> signalfd_siginfo reads, but selecting from a specific queue.
> >
> > Yes, we can
> >>
> >> Q2: Is it possible to specify SFD_PEEK without SFD_RAW, so that one
> >> can peek at siginfo structs rather than signalfd_siginfo structs?
> >
> > Yes, it is possible. read() and pread() returns signalfd_siginfo structs
> > in this case.
> >
> >>
> >> Q3: Is it possible to specify SFD_RAW without SFD_PEEK, so that one
> >> can destructively read signalfd_siginfo structs? Can that be done
> >> using any read interface (read(), pread(), etc.)?
> > Yes, it is possible too. read() will return siginfo structs.
> 
> 3 * yes is nice!
> 
> For which of the above 3 questions was the answer "No" with the
> previous version of these patches (the version that specified queue
> selection in pread())?

Only for the first question.

> 
> 
> >> Q4: Is it possible to specify both SFD_PER_THREAD_QUEUE and
> >> SFD_SHARED_QUEUE? In that case, in what order are signals read from
> >> the two queues?
> >>
> >
> > It is equal to the case, when none of these flags are not specified.
> > And it is equal to what we had before this patches.
> > signalfd() reads signals from a private queue, then from a shared queue.
> 
> So, the 'offset' argument of pread() is interpreted by considering the
> per-thread and shared queue as one concatenated list, right?

It's true, if both SFD_QUEUE flags was specified.
If only one for these flags is specified, the 'offset' argument is a
sequnce number in a proper list.

> 
> If yes to the previous question, then from an API design point of view
> that seems odd: it exposes an implementation detail. Is specifying
> both SFD_PER_THREAD_QUEUE and SFD_SHARED_QUEUE usefule for
> checkpoint/restore?

No. crtools reads signals from each queue separately.

> I almost wonder if, when SFD_PEEK is specified, a
> requirement should be enforced that SFD_PER_THREAD_QUEUE or
> SFD_SHARED_QUEUE, but not both, must be specified. What do you think?

Looks reasonable.

> 
> Thanks,
> 
> Michael

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Oleg Nesterov]

Michael, thanks for your attention.

On 01/23, Michael Kerrisk (man-pages) wrote:
>
> SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo

I hope you are going to document this API... Then please note that
SFD_RAW not only means siginfo_t, there is a subtle difference in
->si_code which (I think) should be documented too.

Oleg.

Re: [PATCH 0/3] signalfd: a kernel interface for dumping pending signals

[Michael Kerrisk (man-pages)]

On Thu, Feb 7, 2013 at 7:20 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> Michael, thanks for your attention.
>
> On 01/23, Michael Kerrisk (man-pages) wrote:
>>
>> SFD_RAW -- return raw siginfo structs when reading, rather than signalfd_siginfo
>
> I hope you are going to document this API... Then please note that
> SFD_RAW not only means siginfo_t, there is a subtle difference in
> ->si_code which (I think) should be documented too.

Thanks for the tip, Andrey.


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/