Re: [PATCH bpf-next 4/4] selftests: xsk: destroy BPF resources only when ctx refcount drops to 0

From: Magnus Karlsson <magnus.karlsson@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>
Subject: Re: [PATCH bpf-next 4/4] selftests: xsk: destroy BPF resources only when ctx refcount drops to 0
Date: Thu, 30 Jun 2022 11:58:24 +0200	[thread overview]
Message-ID: <CAJ8uoz2Jc1=O4-BJ52QgijgD5fR3As+CXLRpeync=25hc-NDoA@mail.gmail.com> (raw)
In-Reply-To: <20220629143458.934337-5-maciej.fijalkowski@intel.com>

On Wed, Jun 29, 2022 at 4:39 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> Currently, xsk_socket__delete frees BPF resources regardless of ctx
> refcount. Xdpxceiver has a test to verify whether underlying BPF
> resources would not be wiped out after closing XSK socket that was bound
> to interface with other active sockets. From library's xsk part
> perspective it also means that the internal xsk context is shared and
> its refcount is bumped accordingly.
>
> After a switch to loading XDP prog based on previously opened XSK
> socket, mentioned xdpxceiver test fails with:
> not ok 16 [xdpxceiver.c:swap_xsk_resources:1334]: ERROR: 9/"Bad file descriptor
>
> which means that in swap_xsk_resources(), xsk_socket__delete() released
> xskmap which in turn caused a failure of xsk_socket__update_xskmap().
>
> To fix this, when deleting socket, decrement ctx refcount before
> releasing BPF resources and do so only when refcount dropped to 0 which
> means there are no more active sockets for this ctx so BPF resources can
> be freed safely.

Please fix this in libxdp too as the bug is present there also.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

> Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>  tools/testing/selftests/bpf/xsk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
> index db911127720e..95ce5cd572bb 100644
> --- a/tools/testing/selftests/bpf/xsk.c
> +++ b/tools/testing/selftests/bpf/xsk.c
> @@ -1156,8 +1156,6 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
>                 goto out_mmap_tx;
>         }
>
> -       ctx->prog_fd = -1;
> -
>         if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>                 err = __xsk_setup_xdp_prog(xsk, NULL);
>                 if (err)
> @@ -1238,7 +1236,10 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>
>         ctx = xsk->ctx;
>         umem = ctx->umem;
> -       if (ctx->prog_fd != -1) {
> +
> +       xsk_put_ctx(ctx, true);
> +
> +       if (!ctx->refcount) {
>                 xsk_delete_bpf_maps(xsk);
>                 close(ctx->prog_fd);
>                 if (ctx->has_bpf_link)
> @@ -1257,7 +1258,6 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>                 }
>         }
>
> -       xsk_put_ctx(ctx, true);
>
>         umem->refcount--;
>         /* Do not close an fd that also has an associated umem connected
> --
> 2.27.0
>