| Message ID | 20210110070341.1380086-2-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf,1/2] bpf: allow empty module BTFs | expand |
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; >
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; > >
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; >>>
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; > >>>
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
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
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;
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(-)