Re: [RFC PATCH 3/3] bpf: Add sk_lookup test to use ORIGDSTADDR cmsg.

[Tiago Lam]

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@xxxxxxxxxxxxxx>
> > ---
> >   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)