Re: [PATCH 5/5] bpf: sockmap, sockhash: test looking up fds

On Wed, 11 Mar 2020 at 13:52, Jakub Sitnicki [off-list ref] wrote:

On Tue, Mar 10, 2020 at 06:47 PM CET, Lorenz Bauer wrote:

quoted

Make sure that looking up an element from the map succeeds,
and that the fd is valid by using it an fcntl call.

Signed-off-by: Lorenz Bauer <redacted>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c | 26 ++++++++++++++-----
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 52aa468bdccd..929e1e77ecc6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c

@@ -453,7 +453,7 @@ static void test_lookup_after_delete(int family, int sotype, int mapfd)
      xclose(s);
 }

-static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
+static void test_lookup_fd(int family, int sotype, int mapfd)
 {
      u32 key, value32;
      int err, s;

@@ -466,7 +466,7 @@ static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
                             sizeof(value32), 1, 0);
      if (mapfd < 0) {
              FAIL_ERRNO("map_create");
-             goto close;
+             goto close_sock;
      }

      key = 0;

@@ -475,11 +475,25 @@ static void test_lookup_32_bit_value(int family, int sotype, int mapfd)

      errno = 0;
      err = bpf_map_lookup_elem(mapfd, &key, &value32);
-     if (!err || errno != ENOSPC)
-             FAIL_ERRNO("map_lookup: expected ENOSPC");
+     if (err) {
+             FAIL_ERRNO("map_lookup");
+             goto close_map;
+     }

+     if ((int)value32 == s) {
+             FAIL("return value is identical");
+             goto close;
+     }
+
+     err = fcntl(value32, F_GETFD);
+     if (err == -1)
+             FAIL_ERRNO("fcntl");

I would call getsockopt()/getsockname() to assert that the FD lookup
succeeded.  We want to know not only that it's an FD (-EBADFD case), but
also that it's associated with a socket (-ENOTSOCK).

We can go even further, and compare socket cookies to ensure we got an
FD for the expected socket.

Good idea, thanks!

Also, I'm wondering if we could keep the -ENOSPC case test-covered by
temporarily dropping NET_ADMIN capability.

You mean EPERM? ENOSPC isn't reachable, since the map can only be created
with a map_value of 4 or 8.

quoted

+
+close:
+     xclose(value32);
+close_map:
      xclose(mapfd);
-close:
+close_sock:
      xclose(s);
 }

@@ -1456,7 +1470,7 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
              /* lookup */
              TEST(test_lookup_after_insert),
              TEST(test_lookup_after_delete),
-             TEST(test_lookup_32_bit_value),
+             TEST(test_lookup_fd),
              /* update */
              TEST(test_update_existing),
              /* races with insert/delete */

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com