[PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Andrey Vagin]

This patch adds a new ptrace request PTRACE_PEEKSIGINFO.

This request is used to retrieve information about signals starting with
the specified sequence number. Siginfo_t structures are copied from the
child into the buffer starting at "data".

The argument "addr" is a pointer to struct ptrace_peeksiginfo_args.
struct ptrace_peeksiginfo_args {
	u64 off;	/* from which siginfo to start */
	u32 nr;		/* how may siginfos to take */
	u32 flags;
};

Currently here is only one flag PTRACE_PEEKSIGINFO_SHARED for dumping
signals from process-wide shared queue. If this flag is not set, a
signal is read from a per-thread queue.  A result siginfo contains a
kernel part of si_code which usually striped, but it's required for
queuing the same siginfo back during restore of pending signals.

The request PTRACE_PEEKSIGINFO returns a number of dumped signals.
If a signal with the specified sequence number doesn't exist, ptrace
returns 0.

This functionality is required for checkpointing pending signals.

The prototype of this code was developed by Oleg Nesterov.

v2: * don't wrapped on CONFIG_CHECKPOINT_RESTORE. This functionality is
      going to be used in gdb.
    * use ptrace_peeksiginfo_args, because addr is too small for
      encoding a signal number and flags.

Cc: Roland McGrath <roland@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Pedro Alves <palves@redhat.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 include/uapi/linux/ptrace.h | 12 +++++++
 kernel/ptrace.c             | 76 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index 022ab18..01b4bd8 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -5,6 +5,7 @@
 
 /* has the defines to get at the registers. */
 
+#include <linux/types.h>
 
 #define PTRACE_TRACEME		   0
 #define PTRACE_PEEKTEXT		   1
@@ -52,6 +53,17 @@
 #define PTRACE_INTERRUPT	0x4207
 #define PTRACE_LISTEN		0x4208
 
+#define PTRACE_PEEKSIGINFO	0x4209
+
+struct ptrace_peeksiginfo_args {
+	__u64 off;	/* from which siginfo to start */
+	__u32 nr;	/* how may siginfos to take */
+	__u32 flags;
+};
+
+/* Read signals from a shared (process wide) queue */
+#define PTRACE_PEEKSIGINFO_SHARED	(1 << 0)
+
 /* Wait extended result codes for the above trace options.  */
 #define PTRACE_EVENT_FORK	1
 #define PTRACE_EVENT_VFORK	2
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6cbeaae..ceecdcd 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -24,6 +24,7 @@
 #include <linux/regset.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/cn_proc.h>
+#include <linux/compat.h>
 
 
 static int ptrace_trapping_sleep_fn(void *flags)
@@ -618,6 +619,77 @@ static int ptrace_setsiginfo(struct task_struct *child, const siginfo_t *info)
 	return error;
 }
 
+static int ptrace_peek_siginfo(struct task_struct *child,
+				unsigned long addr,
+				unsigned long data)
+{
+	struct ptrace_peeksiginfo_args arg;
+	struct sigpending *pending;
+	struct sigqueue *q;
+	siginfo_t info;
+	long long off;
+	int ret, i;
+
+	ret = copy_from_user(&arg, (void __user *) addr,
+				sizeof(struct ptrace_peeksiginfo_args));
+	if (ret)
+		return ret;
+
+	if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
+		pending = &child->signal->shared_pending;
+	else
+		pending = &child->pending;
+
+	if (arg.flags & ~PTRACE_PEEKSIGINFO_SHARED)
+		return -EINVAL; /* unknown flags */
+
+	for (i = 0; i < arg.nr; i++) {
+		off = arg.off + i;
+
+		spin_lock_irq(&child->sighand->siglock);
+		list_for_each_entry(q, &pending->list, list) {
+			if (!off--) {
+				copy_siginfo(&info, &q->info);
+				break;
+			}
+		}
+		spin_unlock_irq(&child->sighand->siglock);
+
+		if (off >= 0)
+			break;
+
+#ifdef CONFIG_COMPAT
+		if (unlikely(is_compat_task())) {
+			compat_siginfo_t __user *uinfo = compat_ptr(data);
+
+			ret = copy_siginfo_to_user32(uinfo, &info);
+			if (!ret)
+				ret = __put_user(info.si_code, &uinfo->si_code);
+		} else
+#endif
+		{
+			siginfo_t __user *uinfo = (siginfo_t __user *) data;
+
+			ret = copy_siginfo_to_user(uinfo, &info);
+			if (!ret)
+				ret = __put_user(info.si_code, &uinfo->si_code);
+		}
+
+		if (ret)
+			break;
+
+		data += sizeof(siginfo_t);
+
+		if (signal_pending(current)) {
+			i++;
+			break;
+		}
+
+		cond_resched();
+	}
+
+	return i ? : ret;
+}
 
 #ifdef PTRACE_SINGLESTEP
 #define is_singlestep(request)		((request) == PTRACE_SINGLESTEP)
@@ -742,6 +814,10 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = put_user(child->ptrace_message, datalp);
 		break;
 
+	case PTRACE_PEEKSIGINFO:
+		ret = ptrace_peek_siginfo(child, addr, data);
+		break;
+
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)
-- 
1.7.11.7

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Pavel Emelyanov]

> +	for (i = 0; i < arg.nr; i++) {
> +		off = arg.off + i;
> +
> +		spin_lock_irq(&child->sighand->siglock);
> +		list_for_each_entry(q, &pending->list, list) {
> +			if (!off--) {
> +				copy_siginfo(&info, &q->info);
> +				break;
> +			}
> +		}
> +		spin_unlock_irq(&child->sighand->siglock);

What's the point of arg.nr if you for every single siginfo drop the lock
and perform linear search anyway?

Thanks,
Pavel

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Andrey Wagin]

2013/2/25 Pavel Emelyanov <xemul@parallels.com>:
>> +     for (i = 0; i < arg.nr; i++) {
>> +             off = arg.off + i;
>> +
>> +             spin_lock_irq(&child->sighand->siglock);
>> +             list_for_each_entry(q, &pending->list, list) {
>> +                     if (!off--) {
>> +                             copy_siginfo(&info, &q->info);
>> +                             break;
>> +                     }
>> +             }
>> +             spin_unlock_irq(&child->sighand->siglock);
>
> What's the point of arg.nr if you for every single siginfo drop the lock
> and perform linear search anyway?

arg.nr may be useful, if we will decide to optimize this part in a
future. Currently the problem with complexity of finding a signal in a
queue is true not only for this code. It exists for delivering
real-time signals as well. Probably in a future this problem will be
fixed for both cases by the same way.

Thanks,
  Andrew

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Andrew Morton]

On Mon, 25 Feb 2013 14:06:53 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> This patch adds a new ptrace request PTRACE_PEEKSIGINFO.
> 
> This request is used to retrieve information about signals starting with
> the specified sequence number. Siginfo_t structures are copied from the
> child into the buffer starting at "data".
> 
> The argument "addr" is a pointer to struct ptrace_peeksiginfo_args.
> struct ptrace_peeksiginfo_args {
> 	u64 off;	/* from which siginfo to start */
> 	u32 nr;		/* how may siginfos to take */
> 	u32 flags;
> };
> 
> Currently here is only one flag PTRACE_PEEKSIGINFO_SHARED for dumping
> signals from process-wide shared queue. If this flag is not set, a
> signal is read from a per-thread queue.  A result siginfo contains a
> kernel part of si_code which usually striped, but it's required for
> queuing the same siginfo back during restore of pending signals.
> 
> The request PTRACE_PEEKSIGINFO returns a number of dumped signals.
> If a signal with the specified sequence number doesn't exist, ptrace
> returns 0.
> 
> This functionality is required for checkpointing pending signals.
> 
> The prototype of this code was developed by Oleg Nesterov.
> 
> v2: * don't wrapped on CONFIG_CHECKPOINT_RESTORE. This functionality is
>       going to be used in gdb.

That this will be used by gdb is useful information.  Please expand
upon this within the main changelog.

>     * use ptrace_peeksiginfo_args, because addr is too small for
>       encoding a signal number and flags.
> 
> Cc: Roland McGrath <roland@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>

Would like an Oleg ack, please.

>
> ...
>
> @@ -618,6 +619,77 @@ static int ptrace_setsiginfo(struct task_struct *child, const siginfo_t *info)
>  	return error;
>  }
>  
> +static int ptrace_peek_siginfo(struct task_struct *child,
> +				unsigned long addr,
> +				unsigned long data)
> +{
> +	struct ptrace_peeksiginfo_args arg;
> +	struct sigpending *pending;
> +	struct sigqueue *q;
> +	siginfo_t info;
> +	long long off;
> +	int ret, i;
> +
> +	ret = copy_from_user(&arg, (void __user *) addr,
> +				sizeof(struct ptrace_peeksiginfo_args));
> +	if (ret)
> +		return ret;

return -EFAULT;

> +	if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
> +		pending = &child->signal->shared_pending;
> +	else
> +		pending = &child->pending;
>
> +	if (arg.flags & ~PTRACE_PEEKSIGINFO_SHARED)
> +		return -EINVAL; /* unknown flags */

You could perform this test before the previous one and save an
unmeasurably small amount of CPU time in circumstances which never
happen!

> +	for (i = 0; i < arg.nr; i++) {
> +		off = arg.off + i;

"long long" = "u64" + "int".

Wanna have a little think about what we're doing here, see if we can
pick the most appropriate types?

`off' could be made local to this loop, which is a bit neater.

> +		spin_lock_irq(&child->sighand->siglock);
> +		list_for_each_entry(q, &pending->list, list) {
> +			if (!off--) {
> +				copy_siginfo(&info, &q->info);
> +				break;
> +			}
> +		}
> +		spin_unlock_irq(&child->sighand->siglock);
> +
> +		if (off >= 0)
> +			break;

<scratches head>

Oh I see what you did there - the user requested an entry which is
beyond the end of the list?  A code comment would be nice.

What's the return value in this case?  "i".  So how does userspace find
out what happened?

Please fully describe the interface so things such as this can be
understood.

> +#ifdef CONFIG_COMPAT
> +		if (unlikely(is_compat_task())) {
> +			compat_siginfo_t __user *uinfo = compat_ptr(data);
> +
> +			ret = copy_siginfo_to_user32(uinfo, &info);
> +			if (!ret)
> +				ret = __put_user(info.si_code, &uinfo->si_code);
> +		} else
> +#endif
> +		{
> +			siginfo_t __user *uinfo = (siginfo_t __user *) data;
> +
> +			ret = copy_siginfo_to_user(uinfo, &info);
> +			if (!ret)
> +				ret = __put_user(info.si_code, &uinfo->si_code);
> +		}
> +
> +		if (ret)
> +			break;
> +
> +		data += sizeof(siginfo_t);
> +
> +		if (signal_pending(current)) {
> +			i++;

What does the i++ do?

> +			break;
> +		}
> +
> +		cond_resched();
> +	}
> +
> +	return i ? : ret;

I hate that gcc thing :(

Also, is it buggy?  `ret' might be -EFAULT here.  We appear to be using
read() return semantics here?

Please, document the interface *completely* so that others can review
the design and can then check that the implementation matches that
design.  As it stands, we're reduced to mind-reading.


> +}
>
> ...
>

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Michael Kerrisk (man-pages)]

> Please, document the interface *completely* so that others can review
> the design and can then check that the implementation matches that
> design.  As it stands, we're reduced to mind-reading.

+1.

Andrey, please don't assume that people have read past mails. Even if
they have, they're forgetful. Don't make people hunt down old mails to
try to put the pieces together. The changelog should include a full
description of the interface, and also the sample user-space code that
you showed at some point in another mail. That may help you get better
review, and it will certainly help make a better man page.

Cheers,

Michael

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Andrew Morton]

On Mon, 25 Feb 2013 23:28:11 +0100
"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> wrote:

> > Please, document the interface *completely* so that others can review
> > the design and can then check that the implementation matches that
> > design.  As it stands, we're reduced to mind-reading.
> 
> +1.

I suddenly feel less lonely.

> Andrey, please don't assume that people have read past mails. Even if
> they have, they're forgetful. Don't make people hunt down old mails to
> try to put the pieces together. The changelog should include a full
> description of the interface, and also the sample user-space code that
> you showed at some point in another mail.

If there's sample userspace code, there are all sorts of good reasons
for putting that into tools/testing/selftests/your-name-here/

> That may help you get better
> review, and it will certainly help make a better man page.

Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

[Andrey Wagin]

Andrew, thank you for the comments. I will fix them and send a new
patch. A few comments are inline.

2013/2/26 Andrew Morton <akpm@linux-foundation.org>:
> On Mon, 25 Feb 2013 14:06:53 +0400
>> +     for (i = 0; i < arg.nr; i++) {
>> +             off = arg.off + i;
>
> "long long" = "u64" + "int".
>
> Wanna have a little think about what we're doing here, see if we can
> pick the most appropriate types?

A number of signal has the type int, because the system call ptrace()
return "int" and my interface supposed to return a number of dumped
signals.
"off" is signed to check on a negative value and it can not be more
then MAX_RAM / sizeof(siginfo), so
long long is enough. It can be changed on s64 for avoiding misleading.

>
> `off' could be made local to this loop, which is a bit neater.
>
>> +             spin_lock_irq(&child->sighand->siglock);
>> +             list_for_each_entry(q, &pending->list, list) {
>> +                     if (!off--) {
>> +                             copy_siginfo(&info, &q->info);
>> +                             break;
>> +                     }
>> +             }
>> +             spin_unlock_irq(&child->sighand->siglock);
>> +
>> +             if (off >= 0)
>> +                     break;
>
> <scratches head>
>
> Oh I see what you did there - the user requested an entry which is
> beyond the end of the list? A code comment would be nice.
>
> What's the return value in this case?  "i".  So how does userspace find
> out what happened?

The request PTRACE_PEEKSIGINFO returns a number of dumped signals. If
a signal with the specified sequence number doesn't exist, ptrace
returns 0.

>
> Please fully describe the interface so things such as this can be
> understood.
>
>> +#ifdef CONFIG_COMPAT
>> +             if (unlikely(is_compat_task())) {
>> +                     compat_siginfo_t __user *uinfo = compat_ptr(data);
>> +
>> +                     ret = copy_siginfo_to_user32(uinfo, &info);
>> +                     if (!ret)
>> +                             ret = __put_user(info.si_code, &uinfo->si_code);
>> +             } else
>> +#endif
>> +             {
>> +                     siginfo_t __user *uinfo = (siginfo_t __user *) data;
>> +
>> +                     ret = copy_siginfo_to_user(uinfo, &info);
>> +                     if (!ret)
>> +                             ret = __put_user(info.si_code, &uinfo->si_code);
>> +             }
>> +
>> +             if (ret)
>> +                     break;
>> +
>> +             data += sizeof(siginfo_t);
>> +
>> +             if (signal_pending(current)) {
>> +                     i++;
>
> What does the i++ do?

For accounting a current siginfo. I will add a comment here.

>
>> +                     break;
>> +             }
>> +
>> +             cond_resched();
>> +     }
>> +
>> +     return i ? : ret;
>
> I hate that gcc thing :(
>
> Also, is it buggy?  `ret' might be -EFAULT here.  We appear to be using
> read() return semantics here?

I have tried to implement read() semantics here. ptrace() returns a
number of dumped signals even if dumping of the last signal is failed.
And it returns an error if not one signal has been dumped
successfully.

The similar logic is used in read()
generic_file_aio_read()
....
                retval += desc.written;
                if (desc.error) {
                        retval = retval ?: desc.error;
                        break;
                }

Thanks,
  Andrey