* [PATCH bpf-next v4 0/2] clean-up for BPF_LOG_KERNEL log level
@ 2021-12-01 7:34 Hou Tao
2021-12-01 7:34 ` [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() " Hou Tao
2021-12-01 7:34 ` [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD) Hou Tao
0 siblings, 2 replies; 6+ messages in thread
From: Hou Tao @ 2021-12-01 7:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Yonghong Song, Daniel Borkmann, Andrii Nakryiko,
netdev, bpf, houtao1
Hi,
There are just two clean-up patches for BPF_LOG_KERNEL log level:
patch #1 fixes the possible extra newline for bpf_log() and removes
the unnecessary calculation and truncation, and patch #2 disallows
BPF_LOG_KERNEL log level for bpf_btf_load().
Comments are welcome.
Regards,
Tao
Change Log:
v4:
* rebased on bpf-next
* add Acked-by tags
v3: https://www.spinics.net/lists/bpf/msg48992.html
* rebased on bpf-next
* address comments from Daniel Borkmann:
patch #1: add prefix "BPF: " instead of "BPF:" for error message
patch #2: remove uncessary parenthesis, keep the max buffer length
setting of btf verifier, and add Fixes tag.
v2: https://www.spinics.net/lists/bpf/msg48809.html
* rebased on bpf-next
* patch #1: add a trailing newline if needed (suggested by Martin)
* add patch #2
v1: https://www.spinics.net/lists/bpf/msg48550.html
Hou Tao (2):
bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level
bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD)
include/linux/bpf_verifier.h | 7 +++++++
kernel/bpf/btf.c | 3 +--
kernel/bpf/verifier.c | 16 +++++++++-------
3 files changed, 17 insertions(+), 9 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level
2021-12-01 7:34 [PATCH bpf-next v4 0/2] clean-up for BPF_LOG_KERNEL log level Hou Tao
@ 2021-12-01 7:34 ` Hou Tao
2021-12-01 17:50 ` Alexei Starovoitov
2021-12-01 7:34 ` [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD) Hou Tao
1 sibling, 1 reply; 6+ messages in thread
From: Hou Tao @ 2021-12-01 7:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Yonghong Song, Daniel Borkmann, Andrii Nakryiko,
netdev, bpf, houtao1
An extra newline will output for bpf_log() with BPF_LOG_KERNEL level
as shown below:
[ 52.095704] BPF:The function test_3 has 12 arguments. Too many.
[ 52.095704]
[ 52.096896] Error in parsing func ptr test_3 in struct bpf_dummy_ops
Now all bpf_log() are ended by newline, but not all btf_verifier_log()
are ended by newline, so checking whether or not the log message
has the trailing newline and adding a newline if not.
Also there is no need to calculate the left userspace buffer size
for kernel log output and to truncate the output by '\0' which
has already been done by vscnprintf(), so only do these for
userspace log output.
Signed-off-by: Hou Tao <houtao1@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
kernel/bpf/verifier.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6cfd628ae3e4..722aea00d44e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -293,13 +293,15 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
"verifier log line truncated - local buffer too short\n");
- n = min(log->len_total - log->len_used - 1, n);
- log->kbuf[n] = '\0';
-
if (log->level == BPF_LOG_KERNEL) {
- pr_err("BPF:%s\n", log->kbuf);
+ bool newline = n > 0 && log->kbuf[n - 1] == '\n';
+
+ pr_err("BPF: %s%s", log->kbuf, newline ? "" : "\n");
return;
}
+
+ n = min(log->len_total - log->len_used - 1, n);
+ log->kbuf[n] = '\0';
if (!copy_to_user(log->ubuf + log->len_used, log->kbuf, n + 1))
log->len_used += n;
else
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level
2021-12-01 7:34 ` [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() " Hou Tao
@ 2021-12-01 17:50 ` Alexei Starovoitov
0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2021-12-01 17:50 UTC (permalink / raw)
To: Hou Tao
Cc: Alexei Starovoitov, Martin KaFai Lau, Yonghong Song,
Daniel Borkmann, Andrii Nakryiko, Network Development, bpf
On Tue, Nov 30, 2021 at 11:19 PM Hou Tao <houtao1@huawei.com> wrote:
>
> An extra newline will output for bpf_log() with BPF_LOG_KERNEL level
> as shown below:
>
> [ 52.095704] BPF:The function test_3 has 12 arguments. Too many.
> [ 52.095704]
> [ 52.096896] Error in parsing func ptr test_3 in struct bpf_dummy_ops
>
> Now all bpf_log() are ended by newline, but not all btf_verifier_log()
> are ended by newline, so checking whether or not the log message
> has the trailing newline and adding a newline if not.
>
> Also there is no need to calculate the left userspace buffer size
> for kernel log output and to truncate the output by '\0' which
> has already been done by vscnprintf(), so only do these for
> userspace log output.
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
Applied this patch. Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD)
2021-12-01 7:34 [PATCH bpf-next v4 0/2] clean-up for BPF_LOG_KERNEL log level Hou Tao
2021-12-01 7:34 ` [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() " Hou Tao
@ 2021-12-01 7:34 ` Hou Tao
2021-12-01 17:42 ` Alexei Starovoitov
1 sibling, 1 reply; 6+ messages in thread
From: Hou Tao @ 2021-12-01 7:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Yonghong Song, Daniel Borkmann, Andrii Nakryiko,
netdev, bpf, houtao1
BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
to set log level as BPF_LOG_KERNEL. The same checking has already
been done in bpf_check(), so factor out a helper to check the
validity of log attributes and use it in both places.
Fixes: 8580ac9404f6 ("bpf: Process in-kernel BTF")
Signed-off-by: Hou Tao <houtao1@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
include/linux/bpf_verifier.h | 7 +++++++
kernel/bpf/btf.c | 3 +--
kernel/bpf/verifier.c | 6 +++---
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c8a78e830fca..a3d17601a5a7 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -396,6 +396,13 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
log->level == BPF_LOG_KERNEL);
}
+static inline bool
+bpf_verifier_log_attr_valid(const struct bpf_verifier_log *log, u32 max_total)
+{
+ return log->len_total >= 128 && log->len_total <= max_total &&
+ log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK);
+}
+
#define BPF_MAX_SUBPROGS 256
struct bpf_subprog_info {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6b9d23be1e99..308c345cd811 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4472,8 +4472,7 @@ static struct btf *btf_parse(bpfptr_t btf_data, u32 btf_data_size,
log->len_total = log_size;
/* log attributes have to be sane */
- if (log->len_total < 128 || log->len_total > UINT_MAX >> 8 ||
- !log->level || !log->ubuf) {
+ if (!bpf_verifier_log_attr_valid(log, UINT_MAX >> 8)) {
err = -EINVAL;
goto errout;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 722aea00d44e..f128e6799cb5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13969,11 +13969,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr)
log->ubuf = (char __user *) (unsigned long) attr->log_buf;
log->len_total = attr->log_size;
- ret = -EINVAL;
/* log attributes have to be sane */
- if (log->len_total < 128 || log->len_total > UINT_MAX >> 2 ||
- !log->level || !log->ubuf || log->level & ~BPF_LOG_MASK)
+ if (!bpf_verifier_log_attr_valid(log, UINT_MAX >> 2)) {
+ ret = -EINVAL;
goto err_unlock;
+ }
}
if (IS_ERR(btf_vmlinux)) {
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD)
2021-12-01 7:34 ` [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD) Hou Tao
@ 2021-12-01 17:42 ` Alexei Starovoitov
2021-12-02 3:42 ` Hou Tao
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2021-12-01 17:42 UTC (permalink / raw)
To: Hou Tao
Cc: Alexei Starovoitov, Martin KaFai Lau, Yonghong Song,
Daniel Borkmann, Andrii Nakryiko, Network Development, bpf
On Tue, Nov 30, 2021 at 11:19 PM Hou Tao <houtao1@huawei.com> wrote:
>
> BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
> to set log level as BPF_LOG_KERNEL. The same checking has already
> been done in bpf_check(), so factor out a helper to check the
> validity of log attributes and use it in both places.
>
> Fixes: 8580ac9404f6 ("bpf: Process in-kernel BTF")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
> ---
> include/linux/bpf_verifier.h | 7 +++++++
> kernel/bpf/btf.c | 3 +--
> kernel/bpf/verifier.c | 6 +++---
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index c8a78e830fca..a3d17601a5a7 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -396,6 +396,13 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
> log->level == BPF_LOG_KERNEL);
> }
>
> +static inline bool
> +bpf_verifier_log_attr_valid(const struct bpf_verifier_log *log, u32 max_total)
> +{
> + return log->len_total >= 128 && log->len_total <= max_total &&
> + log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK);
> +}
> +
> #define BPF_MAX_SUBPROGS 256
>
> struct bpf_subprog_info {
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 6b9d23be1e99..308c345cd811 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4472,8 +4472,7 @@ static struct btf *btf_parse(bpfptr_t btf_data, u32 btf_data_size,
> log->len_total = log_size;
>
> /* log attributes have to be sane */
> - if (log->len_total < 128 || log->len_total > UINT_MAX >> 8 ||
> - !log->level || !log->ubuf) {
> + if (!bpf_verifier_log_attr_valid(log, UINT_MAX >> 8)) {
> err = -EINVAL;
> goto errout;
> }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 722aea00d44e..f128e6799cb5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13969,11 +13969,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr)
> log->ubuf = (char __user *) (unsigned long) attr->log_buf;
> log->len_total = attr->log_size;
>
> - ret = -EINVAL;
> /* log attributes have to be sane */
> - if (log->len_total < 128 || log->len_total > UINT_MAX >> 2 ||
> - !log->level || !log->ubuf || log->level & ~BPF_LOG_MASK)
> + if (!bpf_verifier_log_attr_valid(log, UINT_MAX >> 2)) {
> + ret = -EINVAL;
It's actually quite bad that we have this discrepancy in limits.
I've already sent a patch to make them the same.
It was a pain to debug.
https://lore.kernel.org/bpf/20211124060209.493-7-alexei.starovoitov@gmail.com/
"
Otherwise tools that progressively increase log size and use the same log
for BTF loading and program loading will be hitting hard to debug EINVAL.
"
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD)
2021-12-01 17:42 ` Alexei Starovoitov
@ 2021-12-02 3:42 ` Hou Tao
0 siblings, 0 replies; 6+ messages in thread
From: Hou Tao @ 2021-12-02 3:42 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Martin KaFai Lau, Yonghong Song,
Daniel Borkmann, Andrii Nakryiko, Network Development, bpf
Hi,
On 12/2/2021 1:42 AM, Alexei Starovoitov wrote:
> On Tue, Nov 30, 2021 at 11:19 PM Hou Tao <houtao1@huawei.com> wrote:
>> BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
>> to set log level as BPF_LOG_KERNEL. The same checking has already
>> been done in bpf_check(), so factor out a helper to check the
>> validity of log attributes and use it in both places.
>>
>>
snip
>> - ret = -EINVAL;
>> /* log attributes have to be sane */
>> - if (log->len_total < 128 || log->len_total > UINT_MAX >> 2 ||
>> - !log->level || !log->ubuf || log->level & ~BPF_LOG_MASK)
>> + if (!bpf_verifier_log_attr_valid(log, UINT_MAX >> 2)) {
>> + ret = -EINVAL;
> It's actually quite bad that we have this discrepancy in limits.
> I've already sent a patch to make them the same.
> It was a pain to debug.
> https://lore.kernel.org/bpf/20211124060209.493-7-alexei.starovoitov@gmail.com/
> "
> Otherwise tools that progressively increase log size and use the same log
> for BTF loading and program loading will be hitting hard to debug EINVAL.
> "
OK. Will send a single patch to handle that based on your patch set.
Regards,
Tao
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-02 3:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-01 7:34 [PATCH bpf-next v4 0/2] clean-up for BPF_LOG_KERNEL log level Hou Tao
2021-12-01 7:34 ` [PATCH bpf-next v4 1/2] bpf: clean-up bpf_verifier_vlog() " Hou Tao
2021-12-01 17:50 ` Alexei Starovoitov
2021-12-01 7:34 ` [PATCH bpf-next v4 2/2] bpf: disallow BPF_LOG_KERNEL log level for bpf(BPF_BTF_LOAD) Hou Tao
2021-12-01 17:42 ` Alexei Starovoitov
2021-12-02 3:42 ` Hou Tao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox