bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf 1/2] bpf: allow empty module BTFs
@ 2021-01-10  7:03 Andrii Nakryiko
  2021-01-10  7:03 ` [PATCH bpf 2/2] libbpf: allow loading empty BTFs Andrii Nakryiko
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-10  7:03 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii, kernel-team, Christopher William Snowhill

Some modules don't declare any new types and end up with an empty BTF,
containing only valid BTF header and no types or strings sections. This
currently causes BTF validation error. There is nothing wrong with such BTF,
so fix the issue by allowing module BTFs with no types or strings.

Reported-by: Christopher William Snowhill <chris@kode54.net>
Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 8d6bdb4f4d61..84a36ee4a4c2 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4172,7 +4172,7 @@ static int btf_parse_hdr(struct btf_verifier_env *env)
 		return -ENOTSUPP;
 	}
 
-	if (btf_data_size == hdr->hdr_len) {
+	if (!btf->base_btf && btf_data_size == hdr->hdr_len) {
 		btf_verifier_log(env, "No data");
 		return -EINVAL;
 	}
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-10  7:03 [PATCH bpf 1/2] bpf: allow empty module BTFs Andrii Nakryiko
@ 2021-01-10  7:03 ` Andrii Nakryiko
  2021-01-11 18:13   ` Yonghong Song
  2021-01-11 18:08 ` [PATCH bpf 1/2] bpf: allow empty module BTFs Yonghong Song
  2021-01-12 20:20 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-10  7:03 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii, kernel-team, Christopher William Snowhill

Empty BTFs do come up (e.g., simple kernel modules with no new types and
strings, compared to the vmlinux BTF) and there is nothing technically wrong
with them. So remove unnecessary check preventing loading empty BTFs.

Reported-by: Christopher William Snowhill <chris@kode54.net>
Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 3c3f2bc6c652..9970a288dda5 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
 	}
 
 	meta_left = btf->raw_size - sizeof(*hdr);
-	if (!meta_left) {
-		pr_debug("BTF has no data\n");
-		return -EINVAL;
-	}
-
 	if (meta_left < hdr->str_off + hdr->str_len) {
 		pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
 		return -EINVAL;
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 1/2] bpf: allow empty module BTFs
  2021-01-10  7:03 [PATCH bpf 1/2] bpf: allow empty module BTFs Andrii Nakryiko
  2021-01-10  7:03 ` [PATCH bpf 2/2] libbpf: allow loading empty BTFs Andrii Nakryiko
@ 2021-01-11 18:08 ` Yonghong Song
  2021-01-12 20:20 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2021-01-11 18:08 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: kernel-team, Christopher William Snowhill



On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> Some modules don't declare any new types and end up with an empty BTF,
> containing only valid BTF header and no types or strings sections. This
> currently causes BTF validation error. There is nothing wrong with such BTF,
> so fix the issue by allowing module BTFs with no types or strings.
> 
> Reported-by: Christopher William Snowhill <chris@kode54.net>
> Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-10  7:03 ` [PATCH bpf 2/2] libbpf: allow loading empty BTFs Andrii Nakryiko
@ 2021-01-11 18:13   ` Yonghong Song
  2021-01-11 20:51     ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2021-01-11 18:13 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: kernel-team, Christopher William Snowhill



On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> Empty BTFs do come up (e.g., simple kernel modules with no new types and
> strings, compared to the vmlinux BTF) and there is nothing technically wrong
> with them. So remove unnecessary check preventing loading empty BTFs.
> 
> Reported-by: Christopher William Snowhill <chris@kode54.net>
> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>   tools/lib/bpf/btf.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 3c3f2bc6c652..9970a288dda5 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
>   	}
>   
>   	meta_left = btf->raw_size - sizeof(*hdr);
> -	if (!meta_left) {
> -		pr_debug("BTF has no data\n");
> -		return -EINVAL;
> -	}

Previous kernel patch allows empty btf only if that btf is module (not 
base/vmlinux) btf. Here it seems we allow any empty non-module btf to be 
loaded into the kernel. In such cases, loading may fail? Maybe we should
detect such cases in libbpf and error out instead of going to kernel and
get error back?

> -
>   	if (meta_left < hdr->str_off + hdr->str_len) {
>   		pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
>   		return -EINVAL;
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-11 18:13   ` Yonghong Song
@ 2021-01-11 20:51     ` Andrii Nakryiko
  2021-01-12  1:15       ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-11 20:51 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Christopher William Snowhill

On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> > Empty BTFs do come up (e.g., simple kernel modules with no new types and
> > strings, compared to the vmlinux BTF) and there is nothing technically wrong
> > with them. So remove unnecessary check preventing loading empty BTFs.
> >
> > Reported-by: Christopher William Snowhill <chris@kode54.net>
> > Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   tools/lib/bpf/btf.c | 5 -----
> >   1 file changed, 5 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 3c3f2bc6c652..9970a288dda5 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
> >       }
> >
> >       meta_left = btf->raw_size - sizeof(*hdr);
> > -     if (!meta_left) {
> > -             pr_debug("BTF has no data\n");
> > -             return -EINVAL;
> > -     }
>
> Previous kernel patch allows empty btf only if that btf is module (not
> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
> loaded into the kernel. In such cases, loading may fail? Maybe we should
> detect such cases in libbpf and error out instead of going to kernel and
> get error back?

I did this consciously. Kernel is more strict, because there is no
reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
least not that now we have FUNCs in BTF). But allowing libbpf to load
empty BTF generically is helpful for bpftool, as one example, for
inspection. If you do `bpftool btf dump` on empty BTF, it will just
print nothing and you'll know that it's a valid (from BTF header
perspective) BTF, just doesn't have any types (besides VOID). If we
don't allow it, then we'll just get an error and then you'll have to
do painful hex dumping and decoding to see what's wrong.

In practice, no BPF program's BTF should be empty, but if it is, the
kernel will rightfully stop you. I don't think it's a common enough
case for libbpf to handle.

>
> > -
> >       if (meta_left < hdr->str_off + hdr->str_len) {
> >               pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
> >               return -EINVAL;
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-11 20:51     ` Andrii Nakryiko
@ 2021-01-12  1:15       ` Yonghong Song
  2021-01-12  6:41         ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2021-01-12  1:15 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Christopher William Snowhill



On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
> On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
>>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
>>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
>>> with them. So remove unnecessary check preventing loading empty BTFs.
>>>
>>> Reported-by: Christopher William Snowhill <chris@kode54.net>
>>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    tools/lib/bpf/btf.c | 5 -----
>>>    1 file changed, 5 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>> index 3c3f2bc6c652..9970a288dda5 100644
>>> --- a/tools/lib/bpf/btf.c
>>> +++ b/tools/lib/bpf/btf.c
>>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
>>>        }
>>>
>>>        meta_left = btf->raw_size - sizeof(*hdr);
>>> -     if (!meta_left) {
>>> -             pr_debug("BTF has no data\n");
>>> -             return -EINVAL;
>>> -     }
>>
>> Previous kernel patch allows empty btf only if that btf is module (not
>> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
>> loaded into the kernel. In such cases, loading may fail? Maybe we should
>> detect such cases in libbpf and error out instead of going to kernel and
>> get error back?
> 
> I did this consciously. Kernel is more strict, because there is no
> reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
> least not that now we have FUNCs in BTF). But allowing libbpf to load
> empty BTF generically is helpful for bpftool, as one example, for
> inspection. If you do `bpftool btf dump` on empty BTF, it will just
> print nothing and you'll know that it's a valid (from BTF header
> perspective) BTF, just doesn't have any types (besides VOID). If we
> don't allow it, then we'll just get an error and then you'll have to
> do painful hex dumping and decoding to see what's wrong.

It is totally okay to allow empty btf in libbpf. I just want to check
if this btf is going to be loaded into the kernel, right before it is 
loading whether libbpf could check whether it is a non-module empty btf
or not, if it is, do not go to kernel.

> 
> In practice, no BPF program's BTF should be empty, but if it is, the
> kernel will rightfully stop you. I don't think it's a common enough
> case for libbpf to handle.

In general, libbpf should catch errors earlier if possible without going
to kernel. This way, we can have better error messages for user.
But I won't insist in this case as it is indeed really rare.

> 
>>
>>> -
>>>        if (meta_left < hdr->str_off + hdr->str_len) {
>>>                pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
>>>                return -EINVAL;
>>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-12  1:15       ` Yonghong Song
@ 2021-01-12  6:41         ` Andrii Nakryiko
  2021-01-12 20:17           ` Daniel Borkmann
  0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-12  6:41 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Christopher William Snowhill

On Mon, Jan 11, 2021 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
> > On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> >>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
> >>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
> >>> with them. So remove unnecessary check preventing loading empty BTFs.
> >>>
> >>> Reported-by: Christopher William Snowhill <chris@kode54.net>
> >>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >>> ---
> >>>    tools/lib/bpf/btf.c | 5 -----
> >>>    1 file changed, 5 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >>> index 3c3f2bc6c652..9970a288dda5 100644
> >>> --- a/tools/lib/bpf/btf.c
> >>> +++ b/tools/lib/bpf/btf.c
> >>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
> >>>        }
> >>>
> >>>        meta_left = btf->raw_size - sizeof(*hdr);
> >>> -     if (!meta_left) {
> >>> -             pr_debug("BTF has no data\n");
> >>> -             return -EINVAL;
> >>> -     }
> >>
> >> Previous kernel patch allows empty btf only if that btf is module (not
> >> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
> >> loaded into the kernel. In such cases, loading may fail? Maybe we should
> >> detect such cases in libbpf and error out instead of going to kernel and
> >> get error back?
> >
> > I did this consciously. Kernel is more strict, because there is no
> > reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
> > least not that now we have FUNCs in BTF). But allowing libbpf to load
> > empty BTF generically is helpful for bpftool, as one example, for
> > inspection. If you do `bpftool btf dump` on empty BTF, it will just
> > print nothing and you'll know that it's a valid (from BTF header
> > perspective) BTF, just doesn't have any types (besides VOID). If we
> > don't allow it, then we'll just get an error and then you'll have to
> > do painful hex dumping and decoding to see what's wrong.
>
> It is totally okay to allow empty btf in libbpf. I just want to check
> if this btf is going to be loaded into the kernel, right before it is
> loading whether libbpf could check whether it is a non-module empty btf
> or not, if it is, do not go to kernel.

Ok, I see what you are proposing. We can do that, but it's definitely
separate from these bug fixes. But, to be honest, I wouldn't bother
because libbpf will return BTF verification log with a very readable
"No data" message in it.

>
> >
> > In practice, no BPF program's BTF should be empty, but if it is, the
> > kernel will rightfully stop you. I don't think it's a common enough
> > case for libbpf to handle.
>
> In general, libbpf should catch errors earlier if possible without going
> to kernel. This way, we can have better error messages for user.
> But I won't insist in this case as it is indeed really rare.

I wouldn't say in general. Rather in cases that commonly would cause
confusion. I don't think libbpf should grow into a massive "let's
double check everything before kernel" thing.

>
> >
> >>
> >>> -
> >>>        if (meta_left < hdr->str_off + hdr->str_len) {
> >>>                pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
> >>>                return -EINVAL;
> >>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-12  6:41         ` Andrii Nakryiko
@ 2021-01-12 20:17           ` Daniel Borkmann
  2021-01-12 20:26             ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Borkmann @ 2021-01-12 20:17 UTC (permalink / raw)
  To: Andrii Nakryiko, Yonghong Song
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Kernel Team, Christopher William Snowhill

On 1/12/21 7:41 AM, Andrii Nakryiko wrote:
> On Mon, Jan 11, 2021 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>> On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
>>> On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
>>>> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
>>>>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
>>>>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
>>>>> with them. So remove unnecessary check preventing loading empty BTFs.
>>>>>
>>>>> Reported-by: Christopher William Snowhill <chris@kode54.net>
>>>>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")

Fixed up Fixes tag ^^^^^ while applying. ;-)

>>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>>>> ---
>>>>>     tools/lib/bpf/btf.c | 5 -----
>>>>>     1 file changed, 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>>> index 3c3f2bc6c652..9970a288dda5 100644
>>>>> --- a/tools/lib/bpf/btf.c
>>>>> +++ b/tools/lib/bpf/btf.c
>>>>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
>>>>>         }
>>>>>
>>>>>         meta_left = btf->raw_size - sizeof(*hdr);
>>>>> -     if (!meta_left) {
>>>>> -             pr_debug("BTF has no data\n");
>>>>> -             return -EINVAL;
>>>>> -     }
>>>>
>>>> Previous kernel patch allows empty btf only if that btf is module (not
>>>> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
>>>> loaded into the kernel. In such cases, loading may fail? Maybe we should
>>>> detect such cases in libbpf and error out instead of going to kernel and
>>>> get error back?
>>>
>>> I did this consciously. Kernel is more strict, because there is no
>>> reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
>>> least not that now we have FUNCs in BTF). But allowing libbpf to load
>>> empty BTF generically is helpful for bpftool, as one example, for
>>> inspection. If you do `bpftool btf dump` on empty BTF, it will just
>>> print nothing and you'll know that it's a valid (from BTF header
>>> perspective) BTF, just doesn't have any types (besides VOID). If we
>>> don't allow it, then we'll just get an error and then you'll have to
>>> do painful hex dumping and decoding to see what's wrong.
>>
>> It is totally okay to allow empty btf in libbpf. I just want to check
>> if this btf is going to be loaded into the kernel, right before it is
>> loading whether libbpf could check whether it is a non-module empty btf
>> or not, if it is, do not go to kernel.
> 
> Ok, I see what you are proposing. We can do that, but it's definitely
> separate from these bug fixes. But, to be honest, I wouldn't bother
> because libbpf will return BTF verification log with a very readable
> "No data" message in it.

Right, seems okay to me for this particular case given the user will be
able to make some sense of it from the log.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 1/2] bpf: allow empty module BTFs
  2021-01-10  7:03 [PATCH bpf 1/2] bpf: allow empty module BTFs Andrii Nakryiko
  2021-01-10  7:03 ` [PATCH bpf 2/2] libbpf: allow loading empty BTFs Andrii Nakryiko
  2021-01-11 18:08 ` [PATCH bpf 1/2] bpf: allow empty module BTFs Yonghong Song
@ 2021-01-12 20:20 ` patchwork-bot+netdevbpf
  2021-01-24 10:27   ` Christopher William Snowhill
  2 siblings, 1 reply; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-01-12 20:20 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, kernel-team, chris

Hello:

This series was applied to bpf/bpf.git (refs/heads/master):

On Sat, 9 Jan 2021 23:03:40 -0800 you wrote:
> Some modules don't declare any new types and end up with an empty BTF,
> containing only valid BTF header and no types or strings sections. This
> currently causes BTF validation error. There is nothing wrong with such BTF,
> so fix the issue by allowing module BTFs with no types or strings.
> 
> Reported-by: Christopher William Snowhill <chris@kode54.net>
> Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: allow empty module BTFs
    https://git.kernel.org/bpf/bpf/c/bcc5e6162d66
  - [bpf,2/2] libbpf: allow loading empty BTFs
    https://git.kernel.org/bpf/bpf/c/b8d52264df85

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs
  2021-01-12 20:17           ` Daniel Borkmann
@ 2021-01-12 20:26             ` Andrii Nakryiko
  0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-12 20:26 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Yonghong Song, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Kernel Team, Christopher William Snowhill

On Tue, Jan 12, 2021 at 12:17 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/12/21 7:41 AM, Andrii Nakryiko wrote:
> > On Mon, Jan 11, 2021 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
> >> On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
> >>> On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
> >>>> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> >>>>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
> >>>>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
> >>>>> with them. So remove unnecessary check preventing loading empty BTFs.
> >>>>>
> >>>>> Reported-by: Christopher William Snowhill <chris@kode54.net>
> >>>>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
>
> Fixed up Fixes tag ^^^^^ while applying. ;-)

Oh the irony, eh? :) Thanks, Daniel!

>
> >>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >>>>> ---
> >>>>>     tools/lib/bpf/btf.c | 5 -----
> >>>>>     1 file changed, 5 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >>>>> index 3c3f2bc6c652..9970a288dda5 100644
> >>>>> --- a/tools/lib/bpf/btf.c
> >>>>> +++ b/tools/lib/bpf/btf.c
> >>>>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
> >>>>>         }
> >>>>>
> >>>>>         meta_left = btf->raw_size - sizeof(*hdr);
> >>>>> -     if (!meta_left) {
> >>>>> -             pr_debug("BTF has no data\n");
> >>>>> -             return -EINVAL;
> >>>>> -     }
> >>>>
> >>>> Previous kernel patch allows empty btf only if that btf is module (not
> >>>> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
> >>>> loaded into the kernel. In such cases, loading may fail? Maybe we should
> >>>> detect such cases in libbpf and error out instead of going to kernel and
> >>>> get error back?
> >>>
> >>> I did this consciously. Kernel is more strict, because there is no
> >>> reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
> >>> least not that now we have FUNCs in BTF). But allowing libbpf to load
> >>> empty BTF generically is helpful for bpftool, as one example, for
> >>> inspection. If you do `bpftool btf dump` on empty BTF, it will just
> >>> print nothing and you'll know that it's a valid (from BTF header
> >>> perspective) BTF, just doesn't have any types (besides VOID). If we
> >>> don't allow it, then we'll just get an error and then you'll have to
> >>> do painful hex dumping and decoding to see what's wrong.
> >>
> >> It is totally okay to allow empty btf in libbpf. I just want to check
> >> if this btf is going to be loaded into the kernel, right before it is
> >> loading whether libbpf could check whether it is a non-module empty btf
> >> or not, if it is, do not go to kernel.
> >
> > Ok, I see what you are proposing. We can do that, but it's definitely
> > separate from these bug fixes. But, to be honest, I wouldn't bother
> > because libbpf will return BTF verification log with a very readable
> > "No data" message in it.
>
> Right, seems okay to me for this particular case given the user will be
> able to make some sense of it from the log.
>
> Thanks,
> Daniel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 1/2] bpf: allow empty module BTFs
  2021-01-12 20:20 ` patchwork-bot+netdevbpf
@ 2021-01-24 10:27   ` Christopher William Snowhill
  2021-01-26  1:26     ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Christopher William Snowhill @ 2021-01-24 10:27 UTC (permalink / raw)
  To: patchwork-bot+netdevbpf, Andrii Nakryiko
  Cc: bpf, netdev, ast, daniel, kernel-team

When is this being applied to an actual kernel? 5.11 is still quite broken without these two patches. Unless you're not using a vfat EFI partition, I guess.

On Tue, Jan 12, 2021, at 12:20 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to bpf/bpf.git (refs/heads/master):
> 
> On Sat, 9 Jan 2021 23:03:40 -0800 you wrote:
> > Some modules don't declare any new types and end up with an empty BTF,
> > containing only valid BTF header and no types or strings sections. This
> > currently causes BTF validation error. There is nothing wrong with such BTF,
> > so fix the issue by allowing module BTFs with no types or strings.
> > 
> > Reported-by: Christopher William Snowhill <chris@kode54.net>
> > Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > 
> > [...]
> 
> Here is the summary with links:
>   - [bpf,1/2] bpf: allow empty module BTFs
>     https://git.kernel.org/bpf/bpf/c/bcc5e6162d66
>   - [bpf,2/2] libbpf: allow loading empty BTFs
>     https://git.kernel.org/bpf/bpf/c/b8d52264df85
> 
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> 
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 1/2] bpf: allow empty module BTFs
  2021-01-24 10:27   ` Christopher William Snowhill
@ 2021-01-26  1:26     ` Andrii Nakryiko
  2021-01-26  2:17       ` Christopher William Snowhill
  0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-01-26  1:26 UTC (permalink / raw)
  To: Christopher William Snowhill
  Cc: patchwork-bot+netdevbpf, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Sun, Jan 24, 2021 at 2:28 AM Christopher William Snowhill
<chris@kode54.net> wrote:
>
> When is this being applied to an actual kernel? 5.11 is still quite broken without these two patches. Unless you're not using a vfat EFI partition, I guess.
>

It's in v5.11-rc5.

> On Tue, Jan 12, 2021, at 12:20 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> > Hello:
> >
> > This series was applied to bpf/bpf.git (refs/heads/master):
> >
> > On Sat, 9 Jan 2021 23:03:40 -0800 you wrote:
> > > Some modules don't declare any new types and end up with an empty BTF,
> > > containing only valid BTF header and no types or strings sections. This
> > > currently causes BTF validation error. There is nothing wrong with such BTF,
> > > so fix the issue by allowing module BTFs with no types or strings.
> > >
> > > Reported-by: Christopher William Snowhill <chris@kode54.net>
> > > Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > >
> > > [...]
> >
> > Here is the summary with links:
> >   - [bpf,1/2] bpf: allow empty module BTFs
> >     https://git.kernel.org/bpf/bpf/c/bcc5e6162d66
> >   - [bpf,2/2] libbpf: allow loading empty BTFs
> >     https://git.kernel.org/bpf/bpf/c/b8d52264df85
> >
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> >
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf 1/2] bpf: allow empty module BTFs
  2021-01-26  1:26     ` Andrii Nakryiko
@ 2021-01-26  2:17       ` Christopher William Snowhill
  0 siblings, 0 replies; 13+ messages in thread
From: Christopher William Snowhill @ 2021-01-26  2:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: patchwork-bot+netdevbpf, Andrii Nakryiko, bpf, Networking,
	Alexei Starovoitov, Daniel Borkmann, Kernel Team

Aha, that was just released. Nice. I'll report this to the issue tracker where I had lodged the bpf patches for QoL use when testing rc4 and older.

On Mon, Jan 25, 2021, at 5:26 PM, Andrii Nakryiko wrote:
> On Sun, Jan 24, 2021 at 2:28 AM Christopher William Snowhill
> <chris@kode54.net> wrote:
> >
> > When is this being applied to an actual kernel? 5.11 is still quite broken without these two patches. Unless you're not using a vfat EFI partition, I guess.
> >
> 
> It's in v5.11-rc5.
> 
> > On Tue, Jan 12, 2021, at 12:20 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> > > Hello:
> > >
> > > This series was applied to bpf/bpf.git (refs/heads/master):
> > >
> > > On Sat, 9 Jan 2021 23:03:40 -0800 you wrote:
> > > > Some modules don't declare any new types and end up with an empty BTF,
> > > > containing only valid BTF header and no types or strings sections. This
> > > > currently causes BTF validation error. There is nothing wrong with such BTF,
> > > > so fix the issue by allowing module BTFs with no types or strings.
> > > >
> > > > Reported-by: Christopher William Snowhill <chris@kode54.net>
> > > > Fixes: 36e68442d1af ("bpf: Load and verify kernel module BTFs")
> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > >   - [bpf,1/2] bpf: allow empty module BTFs
> > >     https://git.kernel.org/bpf/bpf/c/bcc5e6162d66
> > >   - [bpf,2/2] libbpf: allow loading empty BTFs
> > >     https://git.kernel.org/bpf/bpf/c/b8d52264df85
> > >
> > > You are awesome, thank you!
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > >
> > >
> > >
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-01-26  5:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-10  7:03 [PATCH bpf 1/2] bpf: allow empty module BTFs Andrii Nakryiko
2021-01-10  7:03 ` [PATCH bpf 2/2] libbpf: allow loading empty BTFs Andrii Nakryiko
2021-01-11 18:13   ` Yonghong Song
2021-01-11 20:51     ` Andrii Nakryiko
2021-01-12  1:15       ` Yonghong Song
2021-01-12  6:41         ` Andrii Nakryiko
2021-01-12 20:17           ` Daniel Borkmann
2021-01-12 20:26             ` Andrii Nakryiko
2021-01-11 18:08 ` [PATCH bpf 1/2] bpf: allow empty module BTFs Yonghong Song
2021-01-12 20:20 ` patchwork-bot+netdevbpf
2021-01-24 10:27   ` Christopher William Snowhill
2021-01-26  1:26     ` Andrii Nakryiko
2021-01-26  2:17       ` Christopher William Snowhill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).