Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Low] CONFIG_UDMABUF_SIZE_LIMIT_MBYTES lacks a dependency on CONFIG_UDMABUF. - [Low] Missing range constraint on UDMABUF_SIZE_LIMIT_MBYTES allows negative values, bypassing size validation. Pre-existing issues: - [High] Concurrent calls to DMA_BUF_IOCTL_SYNC trigger a race condition in begin_cpu_udmabuf, leading to a memory leak. -- --- Patch [1]: [RFC PATCH 1/2] dma-buf/udmabuf: Introduce CONFIG_UDMABUF_SIZE_LIMIT_MBYTES --- commit 258354f822e15700d93d45826a3e51b308dd35b8 Author: Robert Mader <[email protected]> dma-buf/udmabuf: Introduce CONFIG_UDMABUF_SIZE_LIMIT_MBYTES This patch introduces a new Kconfig option, CONFIG_UDMABUF_SIZE_LIMIT_MBYTES, to allow distributions to configure the maximum size limit for udmabufs at compile time. The limit defaults to 64MB and was previously only configurable via the kernel command line. > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index 7efc0f0d07126..35f0779cdc801 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -40,6 +40,12 @@ config UDMABUF > A driver to let userspace turn memfd regions into dma-bufs. > Qemu can use this to create host dmabufs for guest framebuffers. > > +config UDMABUF_SIZE_LIMIT_MBYTES > + int "Size limit in Mega Bytes" > + default 64 [Severity: Low] Should this config have a 'depends on UDMABUF' directive? Without it, users might be prompted to configure the size limit even when the UDMABUF driver itself is disabled, polluting the global config. [Severity: Low] Could a user configure a negative value here and bypass the size limits? If this is set to a negative value, it appears it will wrap around to a massive value when cast to u64 in udmabuf_create(): drivers/dma-buf/udmabuf.c:udmabuf_create() { ... pglimit = ((u64)size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; ... } Would it be safer to add a range constraint to the Kconfig definition? > + help > + Maximum size of a udmabuf, in megabytes. Default is 64. > + > config DMABUF_DEBUG > bool "DMA-BUF debug checks" > depends on DMA_SHARED_BUFFER [ ... ] > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index bced421c0d658..a831533263621 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -20,7 +20,11 @@ static int list_limit = 1024; > module_param(list_limit, int, 0644); > MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is > 1024."); > > +#ifdef CONFIG_UDMABUF_SIZE_LIMIT_MBYTES > +static int size_limit_mb = CONFIG_UDMABUF_SIZE_LIMIT_MBYTES; > +#else > static int size_limit_mb = 64; > +#endif > module_param(size_limit_mb, int, 0644); > MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes. Default > is 64."); > [Severity: High] This is a pre-existing issue, but can concurrent calls to DMA_BUF_IOCTL_SYNC cause a memory leak? Looking at begin_cpu_udmabuf(), the scatter-gather table pointer is checked and assigned without locks: drivers/dma-buf/udmabuf.c:begin_cpu_udmabuf() { ... if (!ubuf->sg) { ubuf->sg = get_sg_table(dev, buf, direction); ... } If multiple threads invoke the ioctl simultaneously, could they bypass the check, allocate multiple tables, and overwrite ubuf->sg, permanently leaking the previously allocated tables and mappings? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
