Re: [PATCH bpf] xsk: fix possible memory leak at socket close

From: "Björn Töpel" <bjorn.topel@gmail.com>
To: Magnus Karlsson <magnus.karlsson@gmail.com>
Cc: "Karlsson, Magnus" <magnus.karlsson@intel.com>,
	"Björn Töpel" <bjorn.topel@intel.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	Netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	"Fijalkowski, Maciej" <maciej.fijalkowski@intel.com>,
	"Maciej Fijalkowski" <maciejromanfijalkowski@gmail.com>
Subject: Re: [PATCH bpf] xsk: fix possible memory leak at socket close
Date: Thu, 29 Oct 2020 09:35:34 +0100	[thread overview]
Message-ID: <CAJ+HfNi=NZKsrjM5dJ-4TE1o8WonCqJyien3G+Jh6LsuF0SJXg@mail.gmail.com> (raw)
In-Reply-To: <1603801921-2712-1-git-send-email-magnus.karlsson@gmail.com>

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>