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]

Date:   Tue, 16 Feb 2021 10:20:52 +0100
From:   Björn Töpel <bjorn.topel@...el.com>
To:     John Fastabend <john.fastabend@...il.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        daniel@...earbox.net, ast@...nel.org, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Cc:     andrii@...nel.org, toke@...hat.com, magnus.karlsson@...el.com,
        ciara.loftus@...el.com
Subject: Re: [PATCH bpf-next 1/3] libbpf: xsk: use bpf_link

On 2021-02-15 21:49, John Fastabend wrote:
> Maciej Fijalkowski wrote:
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> If there's no bpf_link yet, create one for a given XDP prog and unload
>> explicitly existing prog if XDP_FLAGS_UPDATE_IF_NOEXIST is not set.
>>
>> If bpf_link is already at a given ifindex and underlying program is not
>> AF-XDP one, bail out or update the bpf_link's prog given the presence of
>> XDP_FLAGS_UPDATE_IF_NOEXIST.
>>
>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
>> ---
>>   tools/lib/bpf/xsk.c | 143 +++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 122 insertions(+), 21 deletions(-)
> 
> [...]
> 
>> +static int xsk_create_bpf_link(struct xsk_socket *xsk)
>> +{
>> +	/* bpf_link only accepts XDP_FLAGS_MODES, but xsk->config.xdp_flags
>> +	 * might have set XDP_FLAGS_UPDATE_IF_NOEXIST
>> +	 */
>> +	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
>> +			    .flags = (xsk->config.xdp_flags & XDP_FLAGS_MODES));
>> +	struct xsk_ctx *ctx = xsk->ctx;
>> +	__u32 prog_id;
>> +	int link_fd;
>> +	int err;
>> +
>> +	/* for !XDP_FLAGS_UPDATE_IF_NOEXIST, unload the program first, if any,
>> +	 * so that bpf_link can be attached
>> +	 */
>> +	if (!(xsk->config.xdp_flags & XDP_FLAGS_UPDATE_IF_NOEXIST)) {
>> +		err = bpf_get_link_xdp_id(ctx->ifindex, &prog_id, xsk->config.xdp_flags);
>> +		if (err) {
>> +			pr_warn("getting XDP prog id failed\n");
>> +			return err;
>> +		}
>> +		if (prog_id) {
>> +			err = bpf_set_link_xdp_fd(ctx->ifindex, -1, 0);
>> +			if (err < 0) {
>> +				pr_warn("detaching XDP prog failed\n");
>> +				return err;
>> +			}
>> +		}
>>   	}
>>   
>> -	ctx->prog_fd = prog_fd;
>> +	link_fd = bpf_link_create(ctx->prog_fd, xsk->ctx->ifindex, BPF_XDP, &opts);
>> +	if (link_fd < 0) {
>> +		pr_warn("bpf_link_create failed: %s\n", strerror(errno));
>> +		return link_fd;
>> +	}
>> +
> 
> This can leave the system in a bad state where it unloaded the XDP program
> above, but then failed to create the link. So we should somehow fix that
> if possible or at minimum put a note somewhere so users can't claim they
> shouldn't know this.
> 
> Also related, its not good for real systems to let XDP program go missing
> for some period of time. I didn't check but we should make
> XDP_FLAGS_UPDATE_IF_NOEXIST the default if its not already.
>

This is the default for XDP sockets library. The
"bpf_set_link_xdp_fd(...-1)" way is only when a user sets it explicitly.
One could maybe argue that the "force remove" would be out of scope for
AF_XDP; Meaning that if an XDP program is running, attached via netlink,
the AF_XDP library simply cannot remove it. The user would need to rely
on some other mechanism.


Björn


[...]