From: Jakub Sitnicki <jakub@cloudflare.com>
To: Michal Luczaj <mhal@rbox.co>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, john.fastabend@gmail.com, kuniyu@amazon.com,
Rao.Shoaib@oracle.com, cong.wang@bytedance.com
Subject: Re: [PATCH bpf v3 3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags
Date: Tue, 09 Jul 2024 11:59:47 +0200 [thread overview]
Message-ID: <87v81enawc.fsf@cloudflare.com> (raw)
In-Reply-To: <20240707222842.4119416-4-mhal@rbox.co> (Michal Luczaj's message of "Sun, 7 Jul 2024 23:28:24 +0200")
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);
}
next prev parent reply other threads:[~2024-07-09 9:59 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-07 21:28 [PATCH bpf v3 0/4] af_unix: MSG_OOB handling fix & selftest Michal Luczaj
2024-07-07 21:28 ` [PATCH bpf v3 1/4] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash Michal Luczaj
2024-07-08 19:38 ` Kuniyuki Iwashima
2024-07-09 1:24 ` John Fastabend
2024-07-09 2:18 ` Kuniyuki Iwashima
2024-07-09 9:48 ` Jakub Sitnicki
2024-07-07 21:28 ` [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected() Michal Luczaj
2024-07-09 9:48 ` Jakub Sitnicki
2024-07-11 20:33 ` Michal Luczaj
2024-07-13 9:45 ` Jakub Sitnicki
2024-07-13 20:16 ` Michal Luczaj
2024-07-16 9:14 ` Jakub Sitnicki
2024-07-16 20:58 ` Michal Luczaj
2024-07-17 20:15 ` Michal Luczaj
2024-07-19 11:09 ` Jakub Sitnicki
2024-07-22 13:07 ` Michal Luczaj
2024-07-22 19:26 ` Jakub Sitnicki
2024-07-22 22:07 ` Eduard Zingerman
2024-07-22 22:21 ` Eduard Zingerman
2024-07-23 12:31 ` Michal Luczaj
2024-07-24 11:36 ` Michal Luczaj
2024-07-07 21:28 ` [PATCH bpf v3 3/4] selftest/bpf: Parametrize AF_UNIX redir functions to accept send() flags Michal Luczaj
2024-07-09 9:59 ` Jakub Sitnicki [this message]
2024-07-11 20:34 ` Michal Luczaj
2024-07-07 21:28 ` [PATCH bpf v3 4/4] selftest/bpf: Test sockmap redirect for AF_UNIX MSG_OOB Michal Luczaj
2024-07-09 10:08 ` Jakub Sitnicki
2024-07-11 20:35 ` Michal Luczaj
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v81enawc.fsf@cloudflare.com \
--to=jakub@cloudflare.com \
--cc=Rao.Shoaib@oracle.com \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=kuniyu@amazon.com \
--cc=mhal@rbox.co \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.