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

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] [day] [month] [year] [list]

Message-ID: <CAJ+HfNi=NZKsrjM5dJ-4TE1o8WonCqJyien3G+Jh6LsuF0SJXg@mail.gmail.com>
Date:   Thu, 29 Oct 2020 09:35:34 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Magnus Karlsson <magnus.karlsson@...il.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
Subject: Re: [PATCH bpf] xsk: fix possible memory leak at socket close

On Wed, 28 Oct 2020 at 08:32, Magnus Karlsson <magnus.karlsson@...il.com> wrote:
>
> From: Magnus Karlsson <magnus.karlsson@...el.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@...kaller.appspotmail.com
> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>

Acked-by: Björn Töpel <bjorn.topel@...el.com>