linux-kernel - Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW5WOmyfPqBc_Hn7ApGWP_2uz_cJwyaDWF_VwiHJu9s_1A@mail.gmail.com>
Date: Fri, 9 May 2025 11:46:37 -0700
From: Song Liu <song@...nel.org>
To: "T.J. Mercier" <tjmercier@...gle.com>
Cc: sumit.semwal@...aro.org, christian.koenig@....com, ast@...nel.org, 
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev, 
	skhan@...uxfoundation.org, alexei.starovoitov@...il.com, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org, 
	bpf@...r.kernel.org, linux-kselftest@...r.kernel.org, android-mm@...gle.com, 
	simona@...ll.ch, eddyz87@...il.com, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	jolsa@...nel.org, mykolal@...com, shuah@...nel.org
Subject: Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter

On Thu, May 8, 2025 at 11:21 AM T.J. Mercier <tjmercier@...gle.com> wrote:
>
> Use the same test buffers as the traditional iterator and a new BPF map
> to verify the test buffers can be found with the open coded dmabuf
> iterator.

The way we split 4/5 and 5/5 makes the code tricker to follow. I guess
the motivation is to back port default iter along to older kernels. But I
think we can still make the code cleaner.

>
> Signed-off-by: T.J. Mercier <tjmercier@...gle.com>
> ---
[...]

>
> -static int create_udmabuf(void)
> +static int create_udmabuf(int map_fd)
>  {
>         struct udmabuf_create create;
>         int dev_udmabuf;
> +       bool f = false;
>
>         udmabuf_test_buffer_size = 10 * getpagesize();
>
> @@ -63,10 +64,10 @@ static int create_udmabuf(void)
>         if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B, udmabuf_test_buffer_name), "name"))
>                 return 1;
>
> -       return 0;
> +       return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY);

We don't really need this bpf_map_update_elem() inside
create_udmabuf(), right?

>  }
>
> -static int create_sys_heap_dmabuf(void)
> +static int create_sys_heap_dmabuf(int map_fd)
>  {
>         sysheap_test_buffer_size = 20 * getpagesize();
>
> @@ -77,6 +78,7 @@ static int create_sys_heap_dmabuf(void)
>                 .heap_flags = 0,
>         };
>         int heap_fd, ret;
> +       bool f = false;
>
>         if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG"))
>                 return 1;
> @@ -95,18 +97,18 @@ static int create_sys_heap_dmabuf(void)
>         if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name"))
>                 return 1;
>
> -       return 0;
> +       return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY);

Same for this bpf_map_update_elem(), we can call this directly from
create_test_buffers().

>  }
>
> -static int create_test_buffers(void)
> +static int create_test_buffers(int map_fd)
>  {
>         int ret;
>
> -       ret = create_udmabuf();
> +       ret = create_udmabuf(map_fd);
>         if (ret)
>                 return ret;
>
> -       return create_sys_heap_dmabuf();
> +       return create_sys_heap_dmabuf(map_fd);

Personally, I would prefer we just merge all the logic of
create_udmabuf() and create_sys_heap_dmabuf()
into create_test_buffers().

>  }
>
[...]
> +
>  void test_dmabuf_iter(void)
>  {
>         struct dmabuf_iter *skel = NULL;
> +       int iter_fd, map_fd;
>         char buf[256];
> -       int iter_fd;
>
>         skel = dmabuf_iter__open_and_load();
>         if (!ASSERT_OK_PTR(skel, "dmabuf_iter__open_and_load"))
>                 return;
>
> -       if (!ASSERT_OK(create_test_buffers(), "create_buffers"))
> +       map_fd = bpf_map__fd(skel->maps.testbuf_hash);
> +       if (!ASSERT_OK_FD(map_fd, "map_fd"))
> +               goto destroy_skel;
> +
> +       if (!ASSERT_OK(create_test_buffers(map_fd), "create_buffers"))
>                 goto destroy;
>
>         if (!ASSERT_OK(dmabuf_iter__attach(skel), "skel_attach"))
> @@ -215,10 +246,13 @@ void test_dmabuf_iter(void)
>
>         if (test__start_subtest("default_iter"))
>                 subtest_dmabuf_iter_check_default_iter(skel);
> +       if (test__start_subtest("open_coded"))
> +               subtest_dmabuf_iter_check_open_coded(skel, map_fd);
>
>         close(iter_fd);
>
>  destroy:
>         destroy_test_buffers();
> +destroy_skel:
>         dmabuf_iter__destroy(skel);
>  }
[...]