Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255691 > unrolled thread

[net-next PATCH] bpf: Output error message to logbuf when loading

Back to article view | Back to linux.kernel

Contents

  [net-next PATCH] bpf: Output error message to logbuf when loading Wang Nan <wangnan0@huawei.com> - 2015-10-26 07:40 +0100
    Re: [net-next PATCH] bpf: Output error message to logbuf when loading "Wangnan (F)" <wangnan0@huawei.com> - 2015-10-26 07:50 +0100
    Re: [net-next PATCH] bpf: Output error message to logbuf when loading kbuild test robot <lkp@intel.com> - 2015-10-26 08:00 +0100
    [net-next PATCHv2] bpf: Output error message to logbuf when loading failure Wang Nan <wangnan0@huawei.com> - 2015-10-26 08:20 +0100
      Re: [net-next PATCHv2] bpf: Output error message to logbuf when  loading failure Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-10-27 04:30 +0100
        Re: [net-next PATCHv2] bpf: Output error message to logbuf when loading  failure "Wangnan (F)" <wangnan0@huawei.com> - 2015-10-27 04:50 +0100

#1255691 — [net-next PATCH] bpf: Output error message to logbuf when loading

Many reason can make bpf_prog_load() return EINVAL. This patch utilizes
logbuf passed from user to deliver the actual reason of failure.

Without this patch, people is easy to forget fixing the "version"
section in their BPF objects.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
---
 kernel/bpf/syscall.c | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 687dd6c..3a0e4e7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -574,6 +574,32 @@ struct bpf_prog *bpf_prog_get(u32 ufd)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_get);
 
+static void
+bpf_prog_load_note(union bpf_attr *attr, const char *fmt, ...)
+{
+	u32 log_level, log_size, log_len;
+	char __user *log_ubuf = NULL;
+	/* 64 chars should be long enough for a one line note. */
+	char log_buf[64];
+	va_list args;
+
+	log_ubuf = (char __user *) (unsigned long) attr->log_buf;
+	log_level = attr->log_level;
+	log_size = sizeof(log_buf);
+	if (attr->log_size < log_size)
+		log_size = attr->log_size;
+
+	if (log_level == 0 || !log_size || !log_ubuf)
+		return;
+
+	va_start(args, fmt);
+	log_len = vscnprintf(log_buf, log_size, fmt, args);
+	va_end(args);
+
+	/* Don't need to care the copying result too much */
+	copy_to_user(log_ubuf, log_buf, log_size);
+}
+
 /* last field in 'union bpf_attr' used by this command */
 #define	BPF_PROG_LOAD_LAST_FIELD kern_version
 
@@ -597,12 +623,19 @@ static int bpf_prog_load(union bpf_attr *attr)
 	/* eBPF programs must be GPL compatible to use GPL-ed functions */
 	is_gpl = license_is_gpl_compatible(license);
 
-	if (attr->insn_cnt >= BPF_MAXINSNS)
+	if (attr->insn_cnt >= BPF_MAXINSNS) {
+		bpf_prog_load_note(attr, "Too many instructions: %d > %d\n",
+				   attr->insn_cnt, BPF_MAXINSNS);
 		return -EINVAL;
+	}
 
 	if (type == BPF_PROG_TYPE_KPROBE &&
-	    attr->kern_version != LINUX_VERSION_CODE)
+	    attr->kern_version != LINUX_VERSION_CODE) {
+		bpf_prog_load_note(attr,
+				   "Kernel version mismatch: 0x%x != 0x%x\n",
+				   attr->kern_version, LINUX_VERSION_CODE);
 		return -EINVAL;
+	}
 
 	if (type != BPF_PROG_TYPE_SOCKET_FILTER && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -631,8 +664,10 @@ static int bpf_prog_load(union bpf_attr *attr)
 
 	/* find program type: socket_filter vs tracing_filter */
 	err = find_prog_type(type, prog);
-	if (err < 0)
+	if (err < 0) {
+		bpf_prog_load_note(attr, "Invalid program type: %d\n", type);
 		goto free_prog;
+	}
 
 	/* run eBPF verifier */
 	err = bpf_check(&prog, attr);
-- 
1.8.3.4

--
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/

[toc] | [next] | [standalone]

#1255695

On 2015/10/26 14:36, Wang Nan wrote:
> Many reason can make bpf_prog_load() return EINVAL. This patch utilizes
> logbuf passed from user to deliver the actual reason of failure.
>
> Without this patch, people is easy to forget fixing the "version"
> section in their BPF objects.
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>   kernel/bpf/syscall.c | 41 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 687dd6c..3a0e4e7 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -574,6 +574,32 @@ struct bpf_prog *bpf_prog_get(u32 ufd)
>   }
>   EXPORT_SYMBOL_GPL(bpf_prog_get);
>   
> +static void
> +bpf_prog_load_note(union bpf_attr *attr, const char *fmt, ...)
> +{
> +	u32 log_level, log_size, log_len;
> +	char __user *log_ubuf = NULL;
> +	/* 64 chars should be long enough for a one line note. */
> +	char log_buf[64];
> +	va_list args;
> +
> +	log_ubuf = (char __user *) (unsigned long) attr->log_buf;
> +	log_level = attr->log_level;
> +	log_size = sizeof(log_buf);
> +	if (attr->log_size < log_size)
> +		log_size = attr->log_size;
> +
> +	if (log_level == 0 || !log_size || !log_ubuf)
> +		return;
> +
> +	va_start(args, fmt);
> +	log_len = vscnprintf(log_buf, log_size, fmt, args);

Don't need this log_len actually. Will send a v2.

Thank you.


--
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/

[toc] | [prev] | [next] | [standalone]

#1255703

[Multipart message — attachments visible in raw view] — view raw

Hi Wang,

[auto build test WARNING on net-next/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Wang-Nan/bpf-Output-error-message-to-logbuf-when-loading/20151026-143920
config: x86_64-randconfig-x019-201543 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   kernel/bpf/syscall.c: In function 'bpf_prog_load_note':
>> kernel/bpf/syscall.c:600:2: warning: ignoring return value of 'copy_to_user', declared with attribute warn_unused_result [-Wunused-result]
     copy_to_user(log_ubuf, log_buf, log_size);
     ^

vim +/copy_to_user +600 kernel/bpf/syscall.c

   584		va_list args;
   585	
   586		log_ubuf = (char __user *) (unsigned long) attr->log_buf;
   587		log_level = attr->log_level;
   588		log_size = sizeof(log_buf);
   589		if (attr->log_size < log_size)
   590			log_size = attr->log_size;
   591	
   592		if (log_level == 0 || !log_size || !log_ubuf)
   593			return;
   594	
   595		va_start(args, fmt);
   596		log_len = vscnprintf(log_buf, log_size, fmt, args);
   597		va_end(args);
   598	
   599		/* Don't need to care the copying result too much */
 > 600		copy_to_user(log_ubuf, log_buf, log_size);
   601	}
   602	
   603	/* last field in 'union bpf_attr' used by this command */
   604	#define	BPF_PROG_LOAD_LAST_FIELD kern_version
   605	
   606	static int bpf_prog_load(union bpf_attr *attr)
   607	{
   608		enum bpf_prog_type type = attr->prog_type;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1255706 — [net-next PATCHv2] bpf: Output error message to logbuf when loading failure

Many reasons can make bpf_prog_load() return EINVAL. This patch utilizes
logbuf to deliver the actual reason of the failure.

Without this patch, it is very easy for user to pass an object with
"version" section not match the kernel version code, and the problem
is hard to determine from return code (EINVAL).

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Wu Fengguang <fengguang.wu@intel.com>
---
 kernel/bpf/syscall.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 687dd6c..719d0cb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -17,6 +17,7 @@
 #include <linux/license.h>
 #include <linux/filter.h>
 #include <linux/version.h>
+#include <linux/bug.h>
 
 int sysctl_unprivileged_bpf_disabled __read_mostly;
 
@@ -574,6 +575,35 @@ struct bpf_prog *bpf_prog_get(u32 ufd)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_get);
 
+static void
+bpf_prog_load_note(union bpf_attr *attr, const char *fmt, ...)
+{
+	u32 log_level, log_size;
+	char __user *log_ubuf = NULL;
+	/* 64 chars should be long enough for a one line note. */
+	char log_buf[64];
+	va_list args;
+
+	log_ubuf = (char __user *) (unsigned long) attr->log_buf;
+	log_level = attr->log_level;
+	log_size = sizeof(log_buf);
+	if (attr->log_size < log_size)
+		log_size = attr->log_size;
+
+	if (log_level == 0 || !log_size || !log_ubuf)
+		return;
+
+	va_start(args, fmt);
+	vscnprintf(log_buf, log_size, fmt, args);
+	va_end(args);
+	log_buf[sizeof(log_buf) - 1] = '\0';
+
+	/* Don't need care the copying result too much */
+	WARN(copy_to_user(log_ubuf, log_buf, log_size),
+	     KERN_WARNING "Failed to copy BPF error note '%s' to log buffer\n",
+	     log_buf);
+}
+
 /* last field in 'union bpf_attr' used by this command */
 #define	BPF_PROG_LOAD_LAST_FIELD kern_version
 
@@ -597,12 +627,19 @@ static int bpf_prog_load(union bpf_attr *attr)
 	/* eBPF programs must be GPL compatible to use GPL-ed functions */
 	is_gpl = license_is_gpl_compatible(license);
 
-	if (attr->insn_cnt >= BPF_MAXINSNS)
+	if (attr->insn_cnt >= BPF_MAXINSNS) {
+		bpf_prog_load_note(attr, "Too many instructions: %d > %d\n",
+				   attr->insn_cnt, BPF_MAXINSNS);
 		return -EINVAL;
+	}
 
 	if (type == BPF_PROG_TYPE_KPROBE &&
-	    attr->kern_version != LINUX_VERSION_CODE)
+	    attr->kern_version != LINUX_VERSION_CODE) {
+		bpf_prog_load_note(attr,
+				   "Kernel version mismatch: 0x%x != 0x%x\n",
+				   attr->kern_version, LINUX_VERSION_CODE);
 		return -EINVAL;
+	}
 
 	if (type != BPF_PROG_TYPE_SOCKET_FILTER && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -631,8 +668,10 @@ static int bpf_prog_load(union bpf_attr *attr)
 
 	/* find program type: socket_filter vs tracing_filter */
 	err = find_prog_type(type, prog);
-	if (err < 0)
+	if (err < 0) {
+		bpf_prog_load_note(attr, "Invalid program type: %d\n", type);
 		goto free_prog;
+	}
 
 	/* run eBPF verifier */
 	err = bpf_check(&prog, attr);
-- 
1.8.3.4

--
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/

[toc] | [prev] | [next] | [standalone]

#1256442 — Re: [net-next PATCHv2] bpf: Output error message to logbuf when loading failure

On Mon, Oct 26, 2015 at 07:13:08AM +0000, Wang Nan wrote:
> Many reasons can make bpf_prog_load() return EINVAL. This patch utilizes
> logbuf to deliver the actual reason of the failure.
> 
> Without this patch, it is very easy for user to pass an object with
> "version" section not match the kernel version code, and the problem
> is hard to determine from return code (EINVAL).
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Wu Fengguang <fengguang.wu@intel.com>

NACK
for both implementation and design.

> +	/* Don't need care the copying result too much */
> +	WARN(copy_to_user(log_ubuf, log_buf, log_size),
> +	     KERN_WARNING "Failed to copy BPF error note '%s' to log buffer\n",
> +	     log_buf);

unprivilged user will be spamming kernel logs?!

> -	if (attr->insn_cnt >= BPF_MAXINSNS)
> +	if (attr->insn_cnt >= BPF_MAXINSNS) {
> +		bpf_prog_load_note(attr, "Too many instructions: %d > %d\n",
> +				   attr->insn_cnt, BPF_MAXINSNS);
>  		return -EINVAL;

if user space did that, it's wrong and can detect it
on its own.

>  	if (type == BPF_PROG_TYPE_KPROBE &&
> -	    attr->kern_version != LINUX_VERSION_CODE)
> +	    attr->kern_version != LINUX_VERSION_CODE) {
> +		bpf_prog_load_note(attr,
> +				   "Kernel version mismatch: 0x%x != 0x%x\n",
> +				   attr->kern_version, LINUX_VERSION_CODE);
>  		return -EINVAL;

user space (perf) could have checked that on its own
without kernel changes.

>  	/* find program type: socket_filter vs tracing_filter */
>  	err = find_prog_type(type, prog);
> -	if (err < 0)
> +	if (err < 0) {
> +		bpf_prog_load_note(attr, "Invalid program type: %d\n", type);
>  		goto free_prog;

same here.

In general syscalls muxing different error conditions into EINVAL
is a kernel wide problem and should be solved for all.

--
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/

[toc] | [prev] | [next] | [standalone]

#1256450 — Re: [net-next PATCHv2] bpf: Output error message to logbuf when loading failure

On 2015/10/27 11:26, Alexei Starovoitov wrote:
> On Mon, Oct 26, 2015 at 07:13:08AM +0000, Wang Nan wrote:
>> Many reasons can make bpf_prog_load() return EINVAL. This patch utilizes
>> logbuf to deliver the actual reason of the failure.
>>
>> Without this patch, it is very easy for user to pass an object with
>> "version" section not match the kernel version code, and the problem
>> is hard to determine from return code (EINVAL).
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Wu Fengguang <fengguang.wu@intel.com>
> NACK
> for both implementation and design.
OK. Let perf to report error message.

Thank you.

--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web