| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zumnk3MEqINMnTf6@GHGHG14>
Date: Tue, 17 Sep 2024 17:00:19 +0100
From: Tiago Lam <tiagolam@...udflare.com>
To: Philo Lu <lulie@...ux.alibaba.com>
Cc: "David S. Miller" <davem@...emloft.net>,
David Ahern <dsahern@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
Jakub Sitnicki <jakub@...udflare.com>, kernel-team@...udflare.com
Subject: Re: [RFC PATCH 3/3] bpf: Add sk_lookup test to use ORIGDSTADDR cmsg.
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@...udflare.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)
Powered by blists - more mailing lists