| 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
| ||
|
Message-ID: <CABdmKX2h5cGjNbJshGkQ+2XJ7eOnM+VfbmVr5Pj5c0qfxQA-qg@mail.gmail.com>
Date: Fri, 9 May 2025 14:43:09 -0700
From: "T.J. Mercier" <tjmercier@...gle.com>
To: Song Liu <song@...nel.org>
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 Fri, May 9, 2025 at 11:46 AM Song Liu <song@...nel.org> wrote:
>
> 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().
That's a lot of different stuff to put in one place. How about
returning file descriptors from the buffer create functions while
having them clean up after themselves:
-static int memfd, udmabuf;
+static int udmabuf;
static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] =
"udmabuf_test_buffer_for_iter";
static size_t udmabuf_test_buffer_size;
static int sysheap_dmabuf;
static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] =
"sysheap_test_buffer_for_iter";
static size_t sysheap_test_buffer_size;
-static int create_udmabuf(int map_fd)
+static int create_udmabuf(void)
{
struct udmabuf_create create;
- int dev_udmabuf;
- bool f = false;
+ int dev_udmabuf, memfd, udmabuf;
udmabuf_test_buffer_size = 10 * getpagesize();
if (!ASSERT_LE(sizeof(udmabuf_test_buffer_name),
DMA_BUF_NAME_LEN, "NAMETOOLONG"))
- return 1;
+ return -1;
memfd = memfd_create("memfd_test", MFD_ALLOW_SEALING);
if (!ASSERT_OK_FD(memfd, "memfd_create"))
- return 1;
+ return -1;
if (!ASSERT_OK(ftruncate(memfd, udmabuf_test_buffer_size), "ftruncate"))
- return 1;
+ goto close_memfd;
if (!ASSERT_OK(fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK), "seal"))
- return 1;
+ goto close_memfd;
dev_udmabuf = open("/dev/udmabuf", O_RDONLY);
if (!ASSERT_OK_FD(dev_udmabuf, "open udmabuf"))
- return 1;
+ goto close_memfd;
create.memfd = memfd;
create.flags = UDMABUF_FLAGS_CLOEXEC;
@@ -59,15 +58,21 @@ static int create_udmabuf(int map_fd)
udmabuf = ioctl(dev_udmabuf, UDMABUF_CREATE, &create);
close(dev_udmabuf);
if (!ASSERT_OK_FD(udmabuf, "udmabuf_create"))
- return 1;
+ goto close_memfd;
if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B,
udmabuf_test_buffer_name), "name"))
- return 1;
+ goto close_udmabuf;
+
+ return udmabuf;
- return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name,
&f, BPF_ANY);
+close_udmabuf:
+ close(udmabuf);
+close_memfd:
+ close(memfd);
+ return -1;
}
-static int create_sys_heap_dmabuf(int map_fd)
+static int create_sys_heap_dmabuf(void)
{
sysheap_test_buffer_size = 20 * getpagesize();
@@ -78,43 +83,46 @@ static int create_sys_heap_dmabuf(int map_fd)
.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;
+ return -1;
heap_fd = open("/dev/dma_heap/system", O_RDONLY);
if (!ASSERT_OK_FD(heap_fd, "open dma heap"))
- return 1;
+ return -1;
ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &data);
close(heap_fd);
if (!ASSERT_OK(ret, "syheap alloc"))
- return 1;
+ return -1;
- sysheap_dmabuf = data.fd;
+ if (!ASSERT_OK(ioctl(data.fd, DMA_BUF_SET_NAME_B,
sysheap_test_buffer_name), "name"))
+ goto close_sysheap_dmabuf;
- if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B,
sysheap_test_buffer_name), "name"))
- return 1;
+ return data.fd;
- return bpf_map_update_elem(map_fd, sysheap_test_buffer_name,
&f, BPF_ANY);
+close_sysheap_dmabuf:
+ close(data.fd);
+ return -1;
}
static int create_test_buffers(int map_fd)
{
- int ret;
+ bool f = false;
+
+ udmabuf = create_udmabuf();
+ sysheap_dmabuf = create_sys_heap_dmabuf();
- ret = create_udmabuf(map_fd);
- if (ret)
- return ret;
+ if (udmabuf < 0 || sysheap_dmabuf < 0)
+ return -1;
- return create_sys_heap_dmabuf(map_fd);
+ return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name,
&f, BPF_ANY) ||
+ bpf_map_update_elem(map_fd, sysheap_test_buffer_name,
&f, BPF_ANY);
}
static void destroy_test_buffers(void)
{
close(udmabuf);
- close(memfd);
close(sysheap_dmabuf);
}
Powered by blists - more mailing lists