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

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

[Michal Luczaj]

PATCH 1/4 tells BPF redirect how to deal with AF_UNIX's MSG_OOB
(silent drop). The rest is selftest-related.

v3:
  - 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/

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 | 65 ++++++++++++++-----
 3 files changed, 92 insertions(+), 17 deletions(-)

-- 
2.45.2

[PATCH bpf v3 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>
---
 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 v3 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()

[Michal Luczaj]

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

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 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 v3 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 | 40 +++++++++++++------
 1 file changed, 27 insertions(+), 13 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..59e16f8f2090 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1374,9 +1374,10 @@ 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)
+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 send_flags)
 {
 	const char *log_prefix = redir_mode_str(mode);
 	unsigned int pass;
@@ -1396,11 +1397,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;
 
@@ -1418,6 +1417,14 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL("%s: incomplete recv", log_prefix);
 }
 
+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)
+{
+	__pairs_redir_to_connected(cli0, peer0, cli1, peer1, sock_mapfd,
+				   nop_mapfd, verd_mapfd, mode, 0);
+}
+
 static void unix_redir_to_connected(int sotype, int sock_mapfd,
 			       int verd_mapfd, enum redir_mode mode)
 {
@@ -1815,10 +1822,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 +1838,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);
@@ -1842,6 +1848,14 @@ static void unix_inet_redir_to_connected(int family, int type,
 	xclose(p0);
 }
 
+static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
+					 int nop_mapfd, int verd_mapfd,
+					 enum redir_mode mode)
+{
+	__unix_inet_redir_to_connected(family, type, sock_mapfd, nop_mapfd,
+				       verd_mapfd, mode, 0);
+}
+
 static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 					    struct bpf_map *inner_map, int family)
 {
-- 
2.45.2

[PATCH bpf v3 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. Attempt to recv() stale data that might have been
erroneously left reachable from the original socket.

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 data
wasn't dropped at the source) or if it was still somehow possble to recv()
OOB from the mapped socket.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c | 26 ++++++++++++++++---
 1 file changed, 23 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 59e16f8f2090..878fcca36a55 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1397,10 +1397,10 @@ 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)
+	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;
@@ -1415,6 +1415,19 @@ 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) {
+		key = 0;
+		xbpf_map_delete_elem(sock_mapfd, &key);
+		key = 1;
+		xbpf_map_delete_elem(sock_mapfd, &key);
+
+		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
+		if (n > 0)
+			FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
+		if (n == 0)
+			FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
+	}
 }
 
 static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
@@ -1883,6 +1896,10 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_EGRESS);
+	__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,
@@ -1897,6 +1914,9 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_INGRESS);
+	__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 v3 1/4] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

[Kuniyuki Iwashima]

From: Michal Luczaj <mhal@rbox.co>
Date: Sun,  7 Jul 2024 23:28:22 +0200
> 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>

Thanks!


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

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

[John Fastabend]

Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@rbox.co>
> Date: Sun,  7 Jul 2024 23:28:22 +0200
> > 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>
> 

Why does af_unix put the oob data on the sk_receive_queue()? Wouldn't it
be enough to just have the ousk->oob_skb hold the reference to the skb?

I think for TCP/UDP at least I'll want to handle MSG_OOB data correctly.
For redirect its probably fine to just drop or skip it, but when we are
just reading recv msgs and parsing/observing it would be nice to not change
how the application works. In practice I don't recall anyone reporting
issues on TCP side though from incorrectly handling URG data.

From TCP side I believe we can fix the OOB case by checking the oob queue
before doing the recvmsg handling. If the urg data wasn't on the general
sk_receive_queue we could do similar here for af_unix? My argument for
URG not working for redirect would be to let userspace handle it if they
cared.

Thanks.

> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> 
> Thanks!
> 
> 
> > ---
> >  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

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

[Kuniyuki Iwashima]

From: John Fastabend <john.fastabend@gmail.com>
Date: Mon, 08 Jul 2024 18:24:02 -0700
> Kuniyuki Iwashima wrote:
> > From: Michal Luczaj <mhal@rbox.co>
> > Date: Sun,  7 Jul 2024 23:28:22 +0200
> > > 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>
> > 
> 
> Why does af_unix put the oob data on the sk_receive_queue()? Wouldn't it
> be enough to just have the ousk->oob_skb hold the reference to the skb?

We need to remember when oob_skb was sent for some reasons:

  1. OOB data must stop recv() at the point
  2. ioctl(SIOCATMARK) must return true if OOB data is at the head of
     the recvq regardless that the data is already consumed or not

  See tools/testing/selftests/net/af_unix/msg_oob.c

And actually TCP has OOB data in the normal skb ququed in recvq.

TCP also holds the copied data in tp->urg_data and SEQ# in tp->urg_seq.
Later when recv() passes through the OOB SEQ#, the recv() is stopped at
OOB data.

Note that the implementation has some bugs, e.g. it doesn't have a flag
to indicate if each OOB data is consumed, and we can recv() the stale
OOB data twice.

(CCed Takamitsu who is working on the URG bugs on the TCP side)


> I think for TCP/UDP at least I'll want to handle MSG_OOB data correctly.

UDP doesn't support MSG_OOB.


> For redirect its probably fine to just drop or skip it, but when we are
> just reading recv msgs and parsing/observing it would be nice to not change
> how the application works. In practice I don't recall anyone reporting
> issues on TCP side though from incorrectly handling URG data.

IIUC, the normal read case is processed by tcp_recvmsg(), right ?
Then, OOB data is handled at the found_ok_skb/skip_copy: labels.


> 
> From TCP side I believe we can fix the OOB case by checking the oob queue
> before doing the recvmsg handling. If the urg data wasn't on the general
> sk_receive_queue we could do similar here for af_unix? My argument for
> URG not working for redirect would be to let userspace handle it if they
> cared.

For the redirect cse, in tcp_read_skb(), we need to check tp->urg_data and
tp->{copied,urg}_seq and clear them if needed as done in tcp_recvmsg().

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

[Jakub Sitnicki]

On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> 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: Jakub Sitnicki <jakub@cloudflare.com>

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

[Jakub Sitnicki]

On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> Function ignores the AF_INET socket type argument, SOCK_DGRAM is hardcoded.
> Fix to respect the argument provided.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

Thanks for the fixup.

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

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

[Jakub Sitnicki]

On Sun, Jul 07, 2024 at 11:28 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>
> ---
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 40 +++++++++++++------
>  1 file changed, 27 insertions(+), 13 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..59e16f8f2090 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1374,9 +1374,10 @@ 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)
> +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 send_flags)
>  {
>  	const char *log_prefix = redir_mode_str(mode);
>  	unsigned int pass;
> @@ -1396,11 +1397,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;
>  
> @@ -1418,6 +1417,14 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
>  		FAIL("%s: incomplete recv", log_prefix);
>  }
>  
> +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)
> +{
> +	__pairs_redir_to_connected(cli0, peer0, cli1, peer1, sock_mapfd,
> +				   nop_mapfd, verd_mapfd, mode, 0);
> +}
> +
>  static void unix_redir_to_connected(int sotype, int sock_mapfd,
>  			       int verd_mapfd, enum redir_mode mode)
>  {
> @@ -1815,10 +1822,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 +1838,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);
> @@ -1842,6 +1848,14 @@ static void unix_inet_redir_to_connected(int family, int type,
>  	xclose(p0);
>  }
>  
> +static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
> +					 int nop_mapfd, int verd_mapfd,
> +					 enum redir_mode mode)
> +{
> +	__unix_inet_redir_to_connected(family, type, sock_mapfd, nop_mapfd,
> +				       verd_mapfd, mode, 0);
> +}
> +
>  static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  					    struct bpf_map *inner_map, int family)
>  {

I've got some cosmetic suggestions.

Instead of having two helper variants - with and without send_flags - we
could stick to just one and always pass send_flags. For readability I'd
use a constant for "no flags".

This way we keep the path open to convert
unix_inet_skb_redir_to_connected() to to a loop over a parameter
combination matrix, instead of open-coding multiple calls to
unix_inet_redir_to_connected() for each argument combination.

It seems doing it the current way, it is way too easy to miss
typos. Pretty sure we have another typo at [1], looks like should be
s/SOCK_DGRAM/SOCK_STREAM/.

[1]
https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c#L1863

See below for what I have in mind.

--8<--
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 59e16f8f2090..3ddffaead2cd 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)
 {
@@ -1374,10 +1376,10 @@ 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 send_flags)
+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 send_flags)
 {
 	const char *log_prefix = redir_mode_str(mode);
 	unsigned int pass;
@@ -1417,14 +1419,6 @@ static void __pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL("%s: incomplete recv", log_prefix);
 }
 
-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)
-{
-	__pairs_redir_to_connected(cli0, peer0, cli1, peer1, sock_mapfd,
-				   nop_mapfd, verd_mapfd, mode, 0);
-}
-
 static void unix_redir_to_connected(int sotype, int sock_mapfd,
 			       int verd_mapfd, enum redir_mode mode)
 {
@@ -1439,7 +1433,7 @@ 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);
@@ -1729,7 +1723,7 @@ 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);
@@ -1787,7 +1781,7 @@ 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);
@@ -1822,9 +1816,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, int send_flags)
+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];
@@ -1838,8 +1832,8 @@ static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
 		goto close_cli0;
 	c1 = sfd[0], p1 = sfd[1];
 
-	__pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
-				   verd_mapfd, mode, send_flags);
+	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, nop_mapfd,
+				 verd_mapfd, mode, send_flags);
 
 	xclose(c1);
 	xclose(p1);
@@ -1848,14 +1842,6 @@ static void __unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
 	xclose(p0);
 }
 
-static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
-					 int nop_mapfd, int verd_mapfd,
-					 enum redir_mode mode)
-{
-	__unix_inet_redir_to_connected(family, type, sock_mapfd, nop_mapfd,
-				       verd_mapfd, mode, 0);
-}
-
 static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 					    struct bpf_map *inner_map, int family)
 {
@@ -1872,31 +1858,31 @@ 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);
 }

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

[Jakub Sitnicki]

On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
> Verify that out-of-band packets are silently dropped before they reach the
> redirection logic. Attempt to recv() stale data that might have been
> erroneously left reachable from the original socket.
>
> 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 data
> wasn't dropped at the source) or if it was still somehow possble to recv()

Nit: typo s/possble/possible

Something like below will catch these for you:

$ cat ~/src/linux/.git/hooks/post-commit
exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell

> OOB from the mapped socket.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 26 ++++++++++++++++---
>  1 file changed, 23 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 59e16f8f2090..878fcca36a55 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1397,10 +1397,10 @@ 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)
> +	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;
> @@ -1415,6 +1415,19 @@ 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) {
> +		key = 0;
> +		xbpf_map_delete_elem(sock_mapfd, &key);
> +		key = 1;
> +		xbpf_map_delete_elem(sock_mapfd, &key);
> +
> +		n = recv(peer1, &b, 1, MSG_OOB | MSG_DONTWAIT);
> +		if (n > 0)
> +			FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
> +		if (n == 0)
> +			FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
> +	}
>  }
>  
>  static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> @@ -1883,6 +1896,10 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  	unix_inet_redir_to_connected(family, SOCK_STREAM,
>  				     sock_map, nop_map, verdict_map,
>  				     REDIR_EGRESS);
> +	__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,
> @@ -1897,6 +1914,9 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
>  	unix_inet_redir_to_connected(family, SOCK_STREAM,
>  				     sock_map, nop_map, verdict_map,
>  				     REDIR_INGRESS);
> +	__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);
>  }

This AF_UNIX MSG_OOB use case is super exotic, IMO. TBH, I've just
learned about it. Hence, I think we could use some more comments for the
future readers.

Also, it seems like we only need to remove peer1 from sockmap to test
the behavior. If so, I'd stick to just what is needed to set up the
test. Extra stuff makes you wonder what was the authors intention.

I'd also be more direct about checking return value & error. These
selftests often serve as the only example / API documentation out there.

--8<--
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 25938e66a3c1..1e30e6861805 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1399,6 +1399,7 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 			return;
 	}
 
+	/* 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);
@@ -1419,16 +1420,22 @@ static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
 		FAIL("%s: incomplete recv", log_prefix);
 
 	if (send_flags & MSG_OOB) {
-		key = 0;
-		xbpf_map_delete_elem(sock_mapfd, &key);
-		key = 1;
-		xbpf_map_delete_elem(sock_mapfd, &key);
+		/* 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 > 0)
-			FAIL("%s: recv(MSG_OOB) succeeded", log_prefix);
-		if (n == 0)
-			FAIL("%s: recv(MSG_OOB) returned 0", log_prefix);
+		if (n != -1 || errno != EINVAL)
+			FAIL("%s: recv(MSG_OOB): expected EINVAL: retval=%d errno=%d",
+			     log_prefix, n, errno);
 	}
 }
 
@@ -1882,9 +1889,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_EGRESS, NO_FLAGS);
-	__unix_inet_redir_to_connected(family, SOCK_STREAM,
-				       sock_map, nop_map, verdict_map,
-				       REDIR_EGRESS, MSG_OOB);
+
+	/* 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,
@@ -1900,9 +1909,11 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, nop_map, verdict_map,
 				     REDIR_INGRESS, NO_FLAGS);
-	__unix_inet_redir_to_connected(family, SOCK_STREAM,
-				       sock_map, nop_map, verdict_map,
-				       REDIR_INGRESS, MSG_OOB);
+
+	/* 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);
 }

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

[Michal Luczaj]

On 7/9/24 11:48, Jakub Sitnicki wrote:
> On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
>> Function ignores the AF_INET socket type argument, SOCK_DGRAM is hardcoded.
>> Fix to respect the argument provided.
>>
>> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
> 
> Thanks for the fixup.
> 
> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>

Ugh, my commit message is wrong. Change is for socketpair(AF_UNIX), not
inet_socketpair(). Sorry, will fix.

Speaking of fixups, do you want it tagged with Fixes: 75e0e27db6cf
("selftest/bpf: Change udp to inet in some function names")? And looking at
that commit[1], inet_unix_redir_to_connected() has its @type ignored, too.
Same treatment?

[1] https://lore.kernel.org/netdev/20210816190327.2739291-5-jiang.wang@bytedance.com/

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

[Michal Luczaj]

On 7/9/24 11:59, Jakub Sitnicki wrote:
> I've got some cosmetic suggestions.
> 
> Instead of having two helper variants - with and without send_flags - we
> could stick to just one and always pass send_flags. For readability I'd
> use a constant for "no flags".
> 
> This way we keep the path open to convert
> unix_inet_skb_redir_to_connected() to to a loop over a parameter
> combination matrix, instead of open-coding multiple calls to
> unix_inet_redir_to_connected() for each argument combination.

All right, I think I was aiming for a (short term) churn reduction.

> It seems doing it the current way, it is way too easy to miss
> typos. Pretty sure we have another typo at [1], looks like should be
> s/SOCK_DGRAM/SOCK_STREAM/.
> 
> [1]
> https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c#L1863

Yeah, looks like. I'll add a fix to this series.

> See below for what I have in mind.

Thanks, got it.

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

[Michal Luczaj]

On 7/9/24 12:08, Jakub Sitnicki wrote:
> On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
>> Verify that out-of-band packets are silently dropped before they reach the
>> redirection logic. Attempt to recv() stale data that might have been
>> erroneously left reachable from the original socket.
>>
>> 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 data
>> wasn't dropped at the source) or if it was still somehow possble to recv()
> 
> Nit: typo s/possble/possible
> 
> Something like below will catch these for you:
> 
> $ cat ~/src/linux/.git/hooks/post-commit
> exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell

Heh, I have it. Just not in the right repo :) Will fix.

> This AF_UNIX MSG_OOB use case is super exotic, IMO. TBH, I've just
> learned about it. Hence, I think we could use some more comments for the
> future readers.
> 
> Also, it seems like we only need to remove peer1 from sockmap to test
> the behavior. If so, I'd stick to just what is needed to set up the
> test. Extra stuff makes you wonder what was the authors intention.
> 
> I'd also be more direct about checking return value & error. These
> selftests often serve as the only example / API documentation out there.

Yeah, all fair points. Thanks.

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

[Jakub Sitnicki]

On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
> On 7/9/24 11:48, Jakub Sitnicki wrote:
>> On Sun, Jul 07, 2024 at 11:28 PM +02, Michal Luczaj wrote:
>>> Function ignores the AF_INET socket type argument, SOCK_DGRAM is hardcoded.
>>> Fix to respect the argument provided.
>>>
>>> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>> 
>> Thanks for the fixup.
>> 
>> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
>
> Ugh, my commit message is wrong. Change is for socketpair(AF_UNIX), not
> inet_socketpair(). Sorry, will fix.

Right. Didn't catch that. You can keep my Reviewed-by nevertheless.

> Speaking of fixups, do you want it tagged with Fixes: 75e0e27db6cf
> ("selftest/bpf: Change udp to inet in some function names")?

Yes, we can add Fixes if we want the change to be backported to stable
kernels, or just for information.

> And looking at that commit[1], inet_unix_redir_to_connected() has its
> @type ignored, too.  Same treatment?

That one will not be a trivial fix like this case. inet_socketpair()
won't work for TCP as is. It will fail trying to connect() a listening
socket (p0). I recall now that we are in this state due to some
abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
inet in some function names").

If we bundle stuff together then it takes more energy to push it through
(iterations, reviews). I find it easier to keep the scope down to
minimum for a series.

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

[Michal Luczaj]

On 7/13/24 11:45, Jakub Sitnicki wrote:
> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>> @type ignored, too.  Same treatment?
> 
> That one will not be a trivial fix like this case. inet_socketpair()
> won't work for TCP as is. It will fail trying to connect() a listening
> socket (p0). I recall now that we are in this state due to some
> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
> inet in some function names").

I've assumed @type applies to AF_UNIX. So I've meant to keep
inet_socketpair() with SOCK_DGRAM hardcoded (like it is in
unix_inet_redir_to_connected()), but let the socketpair(AF_UNIX, ...)
accept @type (like this patch does).

> If we bundle stuff together then it takes more energy to push it through
> (iterations, reviews). I find it easier to keep the scope down to
> minimum for a series.

Sure, here's a respin keeping number of patches to a minimum:
https://lore.kernel.org/netdev/20240713200218.2140950-1-mhal@rbox.co/

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

[Jakub Sitnicki]

On Sat, Jul 13, 2024 at 10:16 PM +02, Michal Luczaj wrote:
> On 7/13/24 11:45, Jakub Sitnicki wrote:
>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>> @type ignored, too.  Same treatment?
>> 
>> That one will not be a trivial fix like this case. inet_socketpair()
>> won't work for TCP as is. It will fail trying to connect() a listening
>> socket (p0). I recall now that we are in this state due to some
>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>> inet in some function names").
>
> I've assumed @type applies to AF_UNIX. So I've meant to keep
> inet_socketpair() with SOCK_DGRAM hardcoded (like it is in
> unix_inet_redir_to_connected()), but let the socketpair(AF_UNIX, ...)
> accept @type (like this patch does).

Ah, that is what you had in mind.
Sure, a partial fix gets us closer to a fully working test.

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

[Michal Luczaj]

On 7/16/24 11:14, Jakub Sitnicki wrote:
> On Sat, Jul 13, 2024 at 10:16 PM +02, Michal Luczaj wrote:
>> On 7/13/24 11:45, Jakub Sitnicki wrote:
>>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>>> @type ignored, too.  Same treatment?
>>>
>>> That one will not be a trivial fix like this case. inet_socketpair()
>>> won't work for TCP as is. It will fail trying to connect() a listening
>>> socket (p0). I recall now that we are in this state due to some
>>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>>> inet in some function names").
>>
>> I've assumed @type applies to AF_UNIX. So I've meant to keep
>> inet_socketpair() with SOCK_DGRAM hardcoded (like it is in
>> unix_inet_redir_to_connected()), but let the socketpair(AF_UNIX, ...)
>> accept @type (like this patch does).
> 
> Ah, that is what you had in mind.
> Sure, a partial fix gets us closer to a fully working test.

Well, I'm all for a fully working test. And I'd be happy to help out.

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

[Michal Luczaj]

On 7/13/24 11:45, Jakub Sitnicki wrote:
> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>> @type ignored, too.  Same treatment?
> 
> That one will not be a trivial fix like this case. inet_socketpair()
> won't work for TCP as is. It will fail trying to connect() a listening
> socket (p0). I recall now that we are in this state due to some
> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
> inet in some function names").
> [...]

Is this what you've meant? With this patch inet_socketpair() and
vsock_socketpair_connectible can be reduced to a single call to
create_pair(). And pairs creation in inet_unix_redir_to_connected()
and unix_inet_redir_to_connected() accepts both sotypes.

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 1337153eb0ad..5b17d69c9ee6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -451,11 +451,11 @@ static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
 #define MAX_EVENTS 10
 static void test_sockmap_skb_verdict_shutdown(void)
 {
+	int n, err, map, verdict, c1 = -1, p1 = -1;
 	struct epoll_event ev, events[MAX_EVENTS];
-	int n, err, map, verdict, s, c1 = -1, p1 = -1;
 	struct test_sockmap_pass_prog *skel;
-	int epollfd;
 	int zero = 0;
+	int epollfd;
 	char b;
 
 	skel = test_sockmap_pass_prog__open_and_load();
@@ -469,10 +469,7 @@ static void test_sockmap_skb_verdict_shutdown(void)
 	if (!ASSERT_OK(err, "bpf_prog_attach"))
 		goto out;
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (s < 0)
-		goto out;
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
 	if (err < 0)
 		goto out;
 
@@ -570,16 +567,12 @@ static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 
 static void test_sockmap_skb_verdict_peek_helper(int map)
 {
-	int err, s, c1, p1, zero = 0, sent, recvd, avail;
+	int err, c1, p1, zero = 0, sent, recvd, avail;
 	char snd[256] = "0123456789";
 	char rcv[256] = "0";
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (!ASSERT_GT(s, -1, "socket_loopback(s)"))
-		return;
-
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
-	if (!ASSERT_OK(err, "create_pairs(s)"))
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
+	if (!ASSERT_OK(err, "create_pair()"))
 		return;
 
 	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index e880f97bc44d..4fa8dc97ac88 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -312,54 +312,6 @@ static inline int add_to_sockmap(int sock_mapfd, int fd1, int fd2)
 	return xbpf_map_update_elem(sock_mapfd, &key, &value, BPF_NOEXIST);
 }
 
-static inline int create_pair(int s, int family, int sotype, int *c, int *p)
-{
-	struct sockaddr_storage addr;
-	socklen_t len;
-	int err = 0;
-
-	len = sizeof(addr);
-	err = xgetsockname(s, sockaddr(&addr), &len);
-	if (err)
-		return err;
-
-	*c = xsocket(family, sotype, 0);
-	if (*c < 0)
-		return errno;
-	err = xconnect(*c, sockaddr(&addr), len);
-	if (err) {
-		err = errno;
-		goto close_cli0;
-	}
-
-	*p = xaccept_nonblock(s, NULL, NULL);
-	if (*p < 0) {
-		err = errno;
-		goto close_cli0;
-	}
-	return err;
-close_cli0:
-	close(*c);
-	return err;
-}
-
-static inline int create_socket_pairs(int s, int family, int sotype,
-				      int *c0, int *c1, int *p0, int *p1)
-{
-	int err;
-
-	err = create_pair(s, family, sotype, c0, p0);
-	if (err)
-		return err;
-
-	err = create_pair(s, family, sotype, c1, p1);
-	if (err) {
-		close(*c0);
-		close(*p0);
-	}
-	return err;
-}
-
 static inline int enable_reuseport(int s, int progfd)
 {
 	int err, one = 1;
@@ -412,5 +364,83 @@ static inline int socket_loopback(int family, int sotype)
 	return socket_loopback_reuseport(family, sotype, -1);
 }
 
+static inline int create_pair(int family, int sotype, int *c, int *p)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int s, err;
+
+	s = socket_loopback(family, sotype);
+	if (s < 0)
+		return s;
+
+	err = xgetsockname(s, sockaddr(&addr), &len);
+	if (err)
+		goto close_s;
+
+	*c = xsocket(family, sotype, 0);
+	if (*c < 0) {
+		err = *c;
+		goto close_s;
+	}
+
+	err = connect(*c, sockaddr(&addr), len);
+	if (err) {
+		if (errno != EINPROGRESS) {
+			FAIL_ERRNO("connect");
+			goto close_c;
+		}
+
+		err = poll_connect(*c, IO_TIMEOUT_SEC);
+		if (err) {
+			FAIL_ERRNO("poll_connect");
+			goto close_c;
+		}
+	}
+
+	if (sotype & SOCK_DGRAM) {
+		err = xgetsockname(*c, sockaddr(&addr), &len);
+		if (err)
+			goto close_c;
+
+		err = xconnect(s, sockaddr(&addr), len);
+		if (!err) {
+			*p = s;
+			return err;
+		}
+	} else if (sotype & SOCK_STREAM) {
+		*p = xaccept_nonblock(s, NULL, NULL);
+		if (*p >= 0)
+			goto close_s;
+
+		err = *p;
+	} else {
+		FAIL("Unsupported socket type %#x", sotype);
+		err = -1;
+	}
+
+close_c:
+	close(*c);
+close_s:
+	close(s);
+	return err;
+}
+
+static inline int create_socket_pairs(int s, int family, int sotype,
+				      int *c0, int *c1, int *p0, int *p1)
+{
+	int err;
+
+	err = create_pair(family, sotype, c0, p0);
+	if (err)
+		return err;
+
+	err = create_pair(family, sotype, c1, p1);
+	if (err) {
+		close(*c0);
+		close(*p0);
+	}
+	return err;
+}
 
 #endif // __SOCKMAP_HELPERS__
-- 
2.45.2

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

[Jakub Sitnicki]

On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
> On 7/13/24 11:45, Jakub Sitnicki wrote:
>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>> @type ignored, too.  Same treatment?
>> 
>> That one will not be a trivial fix like this case. inet_socketpair()
>> won't work for TCP as is. It will fail trying to connect() a listening
>> socket (p0). I recall now that we are in this state due to some
>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>> inet in some function names").
>> [...]
>
> Is this what you've meant? With this patch inet_socketpair() and
> vsock_socketpair_connectible can be reduced to a single call to
> create_pair(). And pairs creation in inet_unix_redir_to_connected()
> and unix_inet_redir_to_connected() accepts both sotypes.

Yes, exactly. This looks great.

Classic cleanup with goto to close sockets is all right, but if you're
feeling brave and aim for something less branchy, I've noticed we have
finally started using __attribute__((cleanup)):

https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115

[...]

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

[Michal Luczaj]

On 7/19/24 13:09, Jakub Sitnicki wrote:
> On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
>> On 7/13/24 11:45, Jakub Sitnicki wrote:
>>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>>> @type ignored, too.  Same treatment?
>>>
>>> That one will not be a trivial fix like this case. inet_socketpair()
>>> won't work for TCP as is. It will fail trying to connect() a listening
>>> socket (p0). I recall now that we are in this state due to some
>>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>>> inet in some function names").
>>> [...]
>>
>> Is this what you've meant? With this patch inet_socketpair() and
>> vsock_socketpair_connectible can be reduced to a single call to
>> create_pair(). And pairs creation in inet_unix_redir_to_connected()
>> and unix_inet_redir_to_connected() accepts both sotypes.
> 
> Yes, exactly. This looks great.

Happy to hear that. I'll prepare a series, include the little fixes and
send it out for a proper review.

One more thing: I've noticed changes in sockmap_helpers.h don't trigger
test_progs rebuild (seems to be the case for all .h in prog_tests/). No
idea if this is the right approach, but adding
"$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
selftests/bpf/Makefile does the trick.

> Classic cleanup with goto to close sockets is all right, but if you're
> feeling brave and aim for something less branchy, I've noticed we have
> finally started using __attribute__((cleanup)):
> 
> https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115

I've tried. Is such "ownership passing" (to inhibit the cleanup) via
construct like take_fd()[1] welcomed?

[1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/

static inline void close_fd(int *fd)
{
	if (*fd >= 0)
		xclose(*fd);
}

#define __closefd __attribute__((cleanup(close_fd)))

static inline int create_pair(int family, int sotype, int *c, int *p)
{
	struct sockaddr_storage addr;
	socklen_t len = sizeof(addr);
	int err;

	int s __closefd = socket_loopback(family, sotype);
	if (s < 0)
		return s;

	err = xgetsockname(s, sockaddr(&addr), &len);
	if (err)
		return err;

	int s0 __closefd = xsocket(family, sotype, 0);
	if (s0 < 0)
		return s0;

	err = connect(s0, sockaddr(&addr), len);
	if (err) {
		if (errno != EINPROGRESS) {
			FAIL_ERRNO("connect");
			return err;
		}

		err = poll_connect(s0, IO_TIMEOUT_SEC);
		if (err) {
			FAIL_ERRNO("poll_connect");
			return err;
		}
	}

	switch (sotype & SOCK_TYPE_MASK) {
	case SOCK_DGRAM:
		err = xgetsockname(s0, sockaddr(&addr), &len);
		if (err)
			return err;

		err = xconnect(s, sockaddr(&addr), len);
		if (err)
			return err;

		*p = take_fd(s);
		break;
	case SOCK_STREAM:
	case SOCK_SEQPACKET:
		*p = xaccept_nonblock(s, NULL, NULL);
		if (*p < 0)
			return *p;
		break;
	default:
		FAIL("Unsupported socket type %#x", sotype);
		return -EOPNOTSUPP;
	}

	*c = take_fd(s0);
	return err;
}

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

[Jakub Sitnicki]

On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
> On 7/19/24 13:09, Jakub Sitnicki wrote:
>> On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
>>> On 7/13/24 11:45, Jakub Sitnicki wrote:
>>>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>>>> @type ignored, too.  Same treatment?
>>>>
>>>> That one will not be a trivial fix like this case. inet_socketpair()
>>>> won't work for TCP as is. It will fail trying to connect() a listening
>>>> socket (p0). I recall now that we are in this state due to some
>>>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>>>> inet in some function names").
>>>> [...]
>>>
>>> Is this what you've meant? With this patch inet_socketpair() and
>>> vsock_socketpair_connectible can be reduced to a single call to
>>> create_pair(). And pairs creation in inet_unix_redir_to_connected()
>>> and unix_inet_redir_to_connected() accepts both sotypes.
>> 
>> Yes, exactly. This looks great.
>
> Happy to hear that. I'll prepare a series, include the little fixes and
> send it out for a proper review.
>
> One more thing: I've noticed changes in sockmap_helpers.h don't trigger
> test_progs rebuild (seems to be the case for all .h in prog_tests/). No
> idea if this is the right approach, but adding
> "$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
> selftests/bpf/Makefile does the trick.

CC'ed BPF selftests reviewers in case they'd like to chip in.

>
>> Classic cleanup with goto to close sockets is all right, but if you're
>> feeling brave and aim for something less branchy, I've noticed we have
>> finally started using __attribute__((cleanup)):
>> 
>> https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115
>
> I've tried. Is such "ownership passing" (to inhibit the cleanup) via
> construct like take_fd()[1] welcomed?

I'm fine with having such a helper to complement the cleanup attribute.
Alternatively, we can always open code it like it used to be in systemd
at first [1], if other reviewers don't warm up to it :-)

[1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci


>
> [1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/
>
> static inline void close_fd(int *fd)
> {
> 	if (*fd >= 0)
> 		xclose(*fd);
> }
>
> #define __closefd __attribute__((cleanup(close_fd)))
>
> static inline int create_pair(int family, int sotype, int *c, int *p)
> {
> 	struct sockaddr_storage addr;
> 	socklen_t len = sizeof(addr);
> 	int err;
>
> 	int s __closefd = socket_loopback(family, sotype);
> 	if (s < 0)
> 		return s;
>
> 	err = xgetsockname(s, sockaddr(&addr), &len);
> 	if (err)
> 		return err;
>
> 	int s0 __closefd = xsocket(family, sotype, 0);

I'd stick to no declarations in the body. Init to -1 or -EBADF.

> 	if (s0 < 0)
> 		return s0;
>
> 	err = connect(s0, sockaddr(&addr), len);
> 	if (err) {
> 		if (errno != EINPROGRESS) {
> 			FAIL_ERRNO("connect");
> 			return err;
> 		}
>
> 		err = poll_connect(s0, IO_TIMEOUT_SEC);
> 		if (err) {
> 			FAIL_ERRNO("poll_connect");
> 			return err;
> 		}
> 	}
>
> 	switch (sotype & SOCK_TYPE_MASK) {
> 	case SOCK_DGRAM:
> 		err = xgetsockname(s0, sockaddr(&addr), &len);
> 		if (err)
> 			return err;
>
> 		err = xconnect(s, sockaddr(&addr), len);
> 		if (err)
> 			return err;
>
> 		*p = take_fd(s);
> 		break;
> 	case SOCK_STREAM:
> 	case SOCK_SEQPACKET:
> 		*p = xaccept_nonblock(s, NULL, NULL);

I wouldn't touch output arguments until we have succedeed.  Another
local var will be handy.

> 		if (*p < 0)
> 			return *p;
> 		break;
> 	default:
> 		FAIL("Unsupported socket type %#x", sotype);
> 		return -EOPNOTSUPP;
> 	}
>
> 	*c = take_fd(s0);
> 	return err;
> }

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

[Eduard Zingerman]

On Mon, 2024-07-22 at 21:26 +0200, Jakub Sitnicki wrote:
> On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:

[...]

> > One more thing: I've noticed changes in sockmap_helpers.h don't trigger
> > test_progs rebuild (seems to be the case for all .h in prog_tests/). No
> > idea if this is the right approach, but adding
> > "$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
> > selftests/bpf/Makefile does the trick.
> 
> CC'ed BPF selftests reviewers in case they'd like to chip in.

Are you sure this is reproducible?

I tried the following:

$ make clean
$ make -j test_progs
$ touch prog_tests/sockmap_helpers.h
$ make -j test_progs

And I see the following files being remade:

  TEST-OBJ [test_progs] sockmap_basic.test.o
  TEST-OBJ [test_progs] sockmap_listen.test.o
  TEST-OBJ [test_progs] verifier.test.o
  BINARY   test_progs

(Although, there are a few other files,
 that probably should not be remade, need to look into it).

Also, here is some debug output:

$ make -j24 --print-data-base | grep "sockmap_basic.test.o:" | tr ' ' '\n' | grep '\(:\|sockmap_helpers.h\)'

/home/eddy/work/bpf-next/tools/testing/selftests/bpf/cpuv4/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h

/home/eddy/work/bpf-next/tools/testing/selftests/bpf/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h

/home/eddy/work/bpf-next/tools/testing/selftests/bpf/no_alu32/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h


[...]

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

[Eduard Zingerman]

On Mon, 2024-07-22 at 15:07 -0700, Eduard Zingerman wrote:

[...]

Digging a little bit further, I think the behaviour mentioned was fixed
recently by the following commit:

a3cc56cd2c20 ("selftests/bpf: Use auto-dependencies for test objects")

From 3 days ago.

As the dependency is set from sockmap_basic.test.d,
generated while sockmap_basic.test.o is compiled.

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

[Michal Luczaj]

On 7/23/24 00:21, Eduard Zingerman wrote:
> On Mon, 2024-07-22 at 15:07 -0700, Eduard Zingerman wrote:
> 
> [...]
> 
> Digging a little bit further, I think the behaviour mentioned was fixed
> recently by the following commit:
> 
> a3cc56cd2c20 ("selftests/bpf: Use auto-dependencies for test objects")
> 
> From 3 days ago.
> 
> As the dependency is set from sockmap_basic.test.d,
> generated while sockmap_basic.test.o is compiled.

Ah, yes, you're right: bpf-next works for me. Thank you very much for
solving this. And I apologise for the noise.

Michal

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

[Michal Luczaj]

On 7/22/24 21:26, Jakub Sitnicki wrote:
> On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
>>> Classic cleanup with goto to close sockets is all right, but if you're
>>> feeling brave and aim for something less branchy, I've noticed we have
>>> finally started using __attribute__((cleanup)):
>>>
>>> https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115
>>
>> I've tried. Is such "ownership passing" (to inhibit the cleanup) via
>> construct like take_fd()[1] welcomed?
> 
> I'm fine with having such a helper to complement the cleanup attribute.
> Alternatively, we can always open code it like it used to be in systemd
> at first [1], if other reviewers don't warm up to it :-)
> 
> [1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci

OK, so I've kept create_pair()'s __cleanupfication as the last part of the
series:
https://lore.kernel.org/netdev/20240724-sockmap-selftest-fixes-v1-0-46165d224712@rbox.co

>> [1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/
>>
>> static inline void close_fd(int *fd)
>> {
>> 	if (*fd >= 0)
>> 		xclose(*fd);
>> }
>>
>> #define __closefd __attribute__((cleanup(close_fd)))
>>
>> static inline int create_pair(int family, int sotype, int *c, int *p)
>> {
>> 	struct sockaddr_storage addr;
>> 	socklen_t len = sizeof(addr);
>> 	int err;
>>
>> 	int s __closefd = socket_loopback(family, sotype);
>> 	if (s < 0)
>> 		return s;
>>
>> 	err = xgetsockname(s, sockaddr(&addr), &len);
>> 	if (err)
>> 		return err;
>>
>> 	int s0 __closefd = xsocket(family, sotype, 0);
> 
> I'd stick to no declarations in the body. Init to -1 or -EBADF.

All right, it just felt wrong to (demand to) initialize variables with some
magic values. __attribute__((setup(set_negative))) would solve that :) I've
toyed with `DEFINE_CLASS(fd, int, if (_T >= 0) xclose(_T), -EBADF, void)`
but it felt wrong, too.

>> 	case SOCK_STREAM:
>> 	case SOCK_SEQPACKET:
>> 		*p = xaccept_nonblock(s, NULL, NULL);
> 
> I wouldn't touch output arguments until we have succedeed.  Another
> local var will be handy.

OK, sure. Thanks.