netdev - Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

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: <20200320163514.5f26d547@kicinski-fedora-PC1C0HJN>
Date:   Fri, 20 Mar 2020 16:35:14 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Andrey Ignatov <rdna@...com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing
 program when attaching XDP

On Fri, 20 Mar 2020 22:55:43 +0100 Daniel Borkmann wrote:
> >> Another aspect that falls into this atomic replacement is also that the programs can
> >> actually be atomically replaced at runtime. Last time I looked, some drivers still do
> >> a down/up cycle on replacement and hence traffic would be interrupted. I would argue
> >> that such /atomic/ swap operation on bpf_link would cover a guarantee of not having to
> >> perform this as well (workaround today would be a simple tail call map as entry point).  
> > 
> > I don't think that's the case. Drivers generally have a fast path
> > for the active-active replace.
> > 
> > Up/Down is only done to remap DMA buffers and change RX buffer
> > allocation scheme. That's when program is installed or removed,
> > not replaced.  
> 
> I know; though it seems not all adhere to that scheme sadly. I don't have that HW so can
> only judge on the code, but one example that looked suspicious enough to me is qede_xdp().
> It calls qede_xdp_set(), which does a qede_reload() for /every/ prog update. The latter
> basically does ...
> 
>      if (edev->state == QEDE_STATE_OPEN) {
>          qede_unload(edev, QEDE_UNLOAD_NORMAL, true);
>          if (args)
>              args->func(edev, args);               <-- prog replace here
>          qede_load(edev, QEDE_LOAD_RELOAD, true);
>          [...]
>      }

Ack, one day maybe we can restructure things enough so that drivers
don't have to copy/paste this dance :(

> ... now that is one driver. I haven't checked all the others (aside from i40e/ixgbe/mlx4/
> mlx5/nfp), but in any case it's also fixable in the driver w/o the extra need for bpf_link.

Agreed