| Message ID | 20240724-sockmap-selftest-fixes-v1-1-46165d224712@rbox.co |
|---|---|
| State | Accepted |
| Commit | 190de5449973056f416c855b11fdfee2fc18f550 |
| Headers | show |
| Series | [bpf,1/6] selftest/bpf: Support more socket types in create_pair() | expand |
On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote: > Extend the function to allow creating socket pairs of SOCK_STREAM, > SOCK_DGRAM and SOCK_SEQPACKET. > > Adapt direct callers and leave further cleanups for the following patch. > > Suggested-by: Jakub Sitnicki <jakub@cloudflare.com> > Signed-off-by: Michal Luczaj <mhal@rbox.co> > --- > .../selftests/bpf/prog_tests/sockmap_basic.c | 19 +-- > .../selftests/bpf/prog_tests/sockmap_helpers.h | 138 ++++++++++++++------- > 2 files changed, 96 insertions(+), 61 deletions(-) > [...] > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h > index e880f97bc44d..77b73333f091 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h [...] > +static inline int create_pair(int family, int sotype, int *p0, int *p1) > +{ > + struct sockaddr_storage addr; > + socklen_t len = sizeof(addr); > + int s, c, p, 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; > + } > + } > + > + switch (sotype & SOCK_TYPE_MASK) { > + case SOCK_DGRAM: > + err = xgetsockname(c, sockaddr(&addr), &len); > + if (err) > + goto close_c; > + > + err = xconnect(s, sockaddr(&addr), len); > + if (!err) { > + *p0 = s; > + *p1 = c; > + return err; > + } > + break; > + case SOCK_STREAM: > + case SOCK_SEQPACKET: > + p = xaccept_nonblock(s, NULL, NULL); > + if (p >= 0) { > + *p0 = p; > + *p1 = c; > + goto close_s; > + } > + > + err = p; > + break; > + default: > + FAIL("Unsupported socket type %#x", sotype); > + err = -EOPNOTSUPP; > + } > + > +close_c: > + close(c); > +close_s: > + close(s); > + return err; > +} I was going to suggest that a single return path for success is better than two (diff below), but I see that this is what you ended up with after patch 6. So I think we can leave it as is. --- diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h index 77b73333f091..ed266c6c0117 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h @@ -408,28 +408,31 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1) goto close_c; err = xconnect(s, sockaddr(&addr), len); - if (!err) { - *p0 = s; - *p1 = c; - return err; - } + if (err) + goto close_c; + + p = s; break; case SOCK_STREAM: case SOCK_SEQPACKET: p = xaccept_nonblock(s, NULL, NULL); - if (p >= 0) { - *p0 = p; - *p1 = c; - goto close_s; + if (p < 0) { + err = p; + goto close_c; } - err = p; + xclose(s); break; default: FAIL("Unsupported socket type %#x", sotype); err = -EOPNOTSUPP; + goto close_c; } + *p0 = p; + *p1 = c; + return 0; + close_c: close(c); close_s:
On 7/26/24 19:23, Jakub Sitnicki wrote: > I was going to suggest that a single return path for success is better > than two (diff below), but I see that this is what you ended up with > after patch 6. > > So I think we can leave it as is. > [...] And speaking of which, would you rather have patch 1 and 6 squashed?
On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote: > On 7/26/24 19:23, Jakub Sitnicki wrote: >> I was going to suggest that a single return path for success is better >> than two (diff below), but I see that this is what you ended up with >> after patch 6. >> >> So I think we can leave it as is. >> [...] > > And speaking of which, would you rather have patch 1 and 6 squashed? Don't have a straight answer, sorry . Would have to see if the diff is clear enough after squashing it. Use your best judgement. It's certainly fine with me to review the steps that were taken to massage the code.
On 7/30/24 19:13, Jakub Sitnicki wrote: > On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote: >> On 7/26/24 19:23, Jakub Sitnicki wrote: >>> I was going to suggest that a single return path for success is better >>> than two (diff below), but I see that this is what you ended up with >>> after patch 6. >>> >>> So I think we can leave it as is. >>> [...] >> >> And speaking of which, would you rather have patch 1 and 6 squashed? > > Don't have a straight answer, sorry . Would have to see if the diff is > clear enough after squashing it. Use your best judgement. > > It's certainly fine with me to review the steps that were taken to > massage the code. That's what I've assumed, thanks. So here's the bpf-next based respin: https://lore.kernel.org/bpf/20240731-selftest-sockmap-fixes-v2-0-08a0c73abed2@rbox.co
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..77b73333f091 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h @@ -3,6 +3,9 @@ #include <linux/vm_sockets.h> +/* include/linux/net.h */ +#define SOCK_TYPE_MASK 0xf + #define IO_TIMEOUT_SEC 30 #define MAX_STRERR_LEN 256 #define MAX_TEST_NAME 80 @@ -312,54 +315,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 +367,92 @@ 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 *p0, int *p1) +{ + struct sockaddr_storage addr; + socklen_t len = sizeof(addr); + int s, c, p, 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; + } + } + + switch (sotype & SOCK_TYPE_MASK) { + case SOCK_DGRAM: + err = xgetsockname(c, sockaddr(&addr), &len); + if (err) + goto close_c; + + err = xconnect(s, sockaddr(&addr), len); + if (!err) { + *p0 = s; + *p1 = c; + return err; + } + break; + case SOCK_STREAM: + case SOCK_SEQPACKET: + p = xaccept_nonblock(s, NULL, NULL); + if (p >= 0) { + *p0 = p; + *p1 = c; + goto close_s; + } + + err = p; + break; + default: + FAIL("Unsupported socket type %#x", sotype); + err = -EOPNOTSUPP; + } + +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__
Extend the function to allow creating socket pairs of SOCK_STREAM, SOCK_DGRAM and SOCK_SEQPACKET. Adapt direct callers and leave further cleanups for the following patch. Suggested-by: Jakub Sitnicki <jakub@cloudflare.com> Signed-off-by: Michal Luczaj <mhal@rbox.co> --- .../selftests/bpf/prog_tests/sockmap_basic.c | 19 +-- .../selftests/bpf/prog_tests/sockmap_helpers.h | 138 ++++++++++++++------- 2 files changed, 96 insertions(+), 61 deletions(-)