Hi,
On 22.09.26 09:33, Christian König wrote:
On 9/22/26 07:19, Kasireddy, Vivek wrote:
Hi Christian,
Subject: Re: [PATCH v1 2/2] udmabuf: Add mlock accounting for
udmabuf pages/folios
On 8/31/26 08:00, Vivek Kasireddy wrote:
Since udmabuf allows users to pin pages in memory, which is a vital
resource, we need to ensure that unprivileged users cannot abuse
this feature to exhaust system memory. To address this, introduce
mlock accounting for udmabuf pages/folios. This change ensures that
the number of pages/folios pinned by udmabuf is tracked and limited
based on the user's mlock limits.
If an unprivileged user/process exceeds their mlock limit, no pages
are pinned and EPERM is returned to the user. Note that, privileged
users/processes (CAP_IPC_LOCK capable) are exempt from this limit,
allowing them to pin pages without any restriction.
Earlier, there was a 64 MB default limit on the size of udmabuf
allocations, but it was removed in commit 44e9eb5a7621
("dma-buf/udmabuf: Disable the size limit by default") which left
open the possibility that unpriviledged users could pin unbounded
amount of memory. This change makes the limit dynamic, by invoking
user_shm_lock() to check the mlock limit for unprivileged users.
That makes no sense at all and would create a massive problems.
The mlock limit is per process, but udmabuf is represented by a file
descriptor which can move between processes.
So this here can be trivially abused to account the allocation to one
process while accounting the free to another process and overflowing
the mlock housekeeping.
The goal is to have the mlock accounting be tied to the exporters since they
are the ones that pin the pages/folios. As Sashiko identified, this patch
doesn't
quite do that but once fixed, both alloc and free accounting would be attributed
to the exporter. Would this idea not work?
Most likely not, no.
First of all this is trivially circumvent-able, you just need to create a child
process which allocates and exports the memory and then gives the fd back to
the parent and dies.
The result would be that you have accounted to the mlock limit of the child
process (which is now dead) and then a dangling reference to the cred structure
of a dead process. Both are most likely no-gos for this.
Then the second problem is that you often have processes which allocates the
memory on behalves of another process (display servers for example do that).
The fundamental issue is that Linux doesn't have a way to track file
descriptors as they move from process to process and so if your file descriptor
represents more than just a standard file on a file system you run into tons of
trouble with resource management.
Regards,
Christian.
I'd like to quickly chime in on what we IMHO would optimally want (from
a userspace perspective). Some context:
1. Unprivileged users by default don't have access to /dev/udmabuf
2. Since https://github.com/systemd/systemd/pull/33738 systemd grants
uaccess to graphical users - i.e. if I'm not mistaken only the users
currently owning drm master should be able to use udmabuf (or something
similar)
3. When owning drm master one can probably already DDoS the system in
various ways - so probably not really worth to try to hard to defend
against.
4. An exception here would be sandboxed apps - e.g. flatpaks. Such apps
get udabuf access if they also have dri device access
(https://github.com/flatpak/flatpak/pull/6158). Preventing these apps
from pinning unlimited amounts of memory would be great - however I
guess this would require mlock accounting on the level of cgroups. Is
that realistic to have by any means?
Best regards,
Robert
Thanks,
Vivek
What could potentially work is to grab a reference to the cred structure
of the allocating task, but that is also not the nicest thing to do.
Regards,
Christian.
Signed-off-by: Vivek Kasireddy <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: "Christian König" <[email protected]>
Cc: Robert Mader <[email protected]>
Cc: Xaver Hugl <[email protected]>
Fixes: 44e9eb5a7621 ("dma-buf/udmabuf: Disable the size limit by
default")
---
drivers/dma-buf/udmabuf.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 7c4339130eff..e78947f01656 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -203,9 +203,10 @@ static __always_inline int
init_udmabuf(struct udmabuf *ubuf, pgoff_t pgcnt)
return 0;
}
-static __always_inline void deinit_udmabuf(struct udmabuf *ubuf)
+static __always_inline void deinit_udmabuf(struct udmabuf *ubuf,
pgoff_t pgcnt)
{
unpin_all_folios(ubuf);
+ user_shm_unlock(pgcnt << PAGE_SHIFT, current_ucounts());
kvfree(ubuf->pages);
}
@@ -217,7 +218,7 @@ static void release_udmabuf(struct dma_buf
*buf)
if (ubuf->sg)
put_sg_table(dev, ubuf->sg, ubuf->sg_dir);
- deinit_udmabuf(ubuf);
+ deinit_udmabuf(ubuf, ubuf->pagecount);
kfree(ubuf);
}
@@ -385,6 +386,15 @@ static long udmabuf_create(struct miscdevice
*device,
if (!pgcnt)
goto err_noinit;
+ if (!user_shm_lock(pgcnt << PAGE_SHIFT, current_ucounts())) {
+ pr_warn_once("%s: User not permitted to pin more than
%lu KB "
+ "of memory, either increase ulimits -l, or "
+ "obtain CAP_IPC_LOCK capability\n", current-
comm,
+ rlimit(RLIMIT_MEMLOCK) >> 10);
+ ret = -EPERM;
+ goto err_noinit;
+ }
+
ret = init_udmabuf(ubuf, pgcnt);
if (ret)
goto err;
@@ -441,7 +451,7 @@ static long udmabuf_create(struct miscdevice
*device,
return ret;
err:
- deinit_udmabuf(ubuf);
+ deinit_udmabuf(ubuf, pgcnt);
err_noinit:
kfree(ubuf);
kvfree(folios);
