netdev - Re: [PATCH bpf] nfp: bpf: don't stop offload if replace failed

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: <CAPhsuW4x0SggOYXFGs4xpYE_em-LML-gB6sF0zbmR2Te9Nm2SQ@mail.gmail.com>
Date:   Sun, 24 Jun 2018 22:02:24 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, oss-drivers@...ronome.com
Subject: Re: [PATCH bpf] nfp: bpf: don't stop offload if replace failed

On Fri, Jun 22, 2018 at 11:56 AM, Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
> Stopping offload completely if replace of program failed dates
> back to days of transparent offload.  Back then we wanted to
> silently fall back to the in-driver processing.  Today we mark
> programs for offload when they are loaded into the kernel, so
> the transparent offload is no longer a reality.
>
> Flags check in the driver will only allow replace of a driver
> program with another driver program or an offload program with
> another offload program.
>
> When driver program is replaced stopping offload is a no-op,
> because driver program isn't offloaded.  When replacing
> offloaded program if the offload fails the entire operation
> will fail all the way back to user space and we should continue
> using the old program.  IOW when replacing a driver program
> stopping offload is unnecessary and when replacing offloaded
> program - it's a bug, old program should continue to run.
>
> In practice this bug would mean that if offload operation was to
> fail (either due to FW communication error, kernel OOM or new
> program being offloaded but for a different netdev) driver
> would continue reporting that previous XDP program is offloaded
> but in fact no program will be loaded in hardware.  The failure
> is fairly unlikely (found by inspection, when working on the code)
> but it's unpleasant.
>
> Backport note: even though the bug was introduced in commit
> cafa92ac2553 ("nfp: bpf: add support for XDP_FLAGS_HW_MODE"),
> this fix depends on commit 441a33031fe5 ("net: xdp: don't allow
> device-bound programs in driver mode"), so this fix is sufficient
> only in v4.15 or newer.  Kernels v4.13.x and v4.14.x do need to
> stop offload if it was transparent/opportunistic, i.e. if
> XDP_FLAGS_HW_MODE was not set on running program.
>
> Fixes: cafa92ac2553 ("nfp: bpf: add support for XDP_FLAGS_HW_MODE")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>

Acked-by: Song Liu <songliubraving@...com>


> ---
>  drivers/net/ethernet/netronome/nfp/bpf/main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
> index fcdfb8e7fdea..7184af94aa53 100644
> --- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
> +++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
> @@ -81,10 +81,10 @@ nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
>
>         ret = nfp_net_bpf_offload(nn, prog, running, extack);
>         /* Stop offload if replace not possible */
> -       if (ret && prog)
> -               nfp_bpf_xdp_offload(app, nn, NULL, extack);
> +       if (ret)
> +               return ret;
>
> -       nn->dp.bpf_offload_xdp = prog && !ret;
> +       nn->dp.bpf_offload_xdp = !!prog;
>         return ret;
>  }
>
> --
> 2.17.1
>