linux-kernel - Re: [PATCH] proc: Avoid costly high-order page allocations when reading proc files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6823186-9267-418c-a676-390be9d4524d@suse.cz>
Date: Wed, 2 Apr 2025 11:25:12 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Yafang Shao <laoar.shao@...il.com>, Harry Yoo <harry.yoo@...cle.com>
Cc: Kees Cook <kees@...nel.org>, joel.granados@...nel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 Josef Bacik <josef@...icpanda.com>, linux-mm@...ck.org,
 Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH] proc: Avoid costly high-order page allocations when
 reading proc files

On 4/2/25 10:42, Yafang Shao wrote:
> On Wed, Apr 2, 2025 at 12:15 PM Harry Yoo <harry.yoo@...cle.com> wrote:
>>
>> On Tue, Apr 01, 2025 at 07:01:04AM -0700, Kees Cook wrote:
>> >
>> >
>> > On April 1, 2025 12:30:46 AM PDT, Yafang Shao <laoar.shao@...il.com> wrote:
>> > >While investigating a kcompactd 100% CPU utilization issue in production, I
>> > >observed frequent costly high-order (order-6) page allocations triggered by
>> > >proc file reads from monitoring tools. This can be reproduced with a simple
>> > >test case:
>> > >
>> > >  fd = open(PROC_FILE, O_RDONLY);
>> > >  size = read(fd, buff, 256KB);
>> > >  close(fd);
>> > >
>> > >Although we should modify the monitoring tools to use smaller buffer sizes,
>> > >we should also enhance the kernel to prevent these expensive high-order
>> > >allocations.
>> > >
>> > >Signed-off-by: Yafang Shao <laoar.shao@...il.com>
>> > >Cc: Josef Bacik <josef@...icpanda.com>
>> > >---
>> > > fs/proc/proc_sysctl.c | 10 +++++++++-
>> > > 1 file changed, 9 insertions(+), 1 deletion(-)
>> > >
>> > >diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> > >index cc9d74a06ff0..c53ba733bda5 100644
>> > >--- a/fs/proc/proc_sysctl.c
>> > >+++ b/fs/proc/proc_sysctl.c
>> > >@@ -581,7 +581,15 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>> > >     error = -ENOMEM;
>> > >     if (count >= KMALLOC_MAX_SIZE)
>> > >             goto out;
>> > >-    kbuf = kvzalloc(count + 1, GFP_KERNEL);
>> > >+
>> > >+    /*
>> > >+     * Use vmalloc if the count is too large to avoid costly high-order page
>> > >+     * allocations.
>> > >+     */
>> > >+    if (count < (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
>> > >+            kbuf = kvzalloc(count + 1, GFP_KERNEL);
>> >
>> > Why not move this check into kvmalloc family?
>>
>> Hmm should this check really be in kvmalloc family?
> 
> Modifying the existing kvmalloc functions risks performance regressions.
> Could we instead introduce a new variant like vkmalloc() (favoring
> vmalloc over kmalloc) or kvmalloc_costless()?

We have gfp flags and kmalloc_gfp_adjust() to moderate how aggressive
kmalloc() is before the vmalloc() fallback. It does e.g.:

                if (!(flags & __GFP_RETRY_MAYFAIL))
                        flags |= __GFP_NORETRY;

However if your problem is kcompactd utilization then the kmalloc() attempt
would have to avoid ___GFP_KSWAPD_RECLAIM to avoid waking up kswapd and then
kcompactd. Should we remove the flag for costly orders? Dunno. Ideally the
deferred compaction mechanism would limit the issue in the first place.

The ad-hoc fixing up of a particular place (/proc files reading) or creating
a new vkmalloc() and then spreading its use as you see other places
triggering the issue seems quite suboptimal to me.

>>
>> I don't think users would expect kvmalloc() to implictly decide on using
>> vmalloc() without trying kmalloc() first, just because it's a high-order
>> allocation.
>>
>