On Wed, 11 Mar 2020 at 13:52, Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Mar 10, 2020 at 06:47 PM CET, Lorenz Bauer wrote:
> > 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 <lmb@xxxxxxxxxxxxxx>
> > ---
> > .../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.
>
> > +
> > +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
