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

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:   Thu, 30 Jun 2022 11:58:24 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>
Subject: Re: [PATCH bpf-next 4/4] selftests: xsk: destroy BPF resources only
 when ctx refcount drops to 0

On Wed, Jun 29, 2022 at 4:39 PM Maciej Fijalkowski
<maciej.fijalkowski@...el.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@...el.com>

> Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.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
>