On Mon, Dec 14, 2020 at 09:51:27AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
>
> Fix a possible memory leak when a bind of an AF_XDP socket fails. When
> the fill and completion rings are created, they are tied to the
> socket. But when the buffer pool is later created at bind time, the
> ownership of these two rings are transferred to the buffer pool as
> they might be shared between sockets (and the buffer pool cannot be
> created until we know what we are binding to). So, before the buffer
> pool is created, these two rings are cleaned up with the socket, and
> after they have been transferred they are cleaned up together with
> the buffer pool.
>
> The problem is that ownership was transferred before it was absolutely
> certain that the buffer pool could be created and initialized
> correctly and when one of these errors occurred, the fill and
> completion rings did neither belong to the socket nor the pool and
> where therefore leaked. Solve this by moving the ownership transfer
> to the point where the buffer pool has been completely set up and
> there is no way it can fail.
>
> Fixes: 7361f9c3d719 ("xsk: Move fill and completion rings to buffer pool")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> Reported-by: syzbot+cfa88ddd0655afa88763@xxxxxxxxxxxxxxxxxxxxxxxxx
> ---
> net/xdp/xsk.c | 4 ++++
> net/xdp/xsk_buff_pool.c | 2 --
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 62504471fd20..189cfbbcccc0 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -772,6 +772,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> }
> }
>
> + /* FQ and CQ are now owned by the buffer pool and cleaned up with it. */
> + xs->fq_tmp = NULL;
> + xs->cq_tmp = NULL;
> +
> xs->dev = dev;
> xs->zc = xs->umem->zc;
> xs->queue_id = qid;
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index d5adeee9d5d9..46c2ae7d91d1 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -75,8 +75,6 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
>
> pool->fq = xs->fq_tmp;
> pool->cq = xs->cq_tmp;
> - xs->fq_tmp = NULL;
> - xs->cq_tmp = NULL;
Given this change, are there any circumstances that we could hit
xsk_release with xs->{f,c}q_tmp != NULL ?
>
> for (i = 0; i < pool->free_heads_cnt; i++) {
> xskb = &pool->heads[i];
>
> base-commit: d9838b1d39283c1200c13f9076474c7624b8ec34
> --
> 2.29.0
>
