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

[Philo Lu]

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 <tiago...@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.

  }

--
Philo