[PATCH 0/3] signalfd: a kernel interface for dumping/restoring pending signals (v3)

[PATCH 0/3] signalfd: a kernel interface for dumping/restoring pending signals (v3)

[Andrey Vagin]

The idea is simple. We need to get the siginfo for each signal on dump,
and then return it back on restore.

The first problem is that the kernel doesn’t report complete siginfo-s
in user-space. In a signal handler the kernel strips SI_CODE from
siginfo. When a siginfo is received from signalfd, it has a different
format with fixed sizes of fields. The interface of signalfd was
extended. If a signalfd is created with the flag SFD_RAW, it returns
siginfo in a raw format.

rt_sigqueueinfo looks suitable for restoring signals, but it can’t send
siginfo with a positive si_code, because these codes are reserved for
the kernel. In the real world each person has right to do anything with
himself, so I think a process should able to send any siginfo to itself.

v2: add ability to dump signals without dequeuing them.
    pread with non-zero offset is used for this. offset encodes
    a queue (private of shared) and a sequence number of a signal
    in the queue. Thanks to Oleg for this idea.
v3: minor cleanups

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>

Andrey Vagin (3):
  signal: allow to send any siginfo to itself
  signalfd: add ability to return siginfo in a raw format (v2)
  signalfd: add ability to read siginfo-s without dequeuing signals (v4)

 fs/signalfd.c                 | 109 +++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/signalfd.h |   6 +++
 kernel/signal.c               |   6 ++-
 3 files changed, 113 insertions(+), 8 deletions(-)

-- 
1.7.11.7

[PATCH 1/3] signal: allow to send any siginfo to itself

[Andrey Vagin]

A kernel prevents of sending siginfo with positive si_code, because
these codes is reserved for kernel. I think we can allow to send any
siginfo to itself. This operation should not be dangerous.

This functionality is required for restoring signals.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
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>
---
 kernel/signal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7aaa51d..ac5f5e7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2944,7 +2944,8 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
 	/* Not even root can pretend to send signals from the kernel.
 	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
 	 */
-	if (info.si_code >= 0 || info.si_code == SI_TKILL) {
+	if (((info.si_code >= 0 || info.si_code == SI_TKILL)) &&
+	    (task_pid_vnr(current) != pid)) {
 		/* We used to allow any < 0 si_code */
 		WARN_ON_ONCE(info.si_code < 0);
 		return -EPERM;
@@ -2964,7 +2965,8 @@ long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
 	/* Not even root can pretend to send signals from the kernel.
 	 * Nor can they impersonate a kill()/tgkill(), which adds source info.
 	 */
-	if (info->si_code >= 0 || info->si_code == SI_TKILL) {
+	if ((info->si_code >= 0 || info->si_code == SI_TKILL) &&
+	    (task_pid_vnr(current) != pid)) {
 		/* We used to allow any < 0 si_code */
 		WARN_ON_ONCE(info->si_code < 0);
 		return -EPERM;
-- 
1.7.11.7

[PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrey Vagin]

signalfd should be called with the flag SFD_RAW for that.

signalfd_siginfo is not full for siginfo with a negative si_code.
copy_siginfo_to_user() is copied a full siginfo to user-space, if
si_code is negative.  signalfd_copyinfo() doesn't do that and can't be
expanded, because it has not compatible format with siginfo_t.

Another problem is that a constant __SI_* is removed from si_code.
It's not a problem for usual applications, because they expect
a defined type of siginfo (internal logic).
When we want to dump pending signals, we can't predict a type of
siginfo, so we should get it from kernel.

The main idea of the raw format is that it should be enough for
restoring exactly the same siginfo for the current process.

This functionality is required for checkpointing pending signals.

v2: fix a race condition during setting file flags
    copy_siginfo_to_user32() if is_compat_task

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                 | 64 +++++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/signalfd.h |  1 +
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index b534869..4439a81 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -30,6 +30,7 @@
 #include <linux/signalfd.h>
 #include <linux/syscalls.h>
 #include <linux/proc_fs.h>
+#include <linux/compat.h>
 
 void signalfd_cleanup(struct sighand_struct *sighand)
 {
@@ -74,6 +75,38 @@ static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 }
 
 /*
+ * Copy a whole siginfo into users spaces.
+ * The main idea of this format is that it should be enough
+ * for restoring siginfo back into the kernel.
+ */
+static int signalfd_copy_raw_info(struct signalfd_siginfo __user *siginfo,
+					siginfo_t *kinfo)
+{
+	siginfo_t *uinfo = (siginfo_t *) siginfo;
+	int err;
+
+	BUILD_BUG_ON(sizeof(siginfo_t) != sizeof(struct signalfd_siginfo));
+
+	err = __clear_user(uinfo, sizeof(*uinfo));
+
+#ifdef CONFIG_COMPAT
+	if (unlikely(is_compat_task())) {
+		compat_siginfo_t *compat_uinfo = (compat_siginfo_t *) siginfo;
+
+		err |= copy_siginfo_to_user32(compat_uinfo, kinfo);
+		err |= put_user(kinfo->si_code, &compat_uinfo->si_code);
+
+		return err ? -EFAULT: sizeof(*compat_uinfo);
+	}
+#endif
+
+	err |= copy_siginfo_to_user(uinfo, kinfo);
+	err |= put_user(kinfo->si_code, &uinfo->si_code);
+
+	return err ? -EFAULT: sizeof(*uinfo);
+}
+
+/*
  * Copied from copy_siginfo_to_user() in kernel/signal.c
  */
 static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
@@ -205,6 +238,7 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 	struct signalfd_ctx *ctx = file->private_data;
 	struct signalfd_siginfo __user *siginfo;
 	int nonblock = file->f_flags & O_NONBLOCK;
+	bool raw = file->f_flags & SFD_RAW;
 	ssize_t ret, total = 0;
 	siginfo_t info;
 
@@ -217,7 +251,12 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 		ret = signalfd_dequeue(ctx, &info, nonblock);
 		if (unlikely(ret <= 0))
 			break;
-		ret = signalfd_copyinfo(siginfo, &info);
+
+		if (raw)
+			ret = signalfd_copy_raw_info(siginfo, &info);
+		else
+			ret = signalfd_copyinfo(siginfo, &info);
+
 		if (ret < 0)
 			break;
 		siginfo++;
@@ -262,7 +301,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))
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_RAW))
 		return -EINVAL;
 
 	if (sizemask != sizeof(sigset_t) ||
@@ -272,20 +311,35 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 	signotset(&sigmask);
 
 	if (ufd == -1) {
+		struct file *file;
 		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 		if (!ctx)
 			return -ENOMEM;
 
 		ctx->sigmask = sigmask;
 
+		ufd = get_unused_fd_flags(flags);
+		if (ufd < 0) {
+			kfree(ctx);
+			goto out;
+		}
+
 		/*
 		 * When we call this, the initialization must be complete, since
 		 * anon_inode_getfd() will install the fd.
 		 */
-		ufd = anon_inode_getfd("[signalfd]", &signalfd_fops, ctx,
+		file = anon_inode_getfile("[signalfd]", &signalfd_fops, ctx,
 				       O_RDWR | (flags & (O_CLOEXEC | O_NONBLOCK)));
-		if (ufd < 0)
+		if (IS_ERR(file)) {
+			put_unused_fd(ufd);
+			ufd = PTR_ERR(file);
 			kfree(ctx);
+			goto out;
+		}
+
+		file->f_flags |= flags & SFD_RAW;
+
+		fd_install(ufd, file);
 	} else {
 		struct fd f = fdget(ufd);
 		if (!f.file)
@@ -302,7 +356,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 		wake_up(&current->sighand->signalfd_wqh);
 		fdput(f);
 	}
-
+out:
 	return ufd;
 }
 
diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
index 492c6de..bc31849 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -15,6 +15,7 @@
 /* Flags for signalfd4.  */
 #define SFD_CLOEXEC O_CLOEXEC
 #define SFD_NONBLOCK O_NONBLOCK
+#define SFD_RAW O_DIRECT
 
 struct signalfd_siginfo {
 	__u32 ssi_signo;
-- 
1.7.11.7

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

[Andrey Vagin]

pread(fd, buf, size, pos) with non-zero pos returns siginfo-s
without dequeuing signals.

A sequence number and a queue are encoded in pos.

pos = seq + SFD_*_OFFSET

seq is a sequence number of a signal in a queue.

SFD_PER_THREAD_QUEUE_OFFSET - read signals from a per-thread queue.
SFD_SHARED_QUEUE_OFFSET - read signals from a shared (process wide) queue.

This functionality is required for checkpointing pending signals.

v2: llseek() can't be used here, 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. // Oleg Nesterov

v3,v4: minor cleanups

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                 | 45 ++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/signalfd.h |  5 +++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 4439a81..1eb9b87 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -51,6 +51,44 @@ struct signalfd_ctx {
 	sigset_t sigmask;
 };
 
+static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
+				siginfo_t *info, loff_t *ppos)
+{
+	struct sigpending *pending;
+	struct sigqueue *q;
+	loff_t seq;
+	int ret = 0;
+
+	if (*ppos >= SFD_SHARED_QUEUE_OFFSET) {
+		pending = &current->signal->shared_pending;
+		seq = *ppos - SFD_SHARED_QUEUE_OFFSET;
+	} else if (*ppos >= SFD_PER_THREAD_QUEUE_OFFSET) {
+		pending = &current->pending;
+		seq = *ppos - SFD_PER_THREAD_QUEUE_OFFSET;
+	} else
+		return -EINVAL;
+
+	spin_lock_irq(&current->sighand->siglock);
+
+	list_for_each_entry(q, &pending->list, list) {
+		if (sigismember(&ctx->sigmask, q->info.si_signo))
+			continue;
+
+		if (seq-- == 0) {
+			copy_siginfo(info, &q->info);
+			ret = info->si_signo;
+			break;
+		}
+	}
+
+	spin_unlock_irq(&current->sighand->siglock);
+
+	if (ret)
+		(*ppos)++;
+
+	return ret;
+}
+
 static int signalfd_release(struct inode *inode, struct file *file)
 {
 	kfree(file->private_data);
@@ -248,7 +286,11 @@ 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);
+		if (*ppos == 0)
+			ret = signalfd_dequeue(ctx, &info, nonblock);
+		else
+			ret = signalfd_peek(ctx, &info, ppos);
+
 		if (unlikely(ret <= 0))
 			break;
 
@@ -338,6 +380,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
 		}
 
 		file->f_flags |= flags & SFD_RAW;
+		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 bc31849..0953785 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -17,6 +17,11 @@
 #define SFD_NONBLOCK O_NONBLOCK
 #define SFD_RAW O_DIRECT
 
+/* Read signals from a shared (process wide) queue */
+#define SFD_SHARED_QUEUE_OFFSET (1LL << 62)
+/* Read signals from a per-thread queue */
+#define SFD_PER_THREAD_QUEUE_OFFSET 1
+
 struct signalfd_siginfo {
 	__u32 ssi_signo;
 	__s32 ssi_errno;
-- 
1.7.11.7

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

[Oleg Nesterov]

On 01/14, Andrey Vagin wrote:
>
> v3: minor cleanups

I have no more comments ;)

Obviously I can't ack this series, especially the API.

But correctness-wise everything looks fine to me, so

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrew Morton]

On Mon, 14 Jan 2013 20:53:54 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> signalfd should be called with the flag SFD_RAW for that.
> 
> signalfd_siginfo is not full for siginfo with a negative si_code.
> copy_siginfo_to_user() is copied a full siginfo to user-space, if
> si_code is negative.  signalfd_copyinfo() doesn't do that and can't be
> expanded, because it has not compatible format with siginfo_t.
> 
> Another problem is that a constant __SI_* is removed from si_code.
> It's not a problem for usual applications, because they expect
> a defined type of siginfo (internal logic).
> When we want to dump pending signals, we can't predict a type of
> siginfo, so we should get it from kernel.
> 
> The main idea of the raw format is that it should be enough for
> restoring exactly the same siginfo for the current process.
> 
> This functionality is required for checkpointing pending signals.
> 
> ...
>
> --- a/fs/signalfd.c
> +++ b/fs/signalfd.c
> @@ -30,6 +30,7 @@
>  #include <linux/signalfd.h>
>  #include <linux/syscalls.h>
>  #include <linux/proc_fs.h>
> +#include <linux/compat.h>
>  
>  void signalfd_cleanup(struct sighand_struct *sighand)
>  {
> @@ -74,6 +75,38 @@ static unsigned int signalfd_poll(struct file *file, poll_table *wait)
>  }
>  
>  /*
> + * Copy a whole siginfo into users spaces.

"userspace"

> + * The main idea of this format is that it should be enough
> + * for restoring siginfo back into the kernel.
> + */
> +static int signalfd_copy_raw_info(struct signalfd_siginfo __user *siginfo,
> +					siginfo_t *kinfo)
> +{
> +	siginfo_t *uinfo = (siginfo_t *) siginfo;

Should be

	siginfo_t __user *uinfo = (siginfo_t __user *)siginfo;

> +	int err;
> +
> +	BUILD_BUG_ON(sizeof(siginfo_t) != sizeof(struct signalfd_siginfo));
> +
> +	err = __clear_user(uinfo, sizeof(*uinfo));
>
> +#ifdef CONFIG_COMPAT
> +	if (unlikely(is_compat_task())) {
> +		compat_siginfo_t *compat_uinfo = (compat_siginfo_t *) siginfo;
> +
> +		err |= copy_siginfo_to_user32(compat_uinfo, kinfo);
> +		err |= put_user(kinfo->si_code, &compat_uinfo->si_code);
> +
> +		return err ? -EFAULT: sizeof(*compat_uinfo);
> +	}
> +#endif
> +
> +	err |= copy_siginfo_to_user(uinfo, kinfo);
> +	err |= put_user(kinfo->si_code, &uinfo->si_code);
> +
> +	return err ? -EFAULT: sizeof(*uinfo);
> +}
> +
> +/*
>   * Copied from copy_siginfo_to_user() in kernel/signal.c
>   */
>  static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
>
> ...
>
> --- a/include/uapi/linux/signalfd.h
> +++ b/include/uapi/linux/signalfd.h
> @@ -15,6 +15,7 @@
>  /* Flags for signalfd4.  */
>  #define SFD_CLOEXEC O_CLOEXEC
>  #define SFD_NONBLOCK O_NONBLOCK
> +#define SFD_RAW O_DIRECT
>  
>  struct signalfd_siginfo {
>  	__u32 ssi_signo;

As SFD_RAW is being added to the kernel API we should document it. 
Please keep Michael cc'ed and work with him on getting the manpages
updated.

I usually ask that checkpoint-restart specific code be wrapped in
#ifdef CONFIG_CHECKPOINT_RESTORE, mainly so we can identify it all
if/when your project fails and we decide to remove the feature ;) But
as this patch extends the user API I think it simplifies life if we
make the extension permanent.  Perhaps this is a bad idea, as
permanently adding this extension to the API makes it harder to ever
remove the c/r feature.  


Proposed fixups.  Please review and test this and check that sparse is
happy with it.

From: Andrew Morton <akpm@linux-foundation.org>
Subject: signalfd-add-ability-to-return-siginfo-in-a-raw-format-v2-fix

fix __user annotations, tidy comments and code layout

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrey Vagin <avagin@openvz.org>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/signalfd.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff -puN fs/signalfd.c~signalfd-add-ability-to-return-siginfo-in-a-raw-format-v2-fix fs/signalfd.c
--- a/fs/signalfd.c~signalfd-add-ability-to-return-siginfo-in-a-raw-format-v2-fix
+++ a/fs/signalfd.c
@@ -75,14 +75,14 @@ static unsigned int signalfd_poll(struct
 }
 
 /*
- * Copy a whole siginfo into users spaces.
+ * Copy a whole siginfo into userspace.
  * The main idea of this format is that it should be enough
  * for restoring siginfo back into the kernel.
  */
 static int signalfd_copy_raw_info(struct signalfd_siginfo __user *siginfo,
 					siginfo_t *kinfo)
 {
-	siginfo_t *uinfo = (siginfo_t *) siginfo;
+	siginfo_t __user *uinfo = (siginfo_t __user *)siginfo;
 	int err;
 
 	BUILD_BUG_ON(sizeof(siginfo_t) != sizeof(struct signalfd_siginfo));
@@ -91,19 +91,20 @@ static int signalfd_copy_raw_info(struct
 
 #ifdef CONFIG_COMPAT
 	if (unlikely(is_compat_task())) {
-		compat_siginfo_t *compat_uinfo = (compat_siginfo_t *) siginfo;
+		compat_siginfo_t __user *compat_uinfo;
 
+		compat_uinfo = (compat_siginfo_t __user *)siginfo;
 		err |= copy_siginfo_to_user32(compat_uinfo, kinfo);
 		err |= put_user(kinfo->si_code, &compat_uinfo->si_code);
 
-		return err ? -EFAULT: sizeof(*compat_uinfo);
+		return err ? -EFAULT : sizeof(*compat_uinfo);
 	}
 #endif
 
 	err |= copy_siginfo_to_user(uinfo, kinfo);
 	err |= put_user(kinfo->si_code, &uinfo->si_code);
 
-	return err ? -EFAULT: sizeof(*uinfo);
+	return err ? -EFAULT : sizeof(*uinfo);
 }
 
 /*
_

Re: [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrew Vagin]

On Wed, Jan 16, 2013 at 12:35:02PM -0800, Andrew Morton wrote:
> > --- a/include/uapi/linux/signalfd.h
> > +++ b/include/uapi/linux/signalfd.h
> > @@ -15,6 +15,7 @@
> >  /* Flags for signalfd4.  */
> >  #define SFD_CLOEXEC O_CLOEXEC
> >  #define SFD_NONBLOCK O_NONBLOCK
> > +#define SFD_RAW O_DIRECT
> >  
> >  struct signalfd_siginfo {
> >  	__u32 ssi_signo;
> 
> As SFD_RAW is being added to the kernel API we should document it. 
> Please keep Michael cc'ed and work with him on getting the manpages
> updated.

Ok

> 
> I usually ask that checkpoint-restart specific code be wrapped in
> #ifdef CONFIG_CHECKPOINT_RESTORE, mainly so we can identify it all
> if/when your project fails and we decide to remove the feature ;) But
> as this patch extends the user API I think it simplifies life if we
> make the extension permanent.  Perhaps this is a bad idea, as
> permanently adding this extension to the API makes it harder to ever
> remove the c/r feature.  

I think CRIU is already enough functional to be successful ;).
It already can dump/restore a Linux Container with Oracle Data Base,
Apache and a few other deamons.

> 
> 
> Proposed fixups.  Please review and test this and check that sparse is
> happy with it.
>
I have tested this patch. All is ok and sparce are happy with it.

Thank you.

Re: [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Michael Kerrisk (man-pages)]

> As SFD_RAW is being added to the kernel API we should document it.
> Please keep Michael cc'ed and work with him on getting the manpages
> updated.

The API is certainly no thing of beauty, as I already remarked here:
http://thread.gmane.org/gmane.linux.file-systems/70420/focus=1420228

A description of the API can be found here:
https://lwn.net/Articles/531939/

The main problem is the ugliness of changing the meaning of pread()'s
'offset' argument to be a multiplexed [signal queue selector + queue
position].

I do wonder if we can do better, though I have no particular solution
to offer beyond the sledgehammer of adding a new system call.

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 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrey Wagin]

2013/1/19 Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>:
>> As SFD_RAW is being added to the kernel API we should document it.
>> Please keep Michael cc'ed and work with him on getting the manpages
>> updated.
>
> The API is certainly no thing of beauty, as I already remarked here:
> http://thread.gmane.org/gmane.linux.file-systems/70420/focus=1420228

All of us have similar thoughts, but we don't know a better solution.

Here is an example of usage signalfd and pread:
http://lists.openvz.org/pipermail/criu/2013-January/007040.html

>
> A description of the API can be found here:
> https://lwn.net/Articles/531939/

Thanks you for the attempt to involve more people to this discussion.

>
> The main problem is the ugliness of changing the meaning of pread()'s
> 'offset' argument to be a multiplexed [signal queue selector + queue
> position].
>
> I do wonder if we can do better, though I have no particular solution
> to offer beyond the sledgehammer of adding a new system call.

If we are choosing between this interface or a new syscall, I would
prefer this interface. signalfd is a special descriptor, so I think it
is not a big deal, that it works a bit strange. If all other would
decides, that a new syscall is better, I will not ague.

Michael, thank you for the comments.

>
> 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/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Michael Kerrisk (man-pages)]

Hello Andrey,

On Sat, Jan 19, 2013 at 11:50 AM, Andrey Wagin <avagin@gmail.com> wrote:
> 2013/1/19 Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>:
>>> As SFD_RAW is being added to the kernel API we should document it.
>>> Please keep Michael cc'ed and work with him on getting the manpages
>>> updated.
>>
>> The API is certainly no thing of beauty, as I already remarked here:
>> http://thread.gmane.org/gmane.linux.file-systems/70420/focus=1420228
>
> All of us have similar thoughts, but we don't know a better solution.
>
> Here is an example of usage signalfd and pread:
> http://lists.openvz.org/pipermail/criu/2013-January/007040.html
>
>>
>> A description of the API can be found here:
>> https://lwn.net/Articles/531939/
>
> Thanks you for the attempt to involve more people to this discussion.
>
>>
>> The main problem is the ugliness of changing the meaning of pread()'s
>> 'offset' argument to be a multiplexed [signal queue selector + queue
>> position].
>>
>> I do wonder if we can do better, though I have no particular solution
>> to offer beyond the sledgehammer of adding a new system call.
>
> If we are choosing between this interface or a new syscall, I would
> prefer this interface.

A agree. That's why I called using a new syscall a sledgehammer.
Still, I just wanted to remind folk who might think of something
better.

> signalfd is a special descriptor, so I think it
> is not a big deal, that it works a bit strange.

Sure, but the more we special case things, the uglier the ABI as a
whole becomes. So special casing should be avoided as far as we can.

> If all other would
> decides, that a new syscall is better, I will not ague.

And that's more or less how I see it too. I'm not going to argue for a
new syscall, based on what I know so far.

Here is one idea to think about though, while more or less maintaining
your proposed interface.

At the moment, you select signal queues in the pread() call. An
alternative would be to do it in the signalfd() call. In other words,
you could have the following flags used with signalfd()

SFD_RAW
SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue

Specifying both SFD_SHARED_QUEUE  and SFD_PER_THREAD_QUEUE would be
the same as omitting them both, providing the default behavior of
slecting from both queues.

My point here is that you can then separate the RAW functionality from
the queue selector functionality. Now, it might be that at the moment
you always require that if the caller specifies SFD_SHARED_QUEUE  or
SFD_PER_THREAD_QUEUE, then they must also specify SFD_RAW. But later,
that constraint might be relaxed, so that users could use signalfd()
to select from a particular queue when reading traditional (non-RAW)
signalfd_siginfo structures from a signalfd. This does seem like a
very sensible design optimization to make now (and an easy one, I
would suppose). What do you think?

Cheers,

Michael

Re: [CRIU] [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrew Vagin]

> > signalfd is a special descriptor, so I think it
> > is not a big deal, that it works a bit strange.
> 
> Sure, but the more we special case things, the uglier the ABI as a
> whole becomes. So special casing should be avoided as far as we can.
> 
> > If all other would
> > decides, that a new syscall is better, I will not ague.
> 
> And that's more or less how I see it too. I'm not going to argue for a
> new syscall, based on what I know so far.
> 
> Here is one idea to think about though, while more or less maintaining
> your proposed interface.
> 
> At the moment, you select signal queues in the pread() call. An
> alternative would be to do it in the signalfd() call. In other words,
> you could have the following flags used with signalfd()
> 
> SFD_RAW
> SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
> SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue

I suggested this variant in the initial series, but then we decided to
avoid adding new flags. Oleg, what do you think about this?

> 
> Specifying both SFD_SHARED_QUEUE  and SFD_PER_THREAD_QUEUE would be
> the same as omitting them both, providing the default behavior of
> slecting from both queues.
> 
> My point here is that you can then separate the RAW functionality from
> the queue selector functionality. Now, it might be that at the moment
> you always require that if the caller specifies SFD_SHARED_QUEUE  or
> SFD_PER_THREAD_QUEUE, then they must also specify SFD_RAW. But later,
> that constraint might be relaxed, so that users could use signalfd()
> to select from a particular queue when reading traditional (non-RAW)
> signalfd_siginfo structures from a signalfd.

I am not sure, that you understood this moment correctly.
Currently SFD_RAW is independent on SFD_*_QUEUE. If signalfd is
created without SFD_RAW, pread returns signalfd_siginfo-s.
If SFD_RAW is set, read returns siginfo_t-s.

One more point for two flags is that we will be able to choose a queue
from which signals will be dequeued. Currently we can choose a queue only
for pread.

Thanks

> This does seem like a
> very sensible design optimization to make now (and an easy one, I
> would suppose). What do you think?
> 
> Cheers,
> 
> Michael

Re: [CRIU] [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Michael Kerrisk (man-pages)]

Hi Andrey,

On Sun, Jan 20, 2013 at 6:41 PM, Andrew Vagin <avagin@parallels.com> wrote:
>
>> > signalfd is a special descriptor, so I think it
>> > is not a big deal, that it works a bit strange.
>>
>> Sure, but the more we special case things, the uglier the ABI as a
>> whole becomes. So special casing should be avoided as far as we can.
>>
>> > If all other would
>> > decides, that a new syscall is better, I will not ague.
>>
>> And that's more or less how I see it too. I'm not going to argue for a
>> new syscall, based on what I know so far.
>>
>> Here is one idea to think about though, while more or less maintaining
>> your proposed interface.
>>
>> At the moment, you select signal queues in the pread() call. An
>> alternative would be to do it in the signalfd() call. In other words,
>> you could have the following flags used with signalfd()
>>
>> SFD_RAW
>> SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
>> SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue
>
> I suggested this variant in the initial series, but then we decided to
> avoid adding new flags. Oleg, what do you think about this?
>
>>
>> Specifying both SFD_SHARED_QUEUE  and SFD_PER_THREAD_QUEUE would be
>> the same as omitting them both, providing the default behavior of
>> slecting from both queues.
>>
>> My point here is that you can then separate the RAW functionality from
>> the queue selector functionality. Now, it might be that at the moment
>> you always require that if the caller specifies SFD_SHARED_QUEUE  or
>> SFD_PER_THREAD_QUEUE, then they must also specify SFD_RAW. But later,
>> that constraint might be relaxed, so that users could use signalfd()
>> to select from a particular queue when reading traditional (non-RAW)
>> signalfd_siginfo structures from a signalfd.
>
> I am not sure, that you understood this moment correctly.
> Currently SFD_RAW is independent on SFD_*_QUEUE. If signalfd is
> created without SFD_RAW, pread returns signalfd_siginfo-s.
> If SFD_RAW is set, read returns siginfo_t-s.

So, do you mean it is already possible to do:

fd = signalfd(-1, &mask, 0)      // no SFD_RAW flag
pread(fd, buf, count, SFD_PER_THREAD_QUEUE_OFFSET + pos)

to read a siginfo from the thread specific signal queue?

Thanks

> One more point for two flags is that we will be able to choose a queue
> from which signals will be dequeued. Currently we can choose a queue only
> for pread.

Also a good point, and another reason to place queue selection in the
signalfd() call, rather than in pread().

Cheers,

Michael


>> This does seem like a
>> very sensible design optimization to make now (and an easy one, I
>> would suppose). What do you think?
>>
>> Cheers,
>>
>> Michael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
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 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Oleg Nesterov]

On 01/20, Andrew Vagin wrote:
>
> > SFD_RAW
> > SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
> > SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue
>
> I suggested this variant in the initial series, but then we decided to
> avoid adding new flags.

Yes, because SFD_SHARED/PRIVATE will add even more complications into this
code. And outside of signalfd.c too. And nobody except c/r will ever use
these features I guess.

> Oleg, what do you think about this?

See above... but as I said from the very beginning I won't insist.

Oleg.

Re: [CRIU] [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Michael Kerrisk (man-pages)]

Hi Oleg,

On Sun, Jan 20, 2013 at 8:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/20, Andrew Vagin wrote:
>>
>> > SFD_RAW
>> > SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
>> > SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue
>>
>> I suggested this variant in the initial series, but then we decided to
>> avoid adding new flags.
>
> Yes, because SFD_SHARED/PRIVATE will add even more complications into this
> code. And outside of signalfd.c too. And nobody except c/r will ever use
> these features I guess.

So, I must admit that I don't understand why adding SFD_SHARED and
SFD_PER_THREAD in signalfd() makes things more complicated than adding
the equivalents in pread(). But, I'll take your word for that, if
that's what you meant.

However, note that I did suggest that in the initial implementation
you might require that if SFD_SHARED_QUEUE or SFD_PER_THREAD_QUEUE is
specified in signalfd(), then SFD_RAW could be required as well.
Later, if someone wants to do the work, they could relax the
constraint, so as to allow signalfd() to be used to do per-queue
select even when reading siginfo (i.e., SFD_SHARED_QUEUE or
SFD_PER_THREAD_QUEUE specified but not SFD_RAW). Surely that is not
more complicated than the current implementation. And it allows for an
orthogonal expansion of the design that seems more natural and may one
day be useful.

Cheers,

Michael

Re: [CRIU] [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrew Vagin]

On Sun, Jan 20, 2013 at 09:33:41PM +0100, Michael Kerrisk (man-pages) wrote:
> Hi Oleg,
> 
> On Sun, Jan 20, 2013 at 8:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 01/20, Andrew Vagin wrote:
> >>
> >> > SFD_RAW
> >> > SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
> >> > SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue
> >>
> >> I suggested this variant in the initial series, but then we decided to
> >> avoid adding new flags.
> >
> > Yes, because SFD_SHARED/PRIVATE will add even more complications into this
> > code. And outside of signalfd.c too. And nobody except c/r will ever use
> > these features I guess.
> 
> So, I must admit that I don't understand why adding SFD_SHARED and
> SFD_PER_THREAD in signalfd() makes things more complicated than adding
> the equivalents in pread(). But, I'll take your word for that, if
> that's what you meant.

You could look at the initial series.
http://thread.gmane.org/gmane.linux.file-systems/70327

In this case dequeue_signal should be changed for specifying a queue.
signalfd_poll() should be fixed too. All this changes are not a big
deal, but...

If we want to have two flags for specifying queues, we will need one
more flag SFD_PEEK, which will say, that signals should not be dequeued.
Currently we get this information from offset too. If offset isn't zero,
signals are not dequeued. With flags this sentence will look ugly.

> 
> However, note that I did suggest that in the initial implementation
> you might require that if SFD_SHARED_QUEUE or SFD_PER_THREAD_QUEUE is
> specified in signalfd(), then SFD_RAW could be required as well.
> Later, if someone wants to do the work, they could relax the
> constraint, so as to allow signalfd() to be used to do per-queue
> select even when reading siginfo (i.e., SFD_SHARED_QUEUE or
> SFD_PER_THREAD_QUEUE specified but not SFD_RAW). Surely that is not
> more complicated than the current implementation. And it allows for an
> orthogonal expansion of the design that seems more natural and may one
> day be useful.
> 
> Cheers,
> 
> Michael

Re: [CRIU] [PATCH 2/3] signalfd: add ability to return siginfo in a raw format (v2)

[Andrey Wagin]

2013/1/21 Andrew Vagin <avagin@parallels.com>:
> On Sun, Jan 20, 2013 at 09:33:41PM +0100, Michael Kerrisk (man-pages) wrote:
>> Hi Oleg,
>>
>> On Sun, Jan 20, 2013 at 8:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 01/20, Andrew Vagin wrote:
>> >>
>> >> > SFD_RAW
>> >> > SFD_SHARED_QUEUE -- reads will be from process-wide shared signal queue
>> >> > SFD_PER_THREAD_QUEUE --reads will be from per-thread signal queue
>> >>
>> >> I suggested this variant in the initial series, but then we decided to
>> >> avoid adding new flags.
>> >
>> > Yes, because SFD_SHARED/PRIVATE will add even more complications into this
>> > code. And outside of signalfd.c too. And nobody except c/r will ever use
>> > these features I guess.
>>
>> So, I must admit that I don't understand why adding SFD_SHARED and
>> SFD_PER_THREAD in signalfd() makes things more complicated than adding
>> the equivalents in pread(). But, I'll take your word for that, if
>> that's what you meant.
>
> You could look at the initial series.
> http://thread.gmane.org/gmane.linux.file-systems/70327
>
> In this case dequeue_signal should be changed for specifying a queue.
> signalfd_poll() should be fixed too. All this changes are not a big
> deal, but...
>
> If we want to have two flags for specifying queues, we will need one
> more flag SFD_PEEK, which will say, that signals should not be dequeued.
> Currently we get this information from offset too. If offset isn't zero,
> signals are not dequeued. With flags this sentence will look ugly.

I'm going to prepare a series with flags tomorrow.

Thanks.

>
>>
>> However, note that I did suggest that in the initial implementation
>> you might require that if SFD_SHARED_QUEUE or SFD_PER_THREAD_QUEUE is
>> specified in signalfd(), then SFD_RAW could be required as well.
>> Later, if someone wants to do the work, they could relax the
>> constraint, so as to allow signalfd() to be used to do per-queue
>> select even when reading siginfo (i.e., SFD_SHARED_QUEUE or
>> SFD_PER_THREAD_QUEUE specified but not SFD_RAW). Surely that is not
>> more complicated than the current implementation. And it allows for an
>> orthogonal expansion of the design that seems more natural and may one
>> day be useful.
>>
>> Cheers,
>>
>> Michael