netdev - Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzbM4-PvOgro-SjHx6h2ndYndSNkbQTA6xq74W=PuPTpjA@mail.gmail.com>
Date:   Tue, 14 Apr 2020 11:24:18 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type
 on program load if supported

On Tue, Apr 14, 2020 at 10:56 AM Andrey Ignatov <rdna@...com> wrote:
>
> Andrii Nakryiko <andriin@...com> [Mon, 2020-04-13 21:56 -0700]:
> > For some types of BPF programs that utilize expected_attach_type, libbpf won't
> > set load_attr.expected_attach_type, even if expected_attach_type is known from
> > section definition. This was done to preserve backwards compatibility with old
> > kernels that didn't recognize expected_attach_type attribute yet (which was
> > added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
> > is problematic for some BPF programs that utilize never features that require
> > kernel to know specific expected_attach_type (e.g., extended set of return
> > codes for cgroup_skb/egress programs).
> >
> > This patch makes libbpf specify expected_attach_type by default, but also
> > detect support for this field in kernel and not set it during program load.
> > This allows to have a good metadata for bpf_program
> > (e.g., bpf_program__get_extected_attach_type()), but still work with old
> > kernels (for cases where it can work at all).
> >
> > Additionally, due to expected_attach_type being always set for recognized
> > program types, bpf_program__attach_cgroup doesn't have to do extra checks to
> > determine correct attach type, so remove that additional logic.
> >
> > Also adjust section_names selftest to account for this change.
> >
> > More detailed discussion can be found in [0].
> >
> >   [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/
> >
> > Reported-by: Andrey Ignatov <rdna@...com>
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> > v1->v2:
> > - fixed prog_type/expected_attach_type combo (Andrey);
> > - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
>
> Thanks for changes.
>
> I built the patch (removing the double .sec Song mentioned since it
> breaks compilation) and tested it: it fixes the problem with NET_XMIT_CN

Wait, what? How does it break compilation? I compiled and tested
before submitting and just cleaned and built again, no compilation
errors or even warnings. Can you share compilation error you got,
please?

> on old kernels and works for me with cgroup skb on old kernels.
>
> Thank you!
>
> Acked-by: Andrey Ignatov <rdna@...com>

Thanks!

>
> I guess we can deal with selftest separately in the original thread.

Sure, if this is going to be applied to bpf as a fix, I'd rather
follow-up with selftests separately.

>
> Also a question about bpf vs bpf-next: since this fixes real problem
> with loading cgroup skb programs, should it go to bpf tree instead?

It will be up to maintainers, it's not so clear whether it's a new
feature or a bug fix.. I don't mind either way.

>
>
> >  tools/lib/bpf/libbpf.c                        | 127 ++++++++++++------
> >  .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
> >  2 files changed, 110 insertions(+), 59 deletions(-)
> >

[...]

trimming :)