| 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: <CAHS8izNGphJPD6-PkATnOETj-LFLadSAKTe_A+FiJ_3Cax35ZA@mail.gmail.com>
Date: Thu, 3 Oct 2024 10:57:00 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v2 02/12] selftests: ncdevmem: Separate out
dmabuf provider
On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> So we can plug the other ones in the future if needed.
>
> Cc: Mina Almasry <almasrymina@...gle.com>
> Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
Feedback is minor, so with or without it:
Reviewed-by: Mina Almasry <almasrymina@...gle.com>
> ---
> tools/testing/selftests/net/ncdevmem.c | 215 +++++++++++++++----------
> 1 file changed, 134 insertions(+), 81 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 9245d3f158dd..557175c3bf02 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -71,17 +71,118 @@ static char *ifname = "eth1";
> static unsigned int ifindex;
> static unsigned int dmabuf_id;
>
> -void print_bytes(void *ptr, size_t size)
> +struct memory_buffer {
> + int fd;
> + size_t size;
> +
> + int devfd;
> + int memfd;
> + char *buf_mem;
> +};
> +
> +struct memory_provider {
> + struct memory_buffer *(*alloc)(size_t size);
> + void (*free)(struct memory_buffer *ctx);
> + void (*memcpy_to_device)(struct memory_buffer *dst, size_t off,
> + void *src, int n);
AFAICT all this code in this api and its implementation is unused. I'm
guessing this is for the future TX path testing?
If you get the chance to slice these changes out of this patch and
punting them for when the TX changes are ready, please do. Just to
minimize dead code until TX path is added, but since these are tests,
dead code is not a huge deal.
> + void (*memcpy_from_device)(void *dst, struct memory_buffer *src,
> + size_t off, int n);
> +};
> +
> +static struct memory_buffer *udmabuf_alloc(size_t size)
> {
> - unsigned char *p = ptr;
> - int i;
> + struct udmabuf_create create;
> + struct memory_buffer *ctx;
> + int ret;
>
> - for (i = 0; i < size; i++)
> - printf("%02hhX ", p[i]);
> - printf("\n");
> + ctx = malloc(sizeof(*ctx));
> + if (!ctx)
> + error(1, ENOMEM, "malloc failed");
> +
> + ctx->size = size;
> +
> + ctx->devfd = open("/dev/udmabuf", O_RDWR);
> + if (ctx->devfd < 0)
> + error(1, errno,
> + "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> + TEST_PREFIX);
> +
> + ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> + if (ctx->memfd < 0)
> + error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> +
> + ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> + if (ret < 0)
> + error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> +
> + ret = ftruncate(ctx->memfd, size);
> + if (ret == -1)
> + error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> +
> + memset(&create, 0, sizeof(create));
> +
> + create.memfd = ctx->memfd;
> + create.offset = 0;
> + create.size = size;
> + ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);
> + if (ctx->fd < 0)
> + error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> +
> + ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> + ctx->fd, 0);
> + if (ctx->buf_mem == MAP_FAILED)
> + error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
> +
> + return ctx;
> +}
> +
> +static void udmabuf_free(struct memory_buffer *ctx)
> +{
> + munmap(ctx->buf_mem, ctx->size);
> + close(ctx->fd);
> + close(ctx->memfd);
> + close(ctx->devfd);
> + free(ctx);
> +}
> +
> +static void udmabuf_memcpy_to_device(struct memory_buffer *dst, size_t off,
> + void *src, int n)
> +{
> + struct dma_buf_sync sync = {};
> +
> + sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE;
> + ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +
> + memcpy(dst->buf_mem + off, src, n);
> +
> + sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE;
> + ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +}
> +
> +static void udmabuf_memcpy_from_device(void *dst, struct memory_buffer *src,
> + size_t off, int n)
> +{
> + struct dma_buf_sync sync = {};
> +
> + sync.flags = DMA_BUF_SYNC_START;
> + ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +
> + memcpy(dst, src->buf_mem + off, n);
> +
> + sync.flags = DMA_BUF_SYNC_END;
> + ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
> }
>
> -void print_nonzero_bytes(void *ptr, size_t size)
> +static struct memory_provider udmabuf_memory_provider = {
> + .alloc = udmabuf_alloc,
> + .free = udmabuf_free,
> + .memcpy_to_device = udmabuf_memcpy_to_device,
> + .memcpy_from_device = udmabuf_memcpy_from_device,
> +};
> +
> +static struct memory_provider *provider = &udmabuf_memory_provider;
> +
> +static void print_nonzero_bytes(void *ptr, size_t size)
> {
> unsigned char *p = ptr;
> unsigned int i;
> @@ -201,42 +302,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
> return -1;
> }
>
> -static void create_udmabuf(int *devfd, int *memfd, int *buf, size_t dmabuf_size)
> -{
> - struct udmabuf_create create;
> - int ret;
> -
> - *devfd = open("/dev/udmabuf", O_RDWR);
> - if (*devfd < 0) {
> - error(70, 0,
> - "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> - TEST_PREFIX);
> - }
> -
> - *memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> - if (*memfd < 0)
> - error(70, 0, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> -
> - /* Required for udmabuf */
> - ret = fcntl(*memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> - if (ret < 0)
> - error(73, 0, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> -
> - ret = ftruncate(*memfd, dmabuf_size);
> - if (ret == -1)
> - error(74, 0, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> -
> - memset(&create, 0, sizeof(create));
> -
> - create.memfd = *memfd;
> - create.offset = 0;
> - create.size = dmabuf_size;
> - *buf = ioctl(*devfd, UDMABUF_CREATE, &create);
> - if (*buf < 0)
> - error(75, 0, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> -}
> -
> -int do_server(void)
> +int do_server(struct memory_buffer *mem)
> {
> char ctrl_data[sizeof(int) * 20000];
> struct netdev_queue_id *queues;
> @@ -244,23 +310,18 @@ int do_server(void)
> struct sockaddr_in client_addr;
> struct sockaddr_in server_sin;
> size_t page_aligned_frags = 0;
> - int devfd, memfd, buf, ret;
> size_t total_received = 0;
> socklen_t client_addr_len;
> bool is_devmem = false;
> - char *buf_mem = NULL;
> + char *tmp_mem = NULL;
> struct ynl_sock *ys;
> - size_t dmabuf_size;
> char iobuf[819200];
> char buffer[256];
> int socket_fd;
> int client_fd;
> size_t i = 0;
> int opt = 1;
> -
> - dmabuf_size = getpagesize() * NUM_PAGES;
> -
> - create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> + int ret;
>
> if (reset_flow_steering())
> error(1, 0, "Failed to reset flow steering\n");
> @@ -284,13 +345,12 @@ int do_server(void)
> queues[i].id = start_queue + i;
> }
>
> - if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
> + if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
> error(1, 0, "Failed to bind\n");
>
> - buf_mem = mmap(NULL, dmabuf_size, PROT_READ | PROT_WRITE, MAP_SHARED,
> - buf, 0);
> - if (buf_mem == MAP_FAILED)
> - error(1, 0, "mmap()");
> + tmp_mem = malloc(mem->size);
> + if (!tmp_mem)
> + error(1, ENOMEM, "malloc failed");
>
> server_sin.sin_family = AF_INET;
> server_sin.sin_port = htons(atoi(port));
> @@ -341,7 +401,6 @@ int do_server(void)
> struct iovec iov = { .iov_base = iobuf,
> .iov_len = sizeof(iobuf) };
> struct dmabuf_cmsg *dmabuf_cmsg = NULL;
> - struct dma_buf_sync sync = { 0 };
> struct cmsghdr *cm = NULL;
> struct msghdr msg = { 0 };
> struct dmabuf_token token;
> @@ -410,22 +469,17 @@ int do_server(void)
> else
> page_aligned_frags++;
>
> - sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
> - ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> + provider->memcpy_from_device(tmp_mem, mem,
> + dmabuf_cmsg->frag_offset,
> + dmabuf_cmsg->frag_size);
>
> if (do_validation)
> validate_buffer(
> - ((unsigned char *)buf_mem) +
> + ((unsigned char *)tmp_mem) +
> dmabuf_cmsg->frag_offset,
> dmabuf_cmsg->frag_size);
> else
> - print_nonzero_bytes(
> - ((unsigned char *)buf_mem) +
> - dmabuf_cmsg->frag_offset,
> - dmabuf_cmsg->frag_size);
> -
> - sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
> - ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> + print_nonzero_bytes(tmp_mem, dmabuf_cmsg->frag_size);
>
> ret = setsockopt(client_fd, SOL_SOCKET,
> SO_DEVMEM_DONTNEED, &token,
> @@ -450,12 +504,9 @@ int do_server(void)
>
> cleanup:
>
> - munmap(buf_mem, dmabuf_size);
> + free(tmp_mem);
> close(client_fd);
> close(socket_fd);
> - close(buf);
> - close(memfd);
> - close(devfd);
> ynl_sock_destroy(ys);
>
> return 0;
> @@ -464,14 +515,11 @@ int do_server(void)
> void run_devmem_tests(void)
> {
> struct netdev_queue_id *queues;
> - int devfd, memfd, buf;
> + struct memory_buffer *mem;
> struct ynl_sock *ys;
> - size_t dmabuf_size;
> size_t i = 0;
>
> - dmabuf_size = getpagesize() * NUM_PAGES;
> -
> - create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> + mem = provider->alloc(getpagesize() * NUM_PAGES);
>
There is some weirdness here where run_devmem_tests() allocates its
own provider, but do_server() uses a provider allocated in main(). Any
reason these are not symmetric? I would marginally prefer do_server()
to allocate its own provider just like run_devmem_tests(), or at least
make them both match, if possible.
--
Thanks,
Mina
Powered by blists - more mailing lists