[PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[Michal Luczaj]

PATCH 1/4 tells BPF redirect to silently drop AF_UNIX's MSG_OOB. The rest
is selftest-related.

Michal Luczaj (4):
  af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
  selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()
  selftest/bpf: Parametrize AF_UNIX redir functions to accept send()
    flags
  selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB

 net/unix/af_unix.c                            | 41 ++++++++-
 net/unix/unix_bpf.c                           |  3 +
 .../selftests/bpf/prog_tests/sockmap_listen.c | 85 +++++++++++++------
 3 files changed, 102 insertions(+), 27 deletions(-)

-- 
2.45.2

[PATCH bpf v4 1/4] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

[Michal Luczaj]

AF_UNIX socket tracks the most recent OOB packet (in its receive queue)
with an `oob_skb` pointer. BPF redirecting does not account for that: when
an OOB packet is moved between sockets, `oob_skb` is left outdated. This
results in a single skb that may be accessed from two different sockets.

Take the easy way out: silently drop MSG_OOB data targeting any socket that
is in a sockmap or a sockhash. Note that such silent drop is akin to the
fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS).

For symmetry, forbid MSG_OOB in unix_bpf_recvmsg().

Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/unix/af_unix.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
 net/unix/unix_bpf.c |  3 +++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 142f56770b77..11cb5badafb6 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2667,10 +2667,49 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 
 static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
 {
+	struct unix_sock *u = unix_sk(sk);
+	struct sk_buff *skb;
+	int err;
+
 	if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED))
 		return -ENOTCONN;
 
-	return unix_read_skb(sk, recv_actor);
+	mutex_lock(&u->iolock);
+	skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err);
+	mutex_unlock(&u->iolock);
+	if (!skb)
+		return err;
+
+#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
+	if (unlikely(skb == READ_ONCE(u->oob_skb))) {
+		bool drop = false;
+
+		unix_state_lock(sk);
+
+		if (sock_flag(sk, SOCK_DEAD)) {
+			unix_state_unlock(sk);
+			kfree_skb(skb);
+			return -ECONNRESET;
+		}
+
+		spin_lock(&sk->sk_receive_queue.lock);
+		if (likely(skb == u->oob_skb)) {
+			WRITE_ONCE(u->oob_skb, NULL);
+			drop = true;
+		}
+		spin_unlock(&sk->sk_receive_queue.lock);
+
+		unix_state_unlock(sk);
+
+		if (drop) {
+			WARN_ON_ONCE(skb_unref(skb));
+			kfree_skb(skb);
+			return -EAGAIN;
+		}
+	}
+#endif
+
+	return recv_actor(sk, skb);
 }
 
 static int unix_stream_read_generic(struct unix_stream_read_state *state,
diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
index bd84785bf8d6..bca2d86ba97d 100644
--- a/net/unix/unix_bpf.c
+++ b/net/unix/unix_bpf.c
@@ -54,6 +54,9 @@ static int unix_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	struct sk_psock *psock;
 	int copied;
 
+	if (flags & MSG_OOB)
+		return -EOPNOTSUPP;
+
 	if (!len)
 		return 0;
 
-- 
2.45.2

[PATCH bpf v4 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()

[Michal Luczaj]

Function ignores the AF_UNIX socket type argument, SOCK_DGRAM is hardcoded.
Fix to respect the argument provided.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Fixes: 75e0e27db6cf ("selftest/bpf: Change udp to inet in some function names")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index e91b59366030..c075d376fcab 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1828,7 +1828,7 @@ static void unix_inet_redir_to_connected(int family, int type,
 	if (err)
 		return;
 
-	if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, sfd))
+	if (socketpair(AF_UNIX, type | SOCK_NONBLOCK, 0, sfd))
 		goto close_cli0;
 	c1 = sfd[0], p1 = sfd[1];
 
@@ -1840,7 +1840,6 @@ static void unix_inet_redir_to_connected(int family, int type,
 close_cli0:
 	xclose(c0);
 	xclose(p0);
-
 }
 
 static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
-- 
2.45.2

[PATCH bpf v4 3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags

[Michal Luczaj]

Extend pairs_redir_to_connected() and unix_inet_redir_to_connected() with a
send_flags parameter. Replace write() with send() allowing packets to be
sent as MSG_OOB.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c | 48 ++++++++++---------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index c075d376fcab..3514a344bee6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -29,6 +29,8 @@
 
 #include "sockmap_helpers.h"
 
+#define NO_FLAGS 0
+
 static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused,
 				int family, int sotype, int mapfd)
 {
@@ -1376,7 +1378,8 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
 
 static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 				     int sock_mapfd, int nop_mapfd,
-				     int verd_mapfd, enum redir_mode mode)
+				     int verd_mapfd, enum redir_mode mode,
+				     int send_flags)
 {
 	const char *log_prefix = redir_mode_str(mode);
 	unsigned int pass;
@@ -1396,11 +1399,9 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 			return;
 	}
 
-	n = write(cli1, "a", 1);
-	if (n < 0)
-		FAIL_ERRNO("%s: write", log_prefix);
+	n = xsend(cli1, "a", 1, send_flags);
 	if (n == 0)
-		FAIL("%s: incomplete write", log_prefix);
+		FAIL("%s: incomplete send", log_prefix);
 	if (n < 1)
 		return;
 
@@ -1432,7 +1433,8 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd,
 		goto close0;
 	c1 = sfd[0], p1 = sfd[1];
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd,
+				 mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1722,7 +1724,8 @@ static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd,
 	if (err)
 		goto close_cli0;
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd,
+				 mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1780,7 +1783,8 @@ static void inet_unix_redir_to_connected(int family, int type, int sock_mapfd,
 	if (err)
 		goto close;
 
-	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, -1, verd_mapfd,
+				 mode, NO_FLAGS);
 
 	xclose(c1);
 	xclose(p1);
@@ -1815,10 +1819,9 @@ static void inet_unix_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }
 
-static void unix_inet_redir_to_connected(int family, int type,
-					int sock_mapfd, int nop_mapfd,
-					int verd_mapfd,
-					enum redir_mode mode)
+static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
+					 int nop_mapfd, int verd_mapfd,
+					 enum redir_mode mode, int send_flags)
 {
 	int c0, c1, p0, p1;
 	int sfd[2];
@@ -1832,8 +1835,8 @@ static void unix_inet_redir_to_connected(int family, int type,
 		goto close_cli0;
 	c1 = sfd[0], p1 = sfd[1];
 
-	pairs_redir_to_connected(c0, p0, c1, p1,
-				 sock_mapfd, nop_mapfd, verd_mapfd, mode);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
+				 verd_mapfd, mode, send_flags);
 
 	xclose(c1);
 	xclose(p1);
@@ -1858,31 +1861,32 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	skel->bss->test_ingress = false;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_EGRESS);
+				     REDIR_EGRESS, NO_FLAGS);
+
 	skel->bss->test_ingress = true;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, -1, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
-				     REDIR_INGRESS);
+				     REDIR_INGRESS, NO_FLAGS);
 
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }
-- 
2.45.2

[PATCH bpf v4 4/4] selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB

[Michal Luczaj]

Verify that out-of-band packets are silently dropped before they reach the
redirection logic.

The idea is to test with a 2 byte long send(). Should a MSG_OOB flag be in
use, only the last byte will be treated as out-of-band. Test fails if
verd_mapfd indicates a wrong number of packets processed (e.g. if OOB
wasn't dropped at the source) or if it was possible to recv() MSG_OOB from
the mapped socket, or if any stale OOB data have been left reachable from
the unmapped socket.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c | 36 +++++++++++++++++--
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 3514a344bee6..9ce0e0e0b7da 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1399,10 +1399,11 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 			return;
 	}
 
-	n = xsend(cli1, "a", 1, send_flags);
-	if (n == 0)
+	/* Last byte is OOB data when send_flags has MSG_OOB bit set */
+	n = xsend(cli1, "ab", 2, send_flags);
+	if (n >= 0 && n < 2)
 		FAIL("%s: incomplete send", log_prefix);
-	if (n < 1)
+	if (n < 2)
 		return;
 
 	key = SK_PASS;
@@ -1417,6 +1418,25 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL_ERRNO("%s: recv_timeout", log_prefix);
 	if (n == 0)
 		FAIL("%s: incomplete recv", log_prefix);
+
+	if (send_flags & MSG_OOB) {
+		/* Check that we can't read OOB while in sockmap */
+		errno = 0;
+		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
+		if (n != -1 || errno != EOPNOTSUPP)
+			FAIL("%s: recv(MSG_OOB): expected EOPNOTSUPP: retval=%d errno=%d",
+			     log_prefix, n, errno);
+
+		/* Remove peer1 from sockmap */
+		xbpf_map_delete_elem(sock_mapfd, &(int){ 1 });
+
+		/* Check that OOB was dropped on redirect */
+		errno = 0;
+		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
+		if (n != -1 || errno != EINVAL)
+			FAIL("%s: recv(MSG_OOB): expected EINVAL: retval=%d errno=%d",
+			     log_prefix, n, errno);
+	}
 }
 
 static void unix_redir_to_connected(int sotype, int sock_mapfd,
@@ -1873,6 +1893,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_EGRESS, NO_FLAGS);
 
+	/* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+	unix_inet_redir_to_connected(family, SOCK_STREAM,
+				     sock_map, nop_map, verdict_map,
+				     REDIR_EGRESS, MSG_OOB);
+
 	skel->bss->test_ingress = true;
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
@@ -1888,6 +1913,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_INGRESS, NO_FLAGS);
 
+	/* MSG_OOB not supported by AF_UNIX SOCK_DGRAM */
+	unix_inet_redir_to_connected(family, SOCK_STREAM,
+				     sock_map, nop_map, verdict_map,
+				     REDIR_INGRESS, MSG_OOB);
+
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_SKB_VERDICT);
 }
 
-- 
2.45.2

Re: [PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[Michal Luczaj]

On 7/13/24 21:41, Michal Luczaj wrote:
> PATCH 1/4 tells BPF redirect to silently drop AF_UNIX's MSG_OOB. The rest
> is selftest-related.
> 
> Michal Luczaj (4):
>   af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
>   selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()
>   selftest/bpf: Parametrize AF_UNIX redir functions to accept send()
>     flags
>   selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB
> 
>  net/unix/af_unix.c                            | 41 ++++++++-
>  net/unix/unix_bpf.c                           |  3 +
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 85 +++++++++++++------
>  3 files changed, 102 insertions(+), 27 deletions(-)

Arrgh, forgot the changelog:

v4:
  - Fix typo; comment, extend and streamline the selftest (Jakub)
  - Fix commit message in PATCH 2/4
  - Collect Reviewed-bys

v3: https://lore.kernel.org/netdev/20240707222842.4119416-1-mhal@rbox.co/
  - Add selftest

v2: https://lore.kernel.org/netdev/20240622223324.3337956-1-mhal@rbox.co/
  - Reduce time under mutex, restructure (Kuniyuki)
  - Handle unix_release_sock() race

v1: https://lore.kernel.org/netdev/20240620203009.2610301-1-mhal@rbox.co/

Re: [PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[Jakub Sitnicki]

On Sat, Jul 13, 2024 at 10:14 PM +02, Michal Luczaj wrote:
> Arrgh, forgot the changelog:

Take a look at b4 for managing patch set the cover letter and changelog:

https://b4.docs.kernel.org/en/latest/

Re: [PATCH bpf v4 3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags

[Jakub Sitnicki]

On Sat, Jul 13, 2024 at 09:41 PM +02, Michal Luczaj wrote:
> Extend pairs_redir_to_connected() and unix_inet_redir_to_connected() with a
> send_flags parameter. Replace write() with send() allowing packets to be
> sent as MSG_OOB.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

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

Re: [PATCH bpf v4 4/4] selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB

[Jakub Sitnicki]

On Sat, Jul 13, 2024 at 09:41 PM +02, Michal Luczaj wrote:
> Verify that out-of-band packets are silently dropped before they reach the
> redirection logic.
>
> The idea is to test with a 2 byte long send(). Should a MSG_OOB flag be in
> use, only the last byte will be treated as out-of-band. Test fails if
> verd_mapfd indicates a wrong number of packets processed (e.g. if OOB
> wasn't dropped at the source) or if it was possible to recv() MSG_OOB from
> the mapped socket, or if any stale OOB data have been left reachable from
> the unmapped socket.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

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

Re: [PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[Jakub Sitnicki]

On Sat, Jul 13, 2024 at 09:41 PM +02, Michal Luczaj wrote:
> PATCH 1/4 tells BPF redirect to silently drop AF_UNIX's MSG_OOB. The rest
> is selftest-related.
>
> Michal Luczaj (4):
>   af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
>   selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()
>   selftest/bpf: Parametrize AF_UNIX redir functions to accept send()
>     flags
>   selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB
>
>  net/unix/af_unix.c                            | 41 ++++++++-
>  net/unix/unix_bpf.c                           |  3 +
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 85 +++++++++++++------
>  3 files changed, 102 insertions(+), 27 deletions(-)

Thanks for taking time to extend the tests.

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

Re: [PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[Michal Luczaj]

On 7/16/24 10:54, Jakub Sitnicki wrote:
> On Sat, Jul 13, 2024 at 10:14 PM +02, Michal Luczaj wrote:
>> Arrgh, forgot the changelog:
> 
> Take a look at b4 for managing patch set the cover letter and changelog:
> 
> https://b4.docs.kernel.org/en/latest/

Thanks, will do.

Re: [PATCH bpf v4 0/4] af_unix: MSG_OOB handling fix & selftest

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Sat, 13 Jul 2024 21:41:37 +0200 you wrote:
> PATCH 1/4 tells BPF redirect to silently drop AF_UNIX's MSG_OOB. The rest
> is selftest-related.
> 
> Michal Luczaj (4):
>   af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
>   selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()
>   selftest/bpf: Parametrize AF_UNIX redir functions to accept send()
>     flags
>   selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB
> 
> [...]

Here is the summary with links:
  - [bpf,v4,1/4] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
    https://git.kernel.org/bpf/bpf/c/638f32604385
  - [bpf,v4,2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()
    https://git.kernel.org/bpf/bpf/c/1b0ad43177c0
  - [bpf,v4,3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags
    https://git.kernel.org/bpf/bpf/c/0befb349c4cd
  - [bpf,v4,4/4] selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB
    https://git.kernel.org/bpf/bpf/c/6caf9efaa169

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