Björn Töpel <bjorn.topel@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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
