| Message ID | 1603801921-2712-1-git-send-email-magnus.karlsson@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf] xsk: fix possible memory leak at socket close | expand |
On Wed, 28 Oct 2020 at 08:32, Magnus Karlsson <magnus.karlsson@gmail.com> wrote: > > From: Magnus Karlsson <magnus.karlsson@intel.com> > > Fix a possible memory leak at xsk socket close that is caused by the > refcounting of the umem object being wrong. The reference count of the > umem was decremented only after the pool had been freed. Note that if > the buffer pool is destroyed, it is important that the umem is > destroyed after the pool, otherwise the umem would disappear while the > driver is still running. And as the buffer pool needs to be destroyed > in a work queue, the umem is also (if its refcount reaches zero) > destroyed after the buffer pool in that same work queue. > > What was missing is that the refcount also needs to be decremented > when the pool is not freed and when the pool has not even been > created. The first case happens when the refcount of the pool is > higher than 1, i.e. it is still being used by some other socket using > the same device and queue id. In this case, it is safe to decrement > the refcount of the umem outside of the work queue as the umem will > never be freed because the refcount of the umem is always greater than > or equal to the refcount of the buffer pool. The second case is if the > buffer pool has not been created yet, i.e. the socket was closed > before it was bound but after the umem was created. In this case, it > is safe to destroy the umem outside of the work queue, since there is > no pool that can use it by definition. > > Fixes: 1c1efc2af158 ("xsk: Create and free buffer pool independently from umem") > Reported-by: syzbot+eb71df123dc2be2c1456@syzkaller.appspotmail.com > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com> Acked-by: Björn Töpel <bjorn.topel@intel.com>
diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h index 0140d08..01755b8 100644 --- a/include/net/xsk_buff_pool.h +++ b/include/net/xsk_buff_pool.h @@ -86,7 +86,7 @@ int xp_assign_dev_shared(struct xsk_buff_pool *pool, struct xdp_umem *umem, void xp_destroy(struct xsk_buff_pool *pool); void xp_release(struct xdp_buff_xsk *xskb); void xp_get_pool(struct xsk_buff_pool *pool); -void xp_put_pool(struct xsk_buff_pool *pool); +bool xp_put_pool(struct xsk_buff_pool *pool); void xp_clear_dev(struct xsk_buff_pool *pool); void xp_add_xsk(struct xsk_buff_pool *pool, struct xdp_sock *xs); void xp_del_xsk(struct xsk_buff_pool *pool, struct xdp_sock *xs); diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index b71a32e..cfbec39 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -1146,7 +1146,8 @@ static void xsk_destruct(struct sock *sk) if (!sock_flag(sk, SOCK_DEAD)) return; - xp_put_pool(xs->pool); + if (!xp_put_pool(xs->pool)) + xdp_put_umem(xs->umem); sk_refcnt_debug_dec(sk); } diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c index 64c9e55..8a3bf4e 100644 --- a/net/xdp/xsk_buff_pool.c +++ b/net/xdp/xsk_buff_pool.c @@ -251,15 +251,18 @@ void xp_get_pool(struct xsk_buff_pool *pool) refcount_inc(&pool->users); } -void xp_put_pool(struct xsk_buff_pool *pool) +bool xp_put_pool(struct xsk_buff_pool *pool) { if (!pool) - return; + return false; if (refcount_dec_and_test(&pool->users)) { INIT_WORK(&pool->work, xp_release_deferred); schedule_work(&pool->work); + return true; } + + return false; } static struct xsk_dma_map *xp_find_dma_map(struct xsk_buff_pool *pool)