netdev - Re: [PATCH bpf-next 1/3] libbpf: xsk: use bpf

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: <20210216021009.GB9572@ranger.igk.intel.com>
Date:   Tue, 16 Feb 2021 03:10:09 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        daniel@...earbox.net, ast@...nel.org, bpf@...r.kernel.org,
        netdev@...r.kernel.org, andrii@...nel.org,
        magnus.karlsson@...el.com, ciara.loftus@...el.com
Subject: Re: [PATCH bpf-next 1/3] libbpf: xsk: use bpf_link

On Mon, Feb 15, 2021 at 12:22:36PM -0800, John Fastabend wrote:
> Björn Töpel wrote:
> > On 2021-02-15 18:07, Toke Høiland-Jørgensen wrote:
> > > Maciej Fijalkowski <maciej.fijalkowski@...el.com> writes:
> > > 
> > >> Currently, if there are multiple xdpsock instances running on a single
> > >> interface and in case one of the instances is terminated, the rest of
> > >> them are left in an inoperable state due to the fact of unloaded XDP
> > >> prog from interface.
> 
> I'm a bit confused by the above. This is only the case if the instance
> that terminated is the one that loaded the XDP program and it also didn't
> pin the program correct? If so lets make the commit message a bit more
> clear about the exact case we are solving.

I can include the following from the cover letter:

<quote>
$ sudo ./xdpsock -i ens801f0 -t -q 10 // load xdp prog and xskmap and
                                      // add entry to xskmap at idx 10

$ sudo ./xdpsock -i ens801f0 -t -q 11 // add entry to xskmap at idx 11

terminate one of the processes and another one is unable to work due to
the fact that the XDP prog was unloaded from interface.
</quote>

We don't do the prog pinning and it doesn't really matter which of the
instances you terminate.

> 
> > >>
> > >> To address that, step away from setting bpf prog in favour of bpf_link.
> > >> This means that refcounting of BPF resources will be done automatically
> > >> by bpf_link itself.
> 
> +1
> 
> > >>
> > >> When setting up BPF resources during xsk socket creation, check whether
> > >> bpf_link for a given ifindex already exists via set of calls to
> > >> bpf_link_get_next_id -> bpf_link_get_fd_by_id -> bpf_obj_get_info_by_fd
> > >> and comparing the ifindexes from bpf_link and xsk socket.
> > > 
> > > One consideration here is that bpf_link_get_fd_by_id() is a privileged
> > > operation (privileged as in CAP_SYS_ADMIN), so this has the side effect
> > > of making AF_XDP privileged as well. Is that the intention?
> > >
> > 
> > We're already using, e.g., bpf_map_get_fd_by_id() which has that
> > as well. So we're assuming that for XDP setup already!
> > 
> > > Another is that the AF_XDP code is in the process of moving to libxdp
> > > (see in-progress PR [0]), and this approach won't carry over as-is to
> > > that model, because libxdp has to pin the bpf_link fds.
> > >
> > 
> > I was assuming there were two modes of operations for AF_XDP in libxdp.
> > One which is with the multi-program support (which AFAIK is why the
> > pinning is required), and one "like the current libbpf" one. For the
> > latter Maciej's series would be a good fit, no?
> > 
> > > However, in libxdp we can solve the original problem in a different way,
> > > and in fact I already suggested to Magnus that we should do this (see
> > > [1]); so one way forward could be to address it during the merge in
> > > libxdp? It should be possible to address the original issue (two
> > > instances of xdpsock breaking each other when they exit), but
> > > applications will still need to do an explicit unload operation before
> > > exiting (i.e., the automatic detach on bpf_link fd closure will take
> > > more work, and likely require extending the bpf_link kernel support)...
> > >
> > 
> > I'd say it's depending on the libbpf 1.0/libxdp merge timeframe. If
> > we're months ahead, then I'd really like to see this in libbpf until the
> > merge. However, I'll leave that for Magnus/you to decide!
> 
> Did I miss some thread? What does this mean libbpf 1.0/libxdp merge? From
> my side libbpf should support the basic operations: load, attach, pin,
> and link for all my BPF objects. I view libxdp as providing 'extra'
> goodness on top of that. Everyone agree?
> 
> > 
> > Bottom line; I'd *really* like bpf_link behavior (process scoped) for
> > AF_XDP sooner than later! ;-)
> 
> Because I use libbpf as my base management for BPF objects I want it
> to support the basic ops for all objects so link ops should land there.
> 
> > 
> > 
> > Thanks for the input!
> > Björn
> > 
> > 
> > > -Toke
> > > 
> > > [0] https://github.com/xdp-project/xdp-tools/pull/92
> > > [1] https://github.com/xdp-project/xdp-tools/pull/92#discussion_r576204719
> > > 
> 
>