netdev - Re: [PATCH bpf 0/5] fix sockmap + stream af

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: <87v88bvk0a.fsf@cloudflare.com>
Date: Tue, 02 Jan 2024 16:18:29 +0100
From: Jakub Sitnicki <jakub@...udflare.com>
To: John Fastabend <john.fastabend@...il.com>
Cc: rivendell7@...il.com, kuniyu@...zon.com, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf 0/5] fix sockmap + stream  af_unix memleak

On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote:
> There was a memleak when streaming af_unix sockets were inserted into
> multiple sockmap slots and/or maps. This is because each insert would
> call a proto update operatino and these must be allowed to be called
> multiple times. The streaming af_unix implementation recently added
> a refcnt to handle a use after free issue, however it introduced a
> memleak when inserted into multiple maps.
>
> This series fixes the memleak, adds a note in the code so we remember
> that proto updates need to support this. And then we add three tests
> for each of the slightly different iterations of adding sockets into
> multiple maps. I kept them as 3 independent test cases here. I have
> some slight preference for this they could however be a single test,
> but then you don't get to run them independently which was sort of
> useful while debugging.
>
> John Fastabend (5):
>   bpf: sockmap, fix proto update hook to avoid dup calls
>   bpf: sockmap, added comments describing update proto rules
>   bpf: sockmap, add tests for proto updates many to single map
>   bpf: sockmap, add tests for proto updates single socket to many map
>   bpf: sockmap, add tests for proto updates replace socket
>
>  include/linux/skmsg.h                         |   5 +
>  net/unix/unix_bpf.c                           |  21 +-
>  .../selftests/bpf/prog_tests/sockmap_basic.c  | 199 +++++++++++++++++-
>  3 files changed, 221 insertions(+), 4 deletions(-)

Sorry for the delay. I was out.

This LGTM with some room for improvement in tests.
You repeat the code to create different kind of sockets in each test.
That could be refactored to use some kind of a factory helper.

Reviewed-by: Jakub Sitnicki <jakub@...udflare.com>