Björn Töpel [off-list ref] writes:

On 2021-02-15 21:49, John Fastabend wrote:

quoted

Maciej Fijalkowski wrote:

quoted

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(-)

[...]

quoted

+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