On 9/22/26 15:40, Robert Mader wrote:
> 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
If we have the normal DRM render node group that would be ok for me, but could
be that others object.
> 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)
No, that is not correct. If I'm not completely mistaken everybody can access
/dev/udmabuf at the moment.
> 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.
Well without cgroups even regular users can trivially cause a local deny of
service on Linux.
Just try the following code:
char buffer[1024];
int fd;
fd = memfd_create("Test", 0);
while (1)
write(fd, buffer, 1024);
The OOM killer will kill every process in the system except for the offending
one. Background is that the OOM killer can't see resources locked up in file
descriptors.
> 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?
I don't think that this will work. Pinning memory is as far as I know not seen
as something problematic.
Regards,
Christian.
>
> 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);
