netdev - Re: [PATCH bpf] xsk: Initialise xskb free_list

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]

Message-ID: <CAJ8uoz2-jZTqT_XkP6T2c0VAzC=QcENr2dJrE5ZivUx8Ak_6ZA@mail.gmail.com>
Date:   Tue, 21 Dec 2021 08:33:40 +0100
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Ciara Loftus <ciara.loftus@...el.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf] xsk: Initialise xskb free_list_node

On Mon, Dec 20, 2021 at 9:10 PM Ciara Loftus <ciara.loftus@...el.com> wrote:
>
> This commit initialises the xskb's free_list_node when the xskb is
> allocated. This prevents a potential false negative returned from a call
> to list_empty for that node, such as the one introduced in commit
> 199d983bc015 ("xsk: Fix crash on double free in buffer pool")
>
> In my environment this issue caused packets to not be received by
> the xdpsock application if the traffic was running prior to application
> launch. This happened when the first batch of packets failed the xskmap
> lookup and XDP_PASS was returned from the bpf program. This action is
> handled in the i40e zc driver (and others) by allocating an skbuff,
> freeing the xdp_buff and adding the associated xskb to the
> xsk_buff_pool's free_list if it hadn't been added already. Without this
> fix, the xskb is not added to the free_list because the check to determine
> if it was added already returns an invalid positive result. Later, this
> caused allocation errors in the driver and the failure to receive packets.

Thank you for this fix Ciara! Though I do think the Fixes tag should
be the one above: 199d983bc015 ("xsk: Fix crash on double free in
buffer pool"). Before that commit, there was no test for an empty list
in the xp_free path. The entry was unconditionally put on the list and
"initialized" in that way, so that code will work without this patch.
What do you think?

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

> Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
>
> Signed-off-by: Ciara Loftus <ciara.loftus@...el.com>
> ---
>  net/xdp/xsk_buff_pool.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index bc4ad48ea4f0..fd39bb660ebc 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -83,6 +83,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
>                 xskb = &pool->heads[i];
>                 xskb->pool = pool;
>                 xskb->xdp.frame_sz = umem->chunk_size - umem->headroom;
> +               INIT_LIST_HEAD(&xskb->free_list_node);
>                 if (pool->unaligned)
>                         pool->free_heads[i] = xskb;
>                 else
> --
> 2.17.1
>