| Message ID | 20240707222842.4119416-3-mhal@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | af_unix: MSG_OOB handling fix & selftest | expand |
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>
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/
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.
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/
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.
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.
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__
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 [...]
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; }
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; > }
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 [...]
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.
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
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.
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,
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(-)