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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Björn Töpel <bjorn.topel@intel.com> writes:

> 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@intel.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.

Yeah, I'd tend to agree with that. In general, I think the proliferation
of "just force-remove (or override) the running program" in code and
instructions has been a mistake; and application should only really be
adding and removing its own program...

-Toke