On Fri, Jun 22, 2018 at 11:56 AM, Jakub Kicinski
<jakub.kicin...@netronome.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.kicin...@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.mon...@netronome.com>
Acked-by: Song Liu <songliubrav...@fb.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
>
