| Message ID | 20240913-reverse-sk-lookup-v1-3-e721ea003d4c@cloudflare.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | Allow sk_lookup UDP return traffic to egress. | expand |
Hi Tiago, On 2024/9/13 17:39, Tiago Lam wrote: > This patch reuses the framework already in place for sk_lookup, allowing > it now to send a reply from the server fd directly, instead of having to > create a socket bound to the original destination address and reply from > there. It does this by passing the source address and port from where to > reply from in a IP_ORIGDSTADDR ancilliary message passed in sendmsg. > > Signed-off-by: Tiago Lam <tiagolam@cloudflare.com> > --- > tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 70 +++++++++++++++------- > 1 file changed, 48 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > index ae87c00867ba..b99aff2e3976 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > @@ -75,6 +75,7 @@ struct test { > struct inet_addr listen_at; > enum server accept_on; > bool reuseport_has_conns; /* Add a connected socket to reuseport group */ > + bool dont_bind_reply; /* Don't bind, send direct from server fd. */ > }; > > struct cb_opts { > @@ -363,7 +364,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) > memset(&v6->sin6_addr.s6_addr[0], 0, 10); > } > > -static int udp_recv_send(int server_fd) > +static int udp_recv_send(int server_fd, bool dont_bind_reply) > { > char cmsg_buf[CMSG_SPACE(sizeof(struct sockaddr_storage))]; > struct sockaddr_storage _src_addr = { 0 }; > @@ -415,26 +416,41 @@ static int udp_recv_send(int server_fd) > v4_to_v6(dst_addr); > } > > - /* Reply from original destination address. */ > - fd = start_server_addr(SOCK_DGRAM, dst_addr, sizeof(*dst_addr), NULL); > - if (!ASSERT_OK_FD(fd, "start_server_addr")) { > - log_err("failed to create tx socket"); > - return -1; > - } > + if (dont_bind_reply) { > + /* Reply directly from server fd, specifying the source address and/or > + * port in struct msghdr. > + */ > + n = sendmsg(server_fd, &msg, 0); > + if (CHECK(n <= 0, "sendmsg", "failed\n")) { > + log_err("failed to send echo reply"); > + return -1; > + } > + } else { > + /* Reply from original destination address. */ > + fd = socket(dst_addr->ss_family, SOCK_DGRAM, 0); > + if (CHECK(fd < 0, "socket", "failed\n")) { > + log_err("failed to create tx socket"); > + return -1; > + } > Just curious, why not use start_server_addr() here like before? > - msg.msg_control = NULL; > - msg.msg_controllen = 0; > - n = sendmsg(fd, &msg, 0); > - if (CHECK(n <= 0, "sendmsg", "failed\n")) { > - log_err("failed to send echo reply"); > - ret = -1; > - goto out; > - } > + ret = bind(fd, (struct sockaddr *)dst_addr, sizeof(*dst_addr)); > + if (CHECK(ret, "bind", "failed\n")) { > + log_err("failed to bind tx socket"); > + close(fd); > + return ret; > + } > > - ret = 0; > -out: > - close(fd); > - return ret; > + msg.msg_control = NULL; > + msg.msg_controllen = 0; > + n = sendmsg(fd, &msg, 0); > + if (CHECK(n <= 0, "sendmsg", "failed\n")) { > + log_err("failed to send echo reply"); > + close(fd); > + return -1; > + } > + > + close(fd); > + } nit: "return 0" missed. > } >
On Fri, Sep 13, 2024 at 08:10:24PM +0800, Philo Lu wrote: > Hi Tiago, > > On 2024/9/13 17:39, Tiago Lam wrote: > > This patch reuses the framework already in place for sk_lookup, allowing > > it now to send a reply from the server fd directly, instead of having to > > create a socket bound to the original destination address and reply from > > there. It does this by passing the source address and port from where to > > reply from in a IP_ORIGDSTADDR ancilliary message passed in sendmsg. > > > > Signed-off-by: Tiago Lam <tiagolam@cloudflare.com> > > --- > > tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 70 +++++++++++++++------- > > 1 file changed, 48 insertions(+), 22 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > index ae87c00867ba..b99aff2e3976 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > @@ -75,6 +75,7 @@ struct test { > > struct inet_addr listen_at; > > enum server accept_on; > > bool reuseport_has_conns; /* Add a connected socket to reuseport group */ > > + bool dont_bind_reply; /* Don't bind, send direct from server fd. */ > > }; > > struct cb_opts { > > @@ -363,7 +364,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) > > memset(&v6->sin6_addr.s6_addr[0], 0, 10); > > } > > -static int udp_recv_send(int server_fd) > > +static int udp_recv_send(int server_fd, bool dont_bind_reply) > > { > > char cmsg_buf[CMSG_SPACE(sizeof(struct sockaddr_storage))]; > > struct sockaddr_storage _src_addr = { 0 }; > > @@ -415,26 +416,41 @@ static int udp_recv_send(int server_fd) > > v4_to_v6(dst_addr); > > } > > - /* Reply from original destination address. */ > > - fd = start_server_addr(SOCK_DGRAM, dst_addr, sizeof(*dst_addr), NULL); > > - if (!ASSERT_OK_FD(fd, "start_server_addr")) { > > - log_err("failed to create tx socket"); > > - return -1; > > - } > > + if (dont_bind_reply) { > > + /* Reply directly from server fd, specifying the source address and/or > > + * port in struct msghdr. > > + */ > > + n = sendmsg(server_fd, &msg, 0); > > + if (CHECK(n <= 0, "sendmsg", "failed\n")) { > > + log_err("failed to send echo reply"); > > + return -1; > > + } > > + } else { > > + /* Reply from original destination address. */ > > + fd = socket(dst_addr->ss_family, SOCK_DGRAM, 0); > > + if (CHECK(fd < 0, "socket", "failed\n")) { > > + log_err("failed to create tx socket"); > > + return -1; > > + } > Just curious, why not use start_server_addr() here like before? > Thanks, you're right. Initially I rebased this on commit e3d332aaf898ed755b29c8cdf59be2cfba1cac4b (v6.6.31), which didn't have start_server_addr(), and used bind() instead. I must have messed up the rebased. I've fixed this and included your other suggestion in the patch below and will fold it into the next revision. diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c index b99aff2e3976..0628a67681c5 100644 --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c @@ -374,7 +374,7 @@ static int udp_recv_send(int server_fd, bool dont_bind_reply) struct iovec iov = { 0 }; struct cmsghdr *cm; char buf[1]; - int ret, fd; + int fd; ssize_t n; iov.iov_base = buf; @@ -427,19 +427,12 @@ static int udp_recv_send(int server_fd, bool dont_bind_reply) } } else { /* Reply from original destination address. */ - fd = socket(dst_addr->ss_family, SOCK_DGRAM, 0); - if (CHECK(fd < 0, "socket", "failed\n")) { + fd = start_server_addr(SOCK_DGRAM, dst_addr, sizeof(*dst_addr), NULL); + if (!ASSERT_OK_FD(fd, "start_server_addr")) { log_err("failed to create tx socket"); return -1; } - ret = bind(fd, (struct sockaddr *)dst_addr, sizeof(*dst_addr)); - if (CHECK(ret, "bind", "failed\n")) { - log_err("failed to bind tx socket"); - close(fd); - return ret; - } - msg.msg_control = NULL; msg.msg_controllen = 0; n = sendmsg(fd, &msg, 0); @@ -451,6 +444,8 @@ static int udp_recv_send(int server_fd, bool dont_bind_reply) close(fd); } + + return 0; } static int tcp_echo_test(int client_fd, int server_fd)
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c index ae87c00867ba..b99aff2e3976 100644 --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c @@ -75,6 +75,7 @@ struct test { struct inet_addr listen_at; enum server accept_on; bool reuseport_has_conns; /* Add a connected socket to reuseport group */ + bool dont_bind_reply; /* Don't bind, send direct from server fd. */ }; struct cb_opts { @@ -363,7 +364,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) memset(&v6->sin6_addr.s6_addr[0], 0, 10); } -static int udp_recv_send(int server_fd) +static int udp_recv_send(int server_fd, bool dont_bind_reply) { char cmsg_buf[CMSG_SPACE(sizeof(struct sockaddr_storage))]; struct sockaddr_storage _src_addr = { 0 }; @@ -415,26 +416,41 @@ static int udp_recv_send(int server_fd) v4_to_v6(dst_addr); } - /* Reply from original destination address. */ - fd = start_server_addr(SOCK_DGRAM, dst_addr, sizeof(*dst_addr), NULL); - if (!ASSERT_OK_FD(fd, "start_server_addr")) { - log_err("failed to create tx socket"); - return -1; - } + if (dont_bind_reply) { + /* Reply directly from server fd, specifying the source address and/or + * port in struct msghdr. + */ + n = sendmsg(server_fd, &msg, 0); + if (CHECK(n <= 0, "sendmsg", "failed\n")) { + log_err("failed to send echo reply"); + return -1; + } + } else { + /* Reply from original destination address. */ + fd = socket(dst_addr->ss_family, SOCK_DGRAM, 0); + if (CHECK(fd < 0, "socket", "failed\n")) { + log_err("failed to create tx socket"); + return -1; + } - msg.msg_control = NULL; - msg.msg_controllen = 0; - n = sendmsg(fd, &msg, 0); - if (CHECK(n <= 0, "sendmsg", "failed\n")) { - log_err("failed to send echo reply"); - ret = -1; - goto out; - } + ret = bind(fd, (struct sockaddr *)dst_addr, sizeof(*dst_addr)); + if (CHECK(ret, "bind", "failed\n")) { + log_err("failed to bind tx socket"); + close(fd); + return ret; + } - ret = 0; -out: - close(fd); - return ret; + msg.msg_control = NULL; + msg.msg_controllen = 0; + n = sendmsg(fd, &msg, 0); + if (CHECK(n <= 0, "sendmsg", "failed\n")) { + log_err("failed to send echo reply"); + close(fd); + return -1; + } + + close(fd); + } } static int tcp_echo_test(int client_fd, int server_fd) @@ -454,14 +470,14 @@ static int tcp_echo_test(int client_fd, int server_fd) return 0; } -static int udp_echo_test(int client_fd, int server_fd) +static int udp_echo_test(int client_fd, int server_fd, bool dont_bind_reply) { int err; err = send_byte(client_fd); if (err) return -1; - err = udp_recv_send(server_fd); + err = udp_recv_send(server_fd, dont_bind_reply); if (err) return -1; err = recv_byte(client_fd); @@ -653,7 +669,7 @@ static void run_lookup_prog(const struct test *t) if (t->sotype == SOCK_STREAM) tcp_echo_test(client_fd, server_fds[t->accept_on]); else - udp_echo_test(client_fd, server_fds[t->accept_on]); + udp_echo_test(client_fd, server_fds[t->accept_on], t->dont_bind_reply); close(client_fd); close: @@ -775,6 +791,16 @@ static void test_redirect_lookup(struct test_sk_lookup *skel) .listen_at = { INT_IP4, INT_PORT }, .accept_on = SERVER_B, }, + { + .desc = "UDP IPv4 redir different ports", + .lookup_prog = skel->progs.select_sock_a_no_reuseport, + .sock_map = skel->maps.redir_map, + .sotype = SOCK_DGRAM, + .connect_to = { EXT_IP4, EXT_PORT }, + .listen_at = { INT_IP4, INT_PORT }, + .accept_on = SERVER_A, + .dont_bind_reply = true, + }, { .desc = "UDP IPv4 redir and reuseport with conns", .lookup_prog = skel->progs.select_sock_a,
This patch reuses the framework already in place for sk_lookup, allowing it now to send a reply from the server fd directly, instead of having to create a socket bound to the original destination address and reply from there. It does this by passing the source address and port from where to reply from in a IP_ORIGDSTADDR ancilliary message passed in sendmsg. Signed-off-by: Tiago Lam <tiagolam@cloudflare.com> --- tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 70 +++++++++++++++------- 1 file changed, 48 insertions(+), 22 deletions(-)