[PATCH RFC v3 0/6] Introduce extended syscall error reporting

[PATCH RFC v3 0/6] Introduce extended syscall error reporting

[Alexander Shishkin]

Hi Ingo, Peter and everybody,

This is another stab at the error reporting problem. I've been sitting
on this code for a couple of weeks now for no good reason, so I
figured I'd just put it out there and see where we go next.

This time around, the error reporting itself is a separate
"infrastructure", which is mostly a header file infested with macros
and some code to set it all up and deliver to userspace. The latter is
now done with a prctl() like Ingo suggested. This is the first
patch. Then, it gets integrated into perf core and one example error
return is annotated with a message. The rest of the patchset adds
support to perf tooling, which includes its own JSON parser (I wasn't
aware that Andi was bringing one in with one of his pull requests at
the moment of writing it and I still like it better not only because
of the NIH symptoms) and extends perf_evsel__open_strerror() to fetch
these error messages from the kernel. I didn't include all the
instrumentation that I did in the previous versions of the patchset to
keep the noise level down.

Alexander Shishkin (6):
  exterr: Introduce extended syscall error reporting
  perf: Use extended syscall error reporting
  perf/x86: Annotate a BTS error with extended error reporting
  perf tools: Add a simple JSON parser
  perf tools: Add userspace counterpart for extended error reporting
  perf tools: Use extended syscall error reporting

 arch/x86/kernel/cpu/perf_event.c |   5 +-
 include/linux/exterr.h           |  99 ++++++++++++++++
 include/linux/perf_event.h       |  14 +++
 include/linux/sched.h            |   1 +
 include/uapi/linux/prctl.h       |   5 +
 kernel/events/core.c             |  17 ++-
 kernel/sys.c                     |   6 +
 lib/Makefile                     |   2 +
 lib/exterr.c                     | 157 ++++++++++++++++++++++++
 tools/include/tools/json.h       |  40 +++++++
 tools/lib/util/json.c            | 250 +++++++++++++++++++++++++++++++++++++++
 tools/perf/util/Build            |   6 +
 tools/perf/util/evsel.c          |  12 +-
 tools/perf/util/exterr.c         |  79 +++++++++++++
 tools/perf/util/exterr.h         |  21 ++++
 15 files changed, 711 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/exterr.h
 create mode 100644 lib/exterr.c
 create mode 100644 tools/include/tools/json.h
 create mode 100644 tools/lib/util/json.c
 create mode 100644 tools/perf/util/exterr.c
 create mode 100644 tools/perf/util/exterr.h

-- 
2.5.1

[PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Alexander Shishkin]

It has been pointed out several times that certain system calls' error
reporting leaves a lot to be desired [1], [2]. Such system calls would
take complex parameter structures as their input and return -EINVAL if
one or more parameters are invalid or in conflict leaving it up to the
user to figure out exactly what is wrong with their request. One such
syscall is perf_event_open() with its attribute structure containing
40+ parameters and tens of parameter validation checks.

This patch introduces a fairly simple infrastructure that allows call
sites to annotate their error codes with arbitrary strings, which the
userspace can fetch using a prctl() along with the module name that
produced the error message, file name, line number and optionally any
amount of additional information in JSON format. This way, we can
provide both human-readable and machine-parsable information to user and
leave room for domain-specific extensions, such as the field in the
parameter structure that caused the error.

Each error "site" is referred to by its index, which is folded into an
integer error value within the range of [-EXT_ERRNO, -MAX_ERRNO], where
EXT_ERRNO is chosen to be below any known error codes, but still leaving
enough room to enumerate error sites. This way, all the traditional macros
will still handle these as error codes and we'd only have to convert them
to their original values right before returning to userspace. At that
point we'd also store a pointer to the error descriptor in the task_struct,
so that a subsequent prctl() call can retrieve it.

[1] http://marc.info/?l=linux-kernel&m=141470811013082
[2] http://marc.info/?l=linux-kernel&m=144049385530680

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/exterr.h     |  99 ++++++++++++++++++++++++++++
 include/linux/sched.h      |   1 +
 include/uapi/linux/prctl.h |   5 ++
 kernel/sys.c               |   6 ++
 lib/Makefile               |   2 +
 lib/exterr.c               | 157 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 270 insertions(+)
 create mode 100644 include/linux/exterr.h
 create mode 100644 lib/exterr.c

diff --git a/include/linux/exterr.h b/include/linux/exterr.h
new file mode 100644
index 0000000000..1f412fe9ac
--- /dev/null
+++ b/include/linux/exterr.h
@@ -0,0 +1,99 @@
+/*
+ * Extended syscall error reporting
+ */
+#ifndef _LINUX_EXTERR_H
+#define _LINUX_EXTERR_H
+
+#include <linux/compiler.h>
+
+/*
+ * Extended error reporting: annotate an error code with a string
+ * and a module name to help users diagnase problems with their
+ * attributes and other syscall parameters.
+ */
+
+/*
+ * This is the basic error descriptor structure that is statically
+ * allocated for every annotated error (error site).
+ *
+ * Subsystems that wish to extend this structure should embed it
+ * and provide a callback for formatting the additional fields.
+ */
+struct ext_err_site {
+	const char		*message;
+	const char		*owner;
+	const char		*file;
+	const int		line;
+	const int		code;
+};
+
+/*
+ * Error domain descriptor (compile/link time)
+ */
+struct ext_err_domain_desc {
+	const char	*name;
+	const size_t	err_site_size;
+	const void	*start, *end;
+	char		*(*format)(void *site);
+	int		first;
+	int		last;
+};
+
+extern struct ext_err_domain_desc __attribute__((weak)) __start___ext_err_domain_desc[];
+extern struct ext_err_domain_desc __attribute__((weak)) __stop___ext_err_domain_desc[];
+
+#define DECLARE_EXTERR_DOMAIN(__name, __format)				\
+	extern const struct __name ## _ext_err_site __attribute__((weak)) __start_ ## __name ## _ext_err[]; \
+	extern const struct __name ## _ext_err_site __attribute__((weak)) __stop_ ## __name ## _ext_err[]; \
+	const struct ext_err_domain_desc __used				\
+	__attribute__ ((__section__("__ext_err_domain_desc")))		\
+	__name ## _ext_err_domain_desc = {				\
+		.name		= __stringify(__name),			\
+		.err_site_size	= sizeof(struct __name ## _ext_err_site), \
+		.start		= __start_ ## __name ## _ext_err,	\
+		.end		= __stop_ ## __name ## _ext_err,	\
+		.format		= __format,				\
+	};								\
+
+#define __ext_err(__domain, __c, __m, __domain__fields ...) ({		\
+	static struct __domain ## _ext_err_site				\
+	__attribute__ ((__section__(__stringify(__domain) "_ext_err"))) \
+		__err_site = {						\
+		.site = {						\
+			.message	= (__m),			\
+			.owner		= EXTERR_MODNAME,		\
+			.file		= __FILE__,			\
+			.line		= __LINE__,			\
+			.code		= __builtin_constant_p((__c)) ?	\
+			(__c) : -EINVAL,				\
+		},							\
+		## __domain__fields,					\
+	};								\
+	&__err_site;							\
+})
+
+#define ext_err(__domain, __c, __m, __domain__fields ...)		\
+	({								\
+		extern const struct __domain ## _ext_err_site __start_ ## __domain ## _ext_err[]; \
+		extern struct ext_err_domain_desc __domain ## _ext_err_domain_desc; \
+		-(__domain ## _ext_err_domain_desc.first +		\
+		  (__ext_err(__domain, __c, __m, __domain__fields) -	\
+		   __start_ ## __domain ## _ext_err));			\
+	})
+
+int ext_err_copy_to_user(void __user *buf, size_t size);
+
+int ext_err_errno(int code);
+
+/*
+ * Use part of the [-1, -MAX_ERRNO] errno range for the extended error
+ * reporting. Anything within [-EXT_ERRNO, -MAX_ERRNO] is an index of a
+ * ext_err_site structure within __ext_err section. 3k should be enough
+ * for everybody, but let's add a boot-time warning just in case it overflows
+ * one day.
+ */
+#define EXT_ERRNO 1024
+
+#define EXT_ERR_PTR(__e, __m)	(ERR_PTR(ext_err(__e, __m)))
+
+#endif /* _LINUX_EXTERR_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 119823decc..0eeeda62ef 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1776,6 +1776,7 @@ struct task_struct {
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 	unsigned long	task_state_change;
 #endif
+	int ext_err_code;
 	int pagefault_disabled;
 /* CPU-specific state of this task */
 	struct thread_struct thread;
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 31891d9535..c531b8058e 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -190,4 +190,9 @@ struct prctl_mm_map {
 # define PR_FP_MODE_FR		(1 << 0)	/* 64b FP registers */
 # define PR_FP_MODE_FRE		(1 << 1)	/* 32b compatibility */
 
+/*
+ * Retrieve the extended error report for the last error
+ */
+#define PR_GET_ERR_DESC		47
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 259fda25eb..70f1e435c1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -54,6 +54,7 @@
 #include <linux/cred.h>
 
 #include <linux/kmsg_dump.h>
+#include <linux/exterr.h>
 /* Move somewhere else to avoid recompiling? */
 #include <generated/utsrelease.h>
 
@@ -2267,6 +2268,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_GET_FP_MODE:
 		error = GET_FP_MODE(me);
 		break;
+	case PR_GET_ERR_DESC:
+		if (arg4 || arg5)
+			return -EINVAL;
+		error = ext_err_copy_to_user((void __user *)arg2, (size_t)arg3);
+		break;
 	default:
 		error = -EINVAL;
 		break;
diff --git a/lib/Makefile b/lib/Makefile
index 6897b52758..e95330d245 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -205,3 +205,5 @@ quiet_cmd_build_OID_registry = GEN     $@
 clean-files	+= oid_registry_data.c
 
 obj-$(CONFIG_UCS2_STRING) += ucs2_string.o
+
+obj-y		+= exterr.o
diff --git a/lib/exterr.c b/lib/exterr.c
new file mode 100644
index 0000000000..c56c88db58
--- /dev/null
+++ b/lib/exterr.c
@@ -0,0 +1,157 @@
+/*
+ * Extended error reporting
+ */
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/exterr.h>
+#include <linux/uaccess.h>
+
+static struct ext_err_domain_desc ** domains;
+static unsigned int nr_domains;
+
+int exterr_last = EXT_ERRNO;
+
+static struct ext_err_domain_desc *ext_err_domain_find(int code)
+{
+	unsigned int i = nr_domains / 2;
+
+	if (code < EXT_ERRNO || code > domains[nr_domains - 1]->last)
+		return NULL;
+
+	do {
+		if (code > domains[i]->last)
+			i -= i / 2;
+		else if (code < domains[i]->first)
+			i += i / 2;
+		else
+			return domains[i];
+	} while (i < nr_domains);
+
+	return NULL;
+}
+
+static struct ext_err_site *ext_err_site_find(int code)
+{
+	struct ext_err_domain_desc *dom = ext_err_domain_find(code);
+	struct ext_err_site *site;
+	unsigned long off;
+
+	if (!code)
+		return NULL;
+
+	/* shouldn't happen */
+	if (WARN_ON_ONCE(!dom))
+		return NULL;
+
+	code -= dom->first;
+	off = (unsigned long)dom->start + dom->err_site_size * code;
+	site = (struct ext_err_site *)off;
+	
+	return site;
+}
+
+int ext_err_copy_to_user(void __user *buf, size_t size)
+{
+	struct ext_err_domain_desc *dom = ext_err_domain_find(current->ext_err_code);
+	struct ext_err_site *site = ext_err_site_find(current->ext_err_code);
+	char *output, *dom_fmt = NULL;
+	unsigned long len;
+	int ret;
+
+	if (!site)
+		return 0;
+
+	if (dom->format)
+		dom_fmt = dom->format(site);
+
+        output = kasprintf(GFP_KERNEL,
+                           "{\n"
+                           "\t\"file\": \"%s\",\n"
+                           "\t\"line\": %d,\n"
+                           "\t\"code\": %d,\n"
+                           "\t\"module\": \"%s\",\n"
+                           "\t\"message\": \"%s\"%s\n%s"
+                           "}\n",
+                           site->file, site->line,
+                           site->code, site->owner, site->message,
+			   dom_fmt ? "," : "",
+			   dom_fmt ? dom_fmt : ""
+                           );
+
+	kfree(dom_fmt);
+
+        if (!output)
+                return -ENOMEM;
+
+        /* trim the buffer to the supplied boundary */
+        len = strlen(output);
+        if (len >= size) {
+                len = size - 1;
+                output[len] = 0;
+        }
+
+        ret = copy_to_user(buf, output, len + 1);
+
+        kfree(output);
+
+	if (!ret)
+		current->ext_err_code = 0;
+
+	return ret ? ret : len + 1;
+}
+
+int ext_err_errno(int code)
+{
+	struct ext_err_site *site;
+
+	if (code > -EXT_ERRNO)
+		return code;
+
+	site = ext_err_site_find(-code);
+	if (!site)
+		return code;
+
+	current->ext_err_code = -code;
+
+	return site->code;
+}
+
+static int ext_err_init(void)
+{
+	struct ext_err_domain_desc *dom;
+	size_t size;
+	int i;
+
+	if (!__start___ext_err_domain_desc)
+		return 0;
+
+	nr_domains = __stop___ext_err_domain_desc - __start___ext_err_domain_desc;
+	domains = kcalloc(nr_domains, sizeof(void *), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	/* allocate error code to the domains */
+	printk("Extended error reporting domains:\n");
+	for (dom = __start___ext_err_domain_desc, i = 0;
+	     dom < __stop___ext_err_domain_desc;
+	     dom++, i++) {
+		size = dom->end - dom->start;
+		size /= dom->err_site_size;
+
+		if (WARN_ON_ONCE(exterr_last + size >= MAX_ERRNO))
+			break;
+
+		domains[i]         = dom;
+		domains[i]->first  = exterr_last;
+		domains[i]->last   = exterr_last + size - 1;
+
+		printk("  \"%s\": %d..%d\n", dom->name,
+		       -domains[i]->first, -domains[i]->last);
+
+		exterr_last += size;
+	}
+
+	return 0;
+}
+
+core_initcall(ext_err_init);
-- 
2.5.1

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Jonathan Corbet]

On Fri, 11 Sep 2015 19:00:00 +0300
Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> It has been pointed out several times that certain system calls' error
> reporting leaves a lot to be desired [1], [2]. Such system calls would
> take complex parameter structures as their input and return -EINVAL if
> one or more parameters are invalid or in conflict leaving it up to the
> user to figure out exactly what is wrong with their request. One such
> syscall is perf_event_open() with its attribute structure containing
> 40+ parameters and tens of parameter validation checks.
> 
> This patch introduces a fairly simple infrastructure that allows call
> sites to annotate their error codes with arbitrary strings, which the
> userspace can fetch using a prctl() along with the module name that
> produced the error message, file name, line number and optionally any
> amount of additional information in JSON format.

So, a couple of comments as I try to figure this stuff out...

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 119823decc..0eeeda62ef 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1776,6 +1776,7 @@ struct task_struct {
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  	unsigned long	task_state_change;
>  #endif
> +	int ext_err_code;
>  	int pagefault_disabled;

Here you add this extended error code, fine.  I'll come back to this in a
moment, but first...

[...]

> +static struct ext_err_site *ext_err_site_find(int code)
> +{
> +	struct ext_err_domain_desc *dom = ext_err_domain_find(code);
> +	struct ext_err_site *site;
> +	unsigned long off;
> +
> +	if (!code)
> +		return NULL;

You've already used "code" by this point, and it looks like
ext_err_domain_find() will react by doing a full, doomed search for it.

> +	/* shouldn't happen */
> +	if (WARN_ON_ONCE(!dom))
> +		return NULL;
> +
> +	code -= dom->first;
> +	off = (unsigned long)dom->start + dom->err_site_size * code;
> +	site = (struct ext_err_site *)off;
> +	
> +	return site;
> +}
> +
> +int ext_err_copy_to_user(void __user *buf, size_t size)
> +{
> +	struct ext_err_domain_desc *dom = ext_err_domain_find(current->ext_err_code);
> +	struct ext_err_site *site = ext_err_site_find(current->ext_err_code);

Here too you use ext_err_code without making sure it has a useful value.
This will also result in two calls to ext_err_domain_find(), and, thus, a
redundant search of the domains array.  Why not just pass dom to the second
call?

[...]

> +	if (!ret)
> +		current->ext_err_code = 0;

Back to this code: here is the only place you ever clear it, and this seems
to be an error response in its own right.  So, if I read this correctly,
once an extended error has been signalled, it will remain forever in the
task state until another extended error overwrites it, right?

What that says to me is that there will be no way to know whether an error
description returned from prctl() corresponds to an error just reported to
the application or not; it could be an old error from last week.  That
strikes me as being less useful than it could otherwise be.

It seems to me that current->ext_err_code needs to be cleared on each
system call entry (except for your special prctl() of course!).

Or have I missed something somewhere?

Thanks,

jon

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Alexander Shishkin]

Jonathan Corbet <corbet@lwn.net> writes:

> So, a couple of comments as I try to figure this stuff out...

Thanks for taking the time to look at this.

> On Fri, 11 Sep 2015 19:00:00 +0300
> Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
>
>> +int ext_err_copy_to_user(void __user *buf, size_t size)
>> +{
>> +	struct ext_err_domain_desc *dom = ext_err_domain_find(current->ext_err_code);
>> +	struct ext_err_site *site = ext_err_site_find(current->ext_err_code);
>
> Here too you use ext_err_code without making sure it has a useful value.
> This will also result in two calls to ext_err_domain_find(), and, thus, a
> redundant search of the domains array.  Why not just pass dom to the second
> call?

Yes, this is a bit too much of a quick and dirty rfc (as you can
probably guess from the way it's written), you're absolutely right.

> [...]
>
>> +	if (!ret)
>> +		current->ext_err_code = 0;
>
> Back to this code: here is the only place you ever clear it, and this seems
> to be an error response in its own right.  So, if I read this correctly,
> once an extended error has been signalled, it will remain forever in the
> task state until another extended error overwrites it, right?

Yes, and this brings us to an important point (below).

> What that says to me is that there will be no way to know whether an error
> description returned from prctl() corresponds to an error just reported to
> the application or not; it could be an old error from last week.  That
> strikes me as being less useful than it could otherwise be.

It seems to make sense to allow the program to clear it (via a flag in
that prctl(), for example). That is, if we get an error, we try to fetch
the extended description, clear it and forget about it.

Then, this prctl() may be a part of the syscall wrapper (or a library
function that uses that syscall), which might or might not want to leave
the extended error code for the main program to inspect. Or a debugger
might call this prctl() for its debugging purposes, but still keep it
around for the main program.

> It seems to me that current->ext_err_code needs to be cleared on each
> system call entry (except for your special prctl() of course!).

I'd say, it should be up to the program to decide for how long they want
to keep the extended error code around.

Thanks,
--
Alex

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Johannes Berg]

On Tue, 2015-09-15 at 17:15 +0300, Alexander Shishkin wrote:
> 
> > It seems to me that current->ext_err_code needs to be cleared on 
> > each system call entry (except for your special prctl() of 
> > course!).
> 
> I'd say, it should be up to the program to decide for how long they 
> want to keep the extended error code around.
> 

I'm not convinced that works - imagine a library wanting to use the
prctl(), but the main application isn't doing that. Should the library
clear it before every call, to be sure it's not getting stale data?
etc.

johannes

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Alexander Shishkin]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2015-09-15 at 17:15 +0300, Alexander Shishkin wrote:
>> 
>> > It seems to me that current->ext_err_code needs to be cleared on 
>> > each system call entry (except for your special prctl() of 
>> > course!).
>> 
>> I'd say, it should be up to the program to decide for how long they 
>> want to keep the extended error code around.
>> 
>
> I'm not convinced that works - imagine a library wanting to use the
> prctl(), but the main application isn't doing that. Should the library
> clear it before every call, to be sure it's not getting stale data?
> etc.

In other words, a syscall that's capable of throwing an extended error
does clear the current::ext_err_code every time, but not other
syscalls. Otherwise it will indeed get very confusing.

Regards,
--
Alex

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Jonathan Corbet]

[Rather belatedly looping in linux-api]

On Tue, 15 Sep 2015 18:24:28 +0300
Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Tue, 2015-09-15 at 17:15 +0300, Alexander Shishkin wrote:  
> >>   
> >> > It seems to me that current->ext_err_code needs to be cleared on 
> >> > each system call entry (except for your special prctl() of 
> >> > course!).  
> >> 
> >> I'd say, it should be up to the program to decide for how long they 
> >> want to keep the extended error code around.
> >>   
> >
> > I'm not convinced that works - imagine a library wanting to use the
> > prctl(), but the main application isn't doing that. Should the library
> > clear it before every call, to be sure it's not getting stale data?
> > etc.  
> 
> In other words, a syscall that's capable of throwing an extended error
> does clear the current::ext_err_code every time, but not other
> syscalls. Otherwise it will indeed get very confusing.

I think that anything other than the errno "grab it now or lose it"
behavior will prove confusing.  I don't think there is any other way to
know that a given error report corresponds to a specific system call.
Library calls can mess it up.  Kernel changes adding extended reporting to
new system calls can mess it up.  Applications cannot possibly be expected
to know which system calls might change the error-reporting status, they
*have* to assume all of them will.

Or so it seems to me, anyway...

jon

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Johannes Berg]

> I think that anything other than the errno "grab it now or lose it"
> behavior will prove confusing.  I don't think there is any other way to
> know that a given error report corresponds to a specific system call.
> Library calls can mess it up.  Kernel changes adding extended reporting to
> new system calls can mess it up.  Applications cannot possibly be expected
> to know which system calls might change the error-reporting status, they
> *have* to assume all of them will.
> 

Yeah I was about to say something similar - an application that expects
a certain syscall to have extended errors will get confused if running
on an older kernel where that syscall in fact does *not* have extended
errors (and thus also doesn't clear extended errors) and therefore the
extended error from a previous syscall could still be lingering on (for
example because the application didn't care to fetch it for that previo
us syscall.)

johannes

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Jonathan Corbet]

One more little thing:

> +int ext_err_copy_to_user(void __user *buf, size_t size)
> +{

[...]

> +        ret = copy_to_user(buf, output, len + 1);
> +
> +        kfree(output);
> +
> +	if (!ret)
> +		current->ext_err_code = 0;
> +
> +	return ret ? ret : len + 1;
> +}

I'm pretty sure you really want something like:

	return ret ? -EFAULT : (len + 1);

here.

jon

Re: [PATCH RFC v3 1/6] exterr: Introduce extended syscall error reporting

[Alexander Shishkin]

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> It has been pointed out several times that certain system calls' error
> reporting leaves a lot to be desired [1], [2]. Such system calls would
> take complex parameter structures as their input and return -EINVAL if
> one or more parameters are invalid or in conflict leaving it up to the
> user to figure out exactly what is wrong with their request. One such
> syscall is perf_event_open() with its attribute structure containing
> 40+ parameters and tens of parameter validation checks.
>
> This patch introduces a fairly simple infrastructure that allows call
> sites to annotate their error codes with arbitrary strings, which the
> userspace can fetch using a prctl() along with the module name that
> produced the error message, file name, line number and optionally any
> amount of additional information in JSON format. This way, we can
> provide both human-readable and machine-parsable information to user and
> leave room for domain-specific extensions, such as the field in the
> parameter structure that caused the error.
>
> Each error "site" is referred to by its index, which is folded into an
> integer error value within the range of [-EXT_ERRNO, -MAX_ERRNO], where
> EXT_ERRNO is chosen to be below any known error codes, but still leaving
> enough room to enumerate error sites. This way, all the traditional macros
> will still handle these as error codes and we'd only have to convert them
> to their original values right before returning to userspace. At that
> point we'd also store a pointer to the error descriptor in the task_struct,
> so that a subsequent prctl() call can retrieve it.
>
> [1] http://marc.info/?l=linux-kernel&m=141470811013082
> [2] http://marc.info/?l=linux-kernel&m=144049385530680

Ingo, how do you feel about moving this in the more generalized
direction like this or would you say I should give up and keep it perf
specific? Because there seems to be demand for it in the perf land.

Regards,
--
Alex

[PATCH RFC v3 2/6] perf: Use extended syscall error reporting

[Alexander Shishkin]

This patch makes use of the extended syscall error reporting
infrastructure to relay error messages that result from perf_event_open()
attribute validation. On top of the default error report bits, it also
transfers the name of the attribute field that triggered the error.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h | 14 ++++++++++++++
 kernel/events/core.c       | 17 ++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2027809433..eb63074012 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -16,6 +16,20 @@
 
 #include <uapi/linux/perf_event.h>
 
+#include <linux/exterr.h>
+
+struct perf_ext_err_site {
+	struct ext_err_site	site;
+	const char		*attr_field;
+};
+
+#define perf_err(__c, __a, __m)						\
+	({ /* make sure it's a real field before stringifying it */	\
+		struct perf_event_attr __x; (void)__x.__a;		\
+		ext_err(perf, __c, __m,					\
+			.attr_field = __stringify(__a));		\
+	})
+
 /*
  * Kernel-internal data types and definitions:
  */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ae16867670..a714f0602b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9,6 +9,8 @@
  * For licensing details see kernel-base/COPYING
  */
 
+#define EXTERR_MODNAME	"perf"
+
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/cpu.h>
@@ -44,11 +46,24 @@
 #include <linux/compat.h>
 #include <linux/bpf.h>
 #include <linux/filter.h>
+#include <linux/exterr.h>
 
 #include "internal.h"
 
 #include <asm/irq_regs.h>
 
+static char *perf_exterr_format(void *site)
+{
+	struct perf_ext_err_site *psite = site;
+	char *output;
+
+	output = kasprintf(GFP_KERNEL, "\t\"attr_field\": \"%s\"\n",
+			   psite->attr_field);
+	return output;
+}
+
+DECLARE_EXTERR_DOMAIN(perf, perf_exterr_format);
+
 static struct workqueue_struct *perf_wq;
 
 typedef int (*remote_function_f)(void *);
@@ -8352,7 +8367,7 @@ err_group_fd:
 	fdput(group);
 err_fd:
 	put_unused_fd(event_fd);
-	return err;
+	return ext_err_errno(err);
 }
 
 /**
-- 
2.5.1

[PATCH RFC v3 3/6] perf/x86: Annotate a BTS error with extended error reporting

[Alexander Shishkin]

Old-style BTS driver does not support kernel tracing, which normally
would be visible to userspace as -EOPNOTSUPP, or from the tool's user's
perspective:

> # perf record -e branches -c1 ls
> Error:
> No hardware sampling interrupt available.
> No APIC? If so then you can boot the kernel with the "lapic" boot parameter to force-enable it.

but with the new way of reporting errors, they will see the following:

> # perf record -e branches -c1 ls
> Error:
> Syscall returned -95, becasue BTS sampling not allowed for kernel space.
> Offending attribute field: "exclude_kernel"

which is somewhat more to the point of what has actually transpired.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index f56cf074d0..e3d953ef49 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -12,6 +12,8 @@
  *  For licencing details see kernel-base/COPYING
  */
 
+#define EXTERR_MODNAME "perf/x86"
+
 #include <linux/perf_event.h>
 #include <linux/capability.h>
 #include <linux/notifier.h>
@@ -426,7 +428,8 @@ int x86_setup_perfctr(struct perf_event *event)
 
 		/* BTS is currently only allowed for user-mode. */
 		if (!attr->exclude_kernel)
-			return -EOPNOTSUPP;
+			return perf_err(-EOPNOTSUPP, exclude_kernel,
+					"BTS sampling not allowed for kernel space");
 
 		/* disallow bts if conflicting events are present */
 		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
-- 
2.5.1

[PATCH RFC v3 4/6] perf tools: Add a simple JSON parser

[Alexander Shishkin]

In order to process kernel's extended syscall error reports, we need a
JSON parser. This one I wrote myself, it should be very simple and
straightforward and extensible when/if somebody needs more features from
it.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/include/tools/json.h |  40 ++++++++
 tools/lib/util/json.c      | 250 +++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/Build      |   5 +
 3 files changed, 295 insertions(+)
 create mode 100644 tools/include/tools/json.h
 create mode 100644 tools/lib/util/json.c

diff --git a/tools/include/tools/json.h b/tools/include/tools/json.h
new file mode 100644
index 0000000000..a1684574c2
--- /dev/null
+++ b/tools/include/tools/json.h
@@ -0,0 +1,40 @@
+/*
+ * A simple JSON parser
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef _TOOLS_JSON_H
+#define _TOOLS_JSON_H
+
+#define JSON_DEPTH 8
+
+struct json_member {
+	const char	*key;
+	char		*value;
+	unsigned int	size;
+};
+
+struct json_parser {
+	const char		*buffer;
+	const char		*end;
+	const char		*cursor;
+	unsigned int		state;
+	unsigned int		level;
+	unsigned char		stack[JSON_DEPTH];
+	struct json_member	*schema;
+	unsigned int		schema_strict;
+	int			key;
+};
+
+int parse_json(struct json_parser *p);
+
+#endif /* _TOOLS_JSON_H */
diff --git a/tools/lib/util/json.c b/tools/lib/util/json.c
new file mode 100644
index 0000000000..029c244d0e
--- /dev/null
+++ b/tools/lib/util/json.c
@@ -0,0 +1,250 @@
+/*
+ * A simple JSON parser
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <string.h>
+#include <errno.h>
+#include <ctype.h>
+
+#include "tools/json.h"
+
+enum {
+	JS_VALUE = 0,
+	JS_SEPARATOR,
+	JS_KEY,
+	JS_END,
+};
+
+enum {
+	OBJECT = 0,
+	ARRAY,
+	LITERAL,
+};
+
+static int got_key(struct json_parser *p, unsigned int back)
+{
+	const char *key = p->cursor - back;
+	int i;
+
+	for (i = 0; p->schema[i].key; i++) {
+		struct json_member *m = &p->schema[i];
+
+		if (!strncmp(key, m->key, strlen(m->key))) {
+			p->key = i;
+			return 0;
+		}
+	}
+
+	p->key = -1;
+	return p->schema_strict ? -EINVAL : 0;
+}
+
+static int got_value(struct json_parser *p, unsigned int back)
+{
+	unsigned int len = back;
+
+	if (p->key < 0)
+		return p->schema_strict ? -EINVAL : 0;
+
+	if (len > p->schema[p->key].size)
+		len = p->schema[p->key].size;
+
+	memcpy(p->schema[p->key].value, p->cursor - back, len);
+	p->schema[p->key].value[len] = 0;
+	p->key = -1;
+	return 0;
+}
+
+static int consume_string(struct json_parser *p)
+{
+	int ret = 0;
+
+	for (p->cursor++; p->cursor < p->end;
+	     p->cursor++, ret++) {
+		switch(*p->cursor) {
+		case '"':
+			goto done;
+		case '\\':
+			p->cursor++;
+			ret++;
+		default:
+			continue;
+		}
+	}
+
+	return -EINVAL;
+
+done:
+	if (p->state == JS_KEY)
+		ret = got_key(p, ret);
+	else if (p->state == JS_VALUE)
+		ret = got_value(p, ret);
+
+	return ret;
+}
+
+static int consume_number(struct json_parser *p)
+{
+	unsigned int hex = 0;
+	int ret = 0;
+
+	for (; p->cursor < p->end;
+	     p->cursor++, ret++) {
+		switch (*p->cursor) {
+		case '.': /* float */
+			if (!ret)
+				return -EINVAL;
+			continue;
+		case 'x': /* hex */
+			if (ret != 1 || *(p->cursor - 1) != '0')
+				return -EINVAL;
+			hex = 1;
+			continue;
+		case '-': /* negative */
+			if (ret)
+				return -EINVAL;
+			continue;
+		default:
+			if (!hex && !isdigit(*p->cursor))
+				goto done;
+			if (hex && !isxdigit(*p->cursor))
+				goto done;
+
+			continue;
+		}
+	}
+
+	return -EINVAL;
+
+done:
+	if (p->state == JS_KEY)
+		return -EINVAL;
+	else if (p->state == JS_VALUE)
+		ret = got_value(p, ret);
+
+	p->cursor--;
+
+	return ret;
+}
+
+int parse_json(struct json_parser *p)
+{
+	int ret;
+
+	p->key = -1;
+	p->level = 0;
+	p->stack[0] = ARRAY;
+
+	for (p->cursor = p->buffer, p->state = JS_VALUE;
+	     p->cursor < p->end; p->cursor++) {
+		switch (*p->cursor) {
+		case ' ':
+		case '\t':
+		case '\n':
+			/* doesn't change state */
+			continue;
+		case '{':
+			if (p->state != JS_VALUE)
+				return -EINVAL;
+
+			p->stack[++p->level] = OBJECT;
+			p->state = JS_KEY;
+			break;
+		case '}':
+			if ((p->state != JS_END && p->state != JS_KEY) ||
+			    !p->level)
+				return -EINVAL;
+
+			p->level--;
+			p->state = JS_END;
+			break;
+		case ':':
+			if (p->state != JS_SEPARATOR)
+				return -EINVAL;
+
+			p->state = JS_VALUE;
+			break;
+		case '[':
+			if (p->state != JS_VALUE)
+				return -EINVAL;
+
+			p->stack[++p->level] = OBJECT;
+			p->state = JS_VALUE;
+			break;
+		case ']':
+			if ((p->state != JS_END && p->state != JS_VALUE) ||
+			    !p->level)
+				return -EINVAL;
+
+			p->level--;
+			/* still JS_END */
+			break;
+		case '"':
+			/* consume everything up to the matching quote */
+			ret = consume_string(p);
+			if (ret < 0)
+				return ret;
+
+			switch (p->state) {
+			case JS_KEY:
+				p->state = JS_SEPARATOR;
+				break;
+			case JS_VALUE:
+				p->state = JS_END;
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			continue;
+		case ',':
+			if (p->state != JS_END)
+				return -EINVAL;
+
+			if (p->stack[p->level] == ARRAY)
+				p->state = JS_VALUE;
+			else /* OBJECT */
+				p->state = JS_KEY;
+			break;
+		default:
+			if (p->state != JS_VALUE)
+				return -EINVAL;
+
+			if (isdigit(*p->cursor) || *p->cursor == '-') {
+				ret = consume_number(p);
+				if (ret < 0)
+					return ret;
+			} else if (!strncasecmp(p->cursor, "null", 4)) {
+				p->cursor += 3;
+				got_value(p, 4);
+			} else if (!strncasecmp(p->cursor, "true", 4)) {
+				p->cursor += 3;
+				got_value(p, 4);
+			} else if (!strncasecmp(p->cursor, "false", 5)) {
+				p->cursor += 4;
+				got_value(p, 5);
+			} else {
+				return -EINVAL;
+			}
+
+			p->state = JS_END;
+			break;
+		}
+	}
+
+	if (p->level || p->state != JS_END)
+		return -EINVAL;
+
+	return 0;
+}
+
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 349bc96ca1..af5acb9a02 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -12,6 +12,7 @@ libperf-y += evsel.o
 libperf-y += exec_cmd.o
 libperf-y += find_next_bit.o
 libperf-y += help.o
+libperf-y += json.o
 libperf-y += kallsyms.o
 libperf-y += levenshtein.o
 libperf-y += llvm-utils.o
@@ -147,6 +148,10 @@ $(OUTPUT)util/find_next_bit.o: ../lib/util/find_next_bit.c FORCE
 	$(call rule_mkdir)
 	$(call if_changed_dep,cc_o_c)
 
+$(OUTPUT)util/json.o: ../lib/util/json.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
+
 $(OUTPUT)util/rbtree.o: ../lib/rbtree.c FORCE
 	$(call rule_mkdir)
 	$(call if_changed_dep,cc_o_c)
-- 
2.5.1

[PATCH RFC v3 5/6] perf tools: Add userspace counterpart for extended error reporting

[Alexander Shishkin]

Add a wrapper for fetching, parsing and pretty-printing kernel's extended
syscall error reports in a manner that can be useful for communicating
errors to the user.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/perf/util/Build    |  1 +
 tools/perf/util/exterr.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/exterr.h | 21 +++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 tools/perf/util/exterr.c
 create mode 100644 tools/perf/util/exterr.h

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index af5acb9a02..2ccfb3e0e3 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -75,6 +75,7 @@ libperf-y += stat-shadow.o
 libperf-y += record.o
 libperf-y += srcline.o
 libperf-y += data.o
+libperf-y += exterr.o
 libperf-$(CONFIG_X86) += tsc.o
 libperf-$(CONFIG_AUXTRACE) += tsc.o
 libperf-y += cloexec.o
diff --git a/tools/perf/util/exterr.c b/tools/perf/util/exterr.c
new file mode 100644
index 0000000000..3091009688
--- /dev/null
+++ b/tools/perf/util/exterr.c
@@ -0,0 +1,79 @@
+/*
+ * exterr.c: Extended syscall error reporting support
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <sys/prctl.h>
+#include <stdio.h>
+
+#include "tools/json.h"
+#include "util/util.h"
+#include "util/exterr.h"
+
+static char message[BUFSIZ], attr_field[BUFSIZ], line[8], code[8];
+
+#define JSON_FIELD(name)						\
+	{ .key = __stringify(name), .value = (name), .size = sizeof(name), }
+
+static struct json_member exterr_schema[] = {
+	JSON_FIELD(line),
+	JSON_FIELD(code),
+	JSON_FIELD(message),
+	JSON_FIELD(attr_field),
+	{ .key = NULL },
+};
+
+ssize_t exterr__strerror(char *msg, size_t size)
+{
+	char sbuf[BUFSIZ];
+	size_t len;
+	int ret;
+
+	ret = prctl(PR_GET_ERR_DESC, sbuf, sizeof(sbuf), 0, 0);
+	if (ret > 0) {
+		struct json_parser p = {
+			.buffer = sbuf,
+			.end    = sbuf + strlen(sbuf),
+			.schema = exterr_schema,
+			.schema_strict = 0,
+		};
+
+		ret = parse_json(&p);
+		if (!ret) {
+			int orig_err;
+
+			orig_err = atoi(code);
+			ret = scnprintf(msg, size, "Syscall returned %d, becasue %s.\n",
+					orig_err, message);
+			len   = ret;
+			msg  += ret;
+			size -= ret;
+
+			if (attr_field[0]) {
+				/*
+				 * there can also be a lookup table with more
+				 * helpful messages based on this field
+				 */
+				ret = scnprintf(msg, size, "Offending attribute field: \"%s\"\n",
+						attr_field);
+				len  += ret;
+				msg  += ret;
+				size -= ret;
+			}
+
+			ret = len;
+		}
+	}
+
+	return ret;
+}
diff --git a/tools/perf/util/exterr.h b/tools/perf/util/exterr.h
new file mode 100644
index 0000000000..fce70e4f31
--- /dev/null
+++ b/tools/perf/util/exterr.h
@@ -0,0 +1,21 @@
+/*
+ * exterr.h: Extended syscall error reporting support
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef __PERF_EXTERR_H
+#define __PERF_EXTERR_H 1
+
+ssize_t exterr__strerror(char *msg, size_t size);
+
+#endif /* __PERF_EXTERR_H */
-- 
2.5.1

[PATCH RFC v3 6/6] perf tools: Use extended syscall error reporting

[Alexander Shishkin]

If the kernel has an extended error report for us, use it instead of
trying to guess what might have gone wrong.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/perf/util/evsel.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c53f79123b..1804781072 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -27,6 +27,7 @@
 #include "debug.h"
 #include "trace-event.h"
 #include "stat.h"
+#include "exterr.h"
 
 static struct {
 	bool sample_id_all;
@@ -2266,7 +2267,16 @@ bool perf_evsel__fallback(struct perf_evsel *evsel, int err,
 int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			      int err, char *msg, size_t size)
 {
-	char sbuf[STRERR_BUFSIZE];
+	char sbuf[BUFSIZ];
+	int ret;
+
+	ret = exterr__strerror(msg, size);
+	/*
+	 * If kernel gave an extended error description, don't try to be any
+	 * more helpful here.
+	 */
+	if (ret > 0)
+		return ret;
 
 	switch (err) {
 	case EPERM:
-- 
2.5.1