Re: [PATCH v2 2/2] udmabuf: Increase the default list length limit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] udmabuf: order-10 contiguous allocation in udmabuf_ioctl_create_list()
- [Low] udmabuf: missing kernel-parameters.txt update for list_limit parameter
--

commit 70d859f4b270b1a12dc2cb5cc01faf794efe4700
Author: Val Packett <val@invisiblethingslab.com>

udmabuf: Increase the default list length limit

This commit increases the default list_limit module parameter in
drivers/dma-buf/udmabuf.c from 1024 to 131072 to support larger page lists
for fragmented guest memory buffers.

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 6fb69d8399c18..9aec0e3a1e49b 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -16,9 +16,9 @@
>  #include <linux/vmalloc.h>
>  #include <linux/iosys-map.h>
>  
> -static int list_limit = 1024;
> +static int list_limit = 131072;

[Severity: Medium]
With list_limit raised to 131072, udmabuf_ioctl_create_list() in
drivers/dma-buf/udmabuf.c copies the user item array using
memdup_array_user():

	if (head.count > list_limit)
		return -EINVAL;
	list = memdup_array_user((void __user *)(arg + sizeof(head)),
				 head.count, sizeof(struct udmabuf_create_item));
	if (IS_ERR(list))
		return PTR_ERR(list);

	ret = udmabuf_create(filp->private_data, &head, list);
	kfree(list);

which calls memdup_user() in mm/util.c:

	p = kmem_buckets_alloc_track_caller(user_buckets, len, GFP_USER | __GFP_NOWARN);
	if (!p)
		return ERR_PTR(-ENOMEM);

Each struct udmabuf_create_item is 24 bytes, so a head.count of 80,000 to
131,072 entries requests roughly 1.83 MiB to 3 MiB (order 9 to order 10 on
4 KiB page systems).

Because memdup_array_user() -> memdup_user() ->
kmem_buckets_alloc_track_caller() allocates physically contiguous memory
without a vmalloc fallback, could this fail with -ENOMEM or stall in direct
compaction on hosts with fragmented physical memory (or fail unconditionally
when the maximum page allocation order is less than 10)?

Should udmabuf_ioctl_create_list() use vmemdup_array_user() and kvfree() for
list instead?

>  module_param(list_limit, int, 0644);
> -MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 1024.");
> +MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 131072.");

[Severity: Low]
Since list_limit in drivers/dma-buf/udmabuf.c is exposed as a kernel
command-line parameter via module_param(list_limit, int, 0644) (as
udmabuf.list_limit), should Documentation/admin-guide/kernel-parameters.txt
also be updated to document this parameter and its new default?

>  
>  static int size_limit_mb = INT_MAX;
>  module_param(size_limit_mb, int, 0644);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021083933.1983894-1-val@invisiblethingslab.com?part=2