Re: [PATCH bpf 0/5] fix sockmap + stream af_unix memleak

[Jakub Sitnicki <jakub@cloudflare.com>]

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@cloudflare.com>