netdev - Re: [PATCH bpf-next 03/10] bpf: Implement link update for tc BPF link programs

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: <CAEf4BzbBtgXStX5RUVa8s9kh23bQoaTvi-3Va-cE6qMH-hiDAQ@mail.gmail.com>
Date:   Wed, 5 Oct 2022 20:19:25 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org, razor@...ckwall.org, ast@...nel.org,
        andrii@...nel.org, martin.lau@...ux.dev, john.fastabend@...il.com,
        joannelkoong@...il.com, memxor@...il.com, toke@...hat.com,
        joe@...ium.io, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 03/10] bpf: Implement link update for tc BPF link programs

On Tue, Oct 4, 2022 at 4:12 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Add support for LINK_UPDATE command for tc BPF link to allow for a reliable
> replacement of the underlying BPF program.
>
> Co-developed-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> ---
>  kernel/bpf/net.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/kernel/bpf/net.c b/kernel/bpf/net.c
> index 22b7a9b05483..c50bcf656b3f 100644
> --- a/kernel/bpf/net.c
> +++ b/kernel/bpf/net.c
> @@ -303,6 +303,39 @@ static int __xtc_link_attach(struct bpf_link *l, u32 id)
>         return ret;
>  }
>
> +static int xtc_link_update(struct bpf_link *l, struct bpf_prog *nprog,
> +                          struct bpf_prog *oprog)
> +{
> +       struct bpf_tc_link *link = container_of(l, struct bpf_tc_link, link);
> +       int ret = 0;
> +
> +       rtnl_lock();
> +       if (!link->dev) {
> +               ret = -ENOLINK;
> +               goto out;
> +       }
> +       if (oprog && l->prog != oprog) {
> +               ret = -EPERM;
> +               goto out;
> +       }
> +       oprog = l->prog;
> +       if (oprog == nprog) {
> +               bpf_prog_put(nprog);
> +               goto out;
> +       }
> +       ret = __xtc_prog_attach(link->dev, link->location == BPF_NET_INGRESS,
> +                               XTC_MAX_ENTRIES, l->id, nprog, link->priority,
> +                               BPF_F_REPLACE);
> +       if (ret == link->priority) {

prog_attach returning priority is quite confusing. I think it's
because we support specifying zero and letting kernel pick priority,
so we need to communicate it back, is that right? If yes, can you
please add comment to xtc_prog_attach explaining this behavior?

and also, here if it's not an error then priority *has* to be equal to
link->priority, right? So:

if (ret < 0)
    goto out;

oprog = xchg(...)
bpf_prog_put(...)
ret = 0;

would be easier to follow, otherwise we are left wondering what
happens when ret > 0 && ret != link->priority. If you are worried of
bugs, BUG_ON/WARN_ON if ret != link->priority?


> +               oprog = xchg(&l->prog, nprog);
> +               bpf_prog_put(oprog);
> +               ret = 0;
> +       }
> +out:
> +       rtnl_unlock();
> +       return ret;
> +}
> +
>  static void xtc_link_release(struct bpf_link *l)
>  {
>         struct bpf_tc_link *link = container_of(l, struct bpf_tc_link, link);
> @@ -327,6 +360,7 @@ static void xtc_link_dealloc(struct bpf_link *l)
>  static const struct bpf_link_ops bpf_tc_link_lops = {
>         .release        = xtc_link_release,
>         .dealloc        = xtc_link_dealloc,
> +       .update_prog    = xtc_link_update,
>  };
>
>  int xtc_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> --
> 2.34.1
>