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.
>
> 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);
>
