| 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: <Zv8VxOJiEWHmcWpj@mini-arch>
Date: Thu, 3 Oct 2024 15:08:04 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, 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 10/03, Mina Almasry wrote:
> 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.
Ah, yes, I'll remove it for now.
> > + 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.
I wanted to keep them separate in case we end up adding more to
the selftest part. For example, not sure what would happen now if we pass
a udmabuf with just one page? Do we need some test for the drivers
to make sure they handle this case?
Powered by blists - more mailing lists