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

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

[John Fastabend]

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(-)

-- 
2.33.0

[PATCH bpf 1/5] bpf: sockmap, fix proto update hook to avoid dup calls

[John Fastabend]

When sockets are added to a sockmap or sockhash we allocate and init a
psock. Then update the proto ops with sock_map_init_proto the flow is

  sock_hash_update_common
    sock_map_link
      psock = sock_map_psock_get_checked() <-returns existing psock
      sock_map_init_proto(sk, psock)       <- updates sk_proto

If the socket is already in a map this results in the sock_map_init_proto
being called multiple times on the same socket. We do this because when
a socket is added to multiple maps this might result in a new set of BPF
programs being attached to the socket requiring an updated ops struct.

This creates a rule where it must be safe to call psock_update_sk_prot
multiple times. When we added a fix for UAF through unix sockets in patch
4dd9a38a753fc we broke this rule by adding a sock_hold in that path
to ensure the sock is not released. The result is if a af_unix stream sock
is placed in multiple maps it results in a memory leak because we call
sock_hold multiple times with only a single sock_put on it.

Fixes: 4dd9a38a753fc ("bpf: sockmap, fix proto update hook to avoid dup calls")
Rebported-by: Xingwei Lee <xrivendell7@gmail.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/unix/unix_bpf.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
index 7ea7c3a0d0d0..bd84785bf8d6 100644
--- a/net/unix/unix_bpf.c
+++ b/net/unix/unix_bpf.c
@@ -161,15 +161,30 @@ int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool r
 {
 	struct sock *sk_pair;
 
+	/* Restore does not decrement the sk_pair reference yet because we must
+	 * keep the a reference to the socket until after an RCU grace period
+	 * and any pending sends have completed.
+	 */
 	if (restore) {
 		sk->sk_write_space = psock->saved_write_space;
 		sock_replace_proto(sk, psock->sk_proto);
 		return 0;
 	}
 
-	sk_pair = unix_peer(sk);
-	sock_hold(sk_pair);
-	psock->sk_pair = sk_pair;
+	/* psock_update_sk_prot can be called multiple times if psock is
+	 * added to multiple maps and/or slots in the same map. There is
+	 * also an edge case where replacing a psock with itself can trigger
+	 * an extra psock_update_sk_prot during the insert process. So it
+	 * must be safe to do multiple calls. Here we need to ensure we don't
+	 * increment the refcnt through sock_hold many times. There will only
+	 * be a single matching destroy operation.
+	 */
+	if (!psock->sk_pair) {
+		sk_pair = unix_peer(sk);
+		sock_hold(sk_pair);
+		psock->sk_pair = sk_pair;
+	}
+
 	unix_stream_bpf_check_needs_rebuild(psock->sk_proto);
 	sock_replace_proto(sk, &unix_stream_bpf_prot);
 	return 0;
-- 
2.33.0

[PATCH bpf 2/5] bpf: sockmap, added comments describing update proto rules

[John Fastabend]

Add a comment describing that the psock update proto callbback can be
called multiple times and this must be safe.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 include/linux/skmsg.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index c953b8c0d2f4..888a4b217829 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -100,6 +100,11 @@ struct sk_psock {
 	void (*saved_close)(struct sock *sk, long timeout);
 	void (*saved_write_space)(struct sock *sk);
 	void (*saved_data_ready)(struct sock *sk);
+	/* psock_update_sk_prot may be called with restore=false many times
+	 * so the handler must be safe for this case. It will be called
+	 * exactly once with restore=true when the psock is being destroyed
+	 * and psock refcnt is zero, but before an RCU grace period.
+	 */
 	int  (*psock_update_sk_prot)(struct sock *sk, struct sk_psock *psock,
 				     bool restore);
 	struct proto			*sk_proto;
-- 
2.33.0

[PATCH bpf 3/5] bpf: sockmap, add tests for proto updates many to single map

[John Fastabend]

Add test with a single map where each socket is inserted multiple
times. Test protocols: TCP, UDP, stream af_unix and dgram af_unix.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 66 ++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 7c2241fae19a..22240eeb798b 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -555,6 +555,69 @@ static void test_sockmap_unconnected_unix(void)
 	close(dgram);
 }
 
+static void test_sockmap_many_socket(void)
+{
+	struct test_sockmap_pass_prog *skel;
+	int stream[2], dgram, udp, tcp;
+	int i, err, map, entry = 0;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	map = bpf_map__fd(skel->maps.sock_map_rx);
+
+	dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
+	if (dgram < 0)
+		return;
+
+	tcp = connected_socket_v4();
+	if (!ASSERT_GE(tcp, 0, "connected_socket_v4")) {
+		close(dgram);
+		return;
+	}
+
+	udp = xsocket(AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, 0);
+	if (udp < 0) {
+		close(dgram);
+		close(tcp);
+		return;
+	}
+
+	err = socketpair(AF_UNIX, SOCK_STREAM, 0, stream);
+	ASSERT_OK(err, "socketpair(af_unix, sock_stream)");
+	if (err)
+		goto out;
+
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map, &entry, &stream[0], BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(stream)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map, &entry, &dgram, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(dgram)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map, &entry, &udp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(udp)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map, &entry, &tcp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(tcp)");
+	}
+	for (entry--; entry >= 0; entry--) {
+		err = bpf_map_delete_elem(map, &entry);
+		ASSERT_OK(err, "bpf_map_delete_elem(entry)");
+	}
+
+	close(stream[0]);
+	close(stream[1]);
+out:
+	close(dgram);
+	close(tcp);
+	close(udp);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -597,7 +660,8 @@ void test_sockmap_basic(void)
 		test_sockmap_skb_verdict_fionread(false);
 	if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
 		test_sockmap_skb_verdict_peek();
-
 	if (test__start_subtest("sockmap unconnected af_unix"))
 		test_sockmap_unconnected_unix();
+	if (test__start_subtest("sockmap one socket to many map entries"))
+		test_sockmap_many_socket();
 }
-- 
2.33.0

[PATCH bpf 4/5] bpf: sockmap, add tests for proto updates single socket to many map

[John Fastabend]

Add test with multiple maps where each socket is inserted in multiple
maps. Test protocols: TCP, UDP, stream af_unix and dgram af_unix.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 22240eeb798b..337c92cfb4aa 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -618,6 +618,73 @@ static void test_sockmap_many_socket(void)
 	close(udp);
 }
 
+static void test_sockmap_many_maps(void)
+{
+	struct test_sockmap_pass_prog *skel;
+	int stream[2], dgram, udp, tcp;
+	int i, err, map[2], entry = 0;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	map[0] = bpf_map__fd(skel->maps.sock_map_rx);
+	map[1] = bpf_map__fd(skel->maps.sock_map_tx);
+
+	dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
+	if (dgram < 0)
+		return;
+
+	tcp = connected_socket_v4();
+	if (!ASSERT_GE(tcp, 0, "connected_socket_v4")) {
+		close(dgram);
+		return;
+	}
+
+	udp = xsocket(AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, 0);
+	if (udp < 0) {
+		close(dgram);
+		close(tcp);
+		return;
+	}
+
+	err = socketpair(AF_UNIX, SOCK_STREAM, 0, stream);
+	ASSERT_OK(err, "socketpair(af_unix, sock_stream)");
+	if (err)
+		goto out;
+
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map[i], &entry, &stream[0], BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(stream)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map[i], &entry, &dgram, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(dgram)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map[i], &entry, &udp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(udp)");
+	}
+	for (i = 0; i < 2; i++, entry++) {
+		err = bpf_map_update_elem(map[i], &entry, &tcp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(tcp)");
+	}
+	for (entry--; entry >= 0; entry--) {
+		err = bpf_map_delete_elem(map[1], &entry);
+		entry--;
+		ASSERT_OK(err, "bpf_map_delete_elem(entry)");
+		err = bpf_map_delete_elem(map[0], &entry);
+		ASSERT_OK(err, "bpf_map_delete_elem(entry)");
+	}
+
+	close(stream[0]);
+	close(stream[1]);
+out:
+	close(dgram);
+	close(tcp);
+	close(udp);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -664,4 +731,6 @@ void test_sockmap_basic(void)
 		test_sockmap_unconnected_unix();
 	if (test__start_subtest("sockmap one socket to many map entries"))
 		test_sockmap_many_socket();
+	if (test__start_subtest("sockmap one socket to many maps"))
+		test_sockmap_many_maps();
 }
-- 
2.33.0

[PATCH bpf 5/5] bpf: sockmap, add tests for proto updates replace socket

[John Fastabend]

Add test that replaces the same socket with itself. This exercises a
corner case where old element and new element have the same posck.
Test protocols: TCP, UDP, stream af_unix and dgram af_unix.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 337c92cfb4aa..b5eb287912d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -685,6 +685,68 @@ static void test_sockmap_many_maps(void)
 	close(udp);
 }
 
+static void test_sockmap_same_sock(void)
+{
+	struct test_sockmap_pass_prog *skel;
+	int stream[2], dgram, udp, tcp;
+	int i, err, map, zero = 0;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	map = bpf_map__fd(skel->maps.sock_map_rx);
+
+	dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
+	if (dgram < 0)
+		return;
+
+	tcp = connected_socket_v4();
+	if (!ASSERT_GE(tcp, 0, "connected_socket_v4")) {
+		close(dgram);
+		return;
+	}
+
+	udp = xsocket(AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, 0);
+	if (udp < 0) {
+		close(dgram);
+		close(tcp);
+		return;
+	}
+
+	err = socketpair(AF_UNIX, SOCK_STREAM, 0, stream);
+	ASSERT_OK(err, "socketpair(af_unix, sock_stream)");
+	if (err)
+		goto out;
+
+	for (i = 0; i < 2; i++) {
+		err = bpf_map_update_elem(map, &zero, &stream[0], BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(stream)");
+	}
+	for (i = 0; i < 2; i++) {
+		err = bpf_map_update_elem(map, &zero, &dgram, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(dgram)");
+	}
+	for (i = 0; i < 2; i++) {
+		err = bpf_map_update_elem(map, &zero, &udp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(udp)");
+	}
+	for (i = 0; i < 2; i++) {
+		err = bpf_map_update_elem(map, &zero, &tcp, BPF_ANY);
+		ASSERT_OK(err, "bpf_map_update_elem(tcp)");
+	}
+
+	err = bpf_map_delete_elem(map, &zero);
+	ASSERT_OK(err, "bpf_map_delete_elem(entry)");
+
+	close(stream[0]);
+	close(stream[1]);
+out:
+	close(dgram);
+	close(tcp);
+	close(udp);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -733,4 +795,6 @@ void test_sockmap_basic(void)
 		test_sockmap_many_socket();
 	if (test__start_subtest("sockmap one socket to many maps"))
 		test_sockmap_many_maps();
+	if (test__start_subtest("sockmap same socket replace"))
+		test_sockmap_same_sock();
 }
-- 
2.33.0

Re: [PATCH bpf 1/5] bpf: sockmap, fix proto update hook to avoid dup calls

[Jakub Sitnicki]

On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote:
> When sockets are added to a sockmap or sockhash we allocate and init a
> psock. Then update the proto ops with sock_map_init_proto the flow is
>
>   sock_hash_update_common
>     sock_map_link
>       psock = sock_map_psock_get_checked() <-returns existing psock
>       sock_map_init_proto(sk, psock)       <- updates sk_proto
>
> If the socket is already in a map this results in the sock_map_init_proto
> being called multiple times on the same socket. We do this because when
> a socket is added to multiple maps this might result in a new set of BPF
> programs being attached to the socket requiring an updated ops struct.
>
> This creates a rule where it must be safe to call psock_update_sk_prot
> multiple times. When we added a fix for UAF through unix sockets in patch
> 4dd9a38a753fc we broke this rule by adding a sock_hold in that path
> to ensure the sock is not released. The result is if a af_unix stream sock
> is placed in multiple maps it results in a memory leak because we call
> sock_hold multiple times with only a single sock_put on it.
>
> Fixes: 4dd9a38a753fc ("bpf: sockmap, fix proto update hook to avoid dup calls")
> Rebported-by: Xingwei Lee <xrivendell7@gmail.com>

Nit: Typo ^

> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---

[...]

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

[Jakub Sitnicki]

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>

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

[John Fastabend]

Jakub Sitnicki wrote:
> 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.

Thanks for the review.

> 
> 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.

Yeah, my first attempt was uglier than the repeated setup in my
opinion. So figured I would get this out and think a bit more
about it. Lets see if BPF maintainers want me to fix the typo
on Reported-by or if it can be fixed on merged.

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

Re: [PATCH bpf 1/5] bpf: sockmap, fix proto update hook to avoid dup calls

[Martin KaFai Lau]

On 1/2/24 4:00 AM, Jakub Sitnicki wrote:
> On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote:
>> When sockets are added to a sockmap or sockhash we allocate and init a
>> psock. Then update the proto ops with sock_map_init_proto the flow is
>>
>>    sock_hash_update_common
>>      sock_map_link
>>        psock = sock_map_psock_get_checked() <-returns existing psock
>>        sock_map_init_proto(sk, psock)       <- updates sk_proto
>>
>> If the socket is already in a map this results in the sock_map_init_proto
>> being called multiple times on the same socket. We do this because when
>> a socket is added to multiple maps this might result in a new set of BPF
>> programs being attached to the socket requiring an updated ops struct.
>>
>> This creates a rule where it must be safe to call psock_update_sk_prot
>> multiple times. When we added a fix for UAF through unix sockets in patch
>> 4dd9a38a753fc we broke this rule by adding a sock_hold in that path
>> to ensure the sock is not released. The result is if a af_unix stream sock
>> is placed in multiple maps it results in a memory leak because we call
>> sock_hold multiple times with only a single sock_put on it.
>>
>> Fixes: 4dd9a38a753fc ("bpf: sockmap, fix proto update hook to avoid dup calls")

The Fixes tag looks wrong ;)

I changed it to

Fixes: 8866730aed51 ("bpf, sockmap: af_unix stream sockets need to hold ref for 
pair sock")

>> Rebported-by: Xingwei Lee <xrivendell7@gmail.com>
> 
> Nit: Typo ^

yep. fixed.

Also added the missing "test_sockmap_pass_prog__destroy(skel)" to the 
sockmap_basic.c selftest.

Applied. Thanks for the fixes and the review.

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Thu, 21 Dec 2023 15:23:22 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf,1/5] bpf: sockmap, fix proto update hook to avoid dup calls
    https://git.kernel.org/bpf/bpf-next/c/16b2f264983d
  - [bpf,2/5] bpf: sockmap, added comments describing update proto rules
    https://git.kernel.org/bpf/bpf-next/c/7865dfb1eb94
  - [bpf,3/5] bpf: sockmap, add tests for proto updates many to single map
    https://git.kernel.org/bpf/bpf-next/c/8c1b382a555a
  - [bpf,4/5] bpf: sockmap, add tests for proto updates single socket to many map
    https://git.kernel.org/bpf/bpf-next/c/f1300467dd9f
  - [bpf,5/5] bpf: sockmap, add tests for proto updates replace socket
    https://git.kernel.org/bpf/bpf-next/c/bdbca46d3f84

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf 1/5] bpf: sockmap, fix proto update hook to avoid dup calls

[John Fastabend]

Martin KaFai Lau wrote:
> On 1/2/24 4:00 AM, Jakub Sitnicki wrote:
> > On Thu, Dec 21, 2023 at 03:23 PM -08, John Fastabend wrote:
> >> When sockets are added to a sockmap or sockhash we allocate and init a
> >> psock. Then update the proto ops with sock_map_init_proto the flow is
> >>
> >>    sock_hash_update_common
> >>      sock_map_link
> >>        psock = sock_map_psock_get_checked() <-returns existing psock
> >>        sock_map_init_proto(sk, psock)       <- updates sk_proto
> >>
> >> If the socket is already in a map this results in the sock_map_init_proto
> >> being called multiple times on the same socket. We do this because when
> >> a socket is added to multiple maps this might result in a new set of BPF
> >> programs being attached to the socket requiring an updated ops struct.
> >>
> >> This creates a rule where it must be safe to call psock_update_sk_prot
> >> multiple times. When we added a fix for UAF through unix sockets in patch
> >> 4dd9a38a753fc we broke this rule by adding a sock_hold in that path
> >> to ensure the sock is not released. The result is if a af_unix stream sock
> >> is placed in multiple maps it results in a memory leak because we call
> >> sock_hold multiple times with only a single sock_put on it.
> >>
> >> Fixes: 4dd9a38a753fc ("bpf: sockmap, fix proto update hook to avoid dup calls")
> 
> The Fixes tag looks wrong ;)
> 
> I changed it to
> 
> Fixes: 8866730aed51 ("bpf, sockmap: af_unix stream sockets need to hold ref for 
> pair sock")
> 
> >> Rebported-by: Xingwei Lee <xrivendell7@gmail.com>
> > 
> > Nit: Typo ^
> 
> yep. fixed.
> 
> Also added the missing "test_sockmap_pass_prog__destroy(skel)" to the 
> sockmap_basic.c selftest.

Thanks! Appreciate it Martin.

> 
> Applied. Thanks for the fixes and the review.
>