linux-kernel - Re: [PATCH] mm/hugetlb: fix kernel NULL pointer dereference when replacing free hugetlb folios

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87521d93-cc03-480d-a2ef-3ef8c84481c9@redhat.com>
Date: Mon, 26 May 2025 14:59:54 +0200
From: David Hildenbrand <david@...hat.com>
To: Ge Yang <yangge1116@....com>, akpm@...ux-foundation.org
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org,
 21cnbao@...il.com, baolin.wang@...ux.alibaba.com, muchun.song@...ux.dev,
 osalvador@...e.de, liuzixing@...on.cn
Subject: Re: [PATCH] mm/hugetlb: fix kernel NULL pointer dereference when
 replacing free hugetlb folios

On 26.05.25 14:57, Ge Yang wrote:
> 
> 
> 在 2025/5/26 20:41, David Hildenbrand 写道:
>> On 22.05.25 05:22, yangge1116@....com wrote:
>>> From: Ge Yang <yangge1116@....com>
>>>
>>> A kernel crash was observed when replacing free hugetlb folios:
>>>
>>> BUG: kernel NULL pointer dereference, address: 0000000000000028
>>> PGD 0 P4D 0
>>> Oops: Oops: 0000 [#1] SMP NOPTI
>>> CPU: 28 UID: 0 PID: 29639 Comm: test_cma.sh Tainted 6.15.0-rc6-zp #41
>>> PREEMPT(voluntary)
>>> RIP: 0010:alloc_and_dissolve_hugetlb_folio+0x1d/0x1f0
>>> RSP: 0018:ffffc9000b30fa90 EFLAGS: 00010286
>>> RAX: 0000000000000000 RBX: 0000000000342cca RCX: ffffea0043000000
>>> RDX: ffffc9000b30fb08 RSI: ffffea0043000000 RDI: 0000000000000000
>>> RBP: ffffc9000b30fb20 R08: 0000000000001000 R09: 0000000000000000
>>> R10: ffff88886f92eb00 R11: 0000000000000000 R12: ffffea0043000000
>>> R13: 0000000000000000 R14: 00000000010c0200 R15: 0000000000000004
>>> FS:  00007fcda5f14740(0000) GS:ffff8888ec1d8000(0000)
>>> knlGS:0000000000000000
>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 0000000000000028 CR3: 0000000391402000 CR4: 0000000000350ef0
>>> Call Trace:
>>> <TASK>
>>>    replace_free_hugepage_folios+0xb6/0x100
>>>    alloc_contig_range_noprof+0x18a/0x590
>>>    ? srso_return_thunk+0x5/0x5f
>>>    ? down_read+0x12/0xa0
>>>    ? srso_return_thunk+0x5/0x5f
>>>    cma_range_alloc.constprop.0+0x131/0x290
>>>    __cma_alloc+0xcf/0x2c0
>>>    cma_alloc_write+0x43/0xb0
>>>    simple_attr_write_xsigned.constprop.0.isra.0+0xb2/0x110
>>>    debugfs_attr_write+0x46/0x70
>>>    full_proxy_write+0x62/0xa0
>>>    vfs_write+0xf8/0x420
>>>    ? srso_return_thunk+0x5/0x5f
>>>    ? filp_flush+0x86/0xa0
>>>    ? srso_return_thunk+0x5/0x5f
>>>    ? filp_close+0x1f/0x30
>>>    ? srso_return_thunk+0x5/0x5f
>>>    ? do_dup2+0xaf/0x160
>>>    ? srso_return_thunk+0x5/0x5f
>>>    ksys_write+0x65/0xe0
>>>    do_syscall_64+0x64/0x170
>>>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>
>>> There is a potential race between __update_and_free_hugetlb_folio()
>>> and replace_free_hugepage_folios():
>>>
>>> CPU1                              CPU2
>>> __update_and_free_hugetlb_folio   replace_free_hugepage_folios
>>>                                       folio_test_hugetlb(folio)
>>>                                       -- It's still hugetlb folio.
>>>
>>>     __folio_clear_hugetlb(folio)
>>>     hugetlb_free_folio(folio)
>>>                                       h = folio_hstate(folio)
>>>                                       -- Here, h is NULL pointer
>>>
>>> When the above race condition occurs, folio_hstate(folio) returns
>>> NULL, and subsequent access to this NULL pointer will cause the
>>> system to crash. To resolve this issue, execute folio_hstate(folio)
>>> under the protection of the hugetlb_lock lock, ensuring that
>>> folio_hstate(folio) does not return NULL.
>>>
>>> Fixes: 04f13d241b8b ("mm: replace free hugepage folios after migration")
>>> Signed-off-by: Ge Yang <yangge1116@....com>
>>> Cc: <stable@...r.kernel.org>
>>> ---
>>>    mm/hugetlb.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 3d3ca6b..6c2e007 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -2924,12 +2924,20 @@ int replace_free_hugepage_folios(unsigned long
>>> start_pfn, unsigned long end_pfn)
>>>        while (start_pfn < end_pfn) {
>>>            folio = pfn_folio(start_pfn);
>>> +
>>> +        /*
>>> +         * The folio might have been dissolved from under our feet,
>>> so make sure
>>> +         * to carefully check the state under the lock.
>>> +         */
>>> +        spin_lock_irq(&hugetlb_lock);
>>>            if (folio_test_hugetlb(folio)) {
>>>                h = folio_hstate(folio);
>>>            } else {
>>> +            spin_unlock_irq(&hugetlb_lock);
>>>                start_pfn++;
>>>                continue;
>>>            }
>>> +        spin_unlock_irq(&hugetlb_lock);
>>
>> As mentioned elsewhere, this will grab the hugetlb_lock for each and
>> every pfn in the range if there are no hugetlb folios (common case).
>>
>> That should certainly *not* be done.
>>
>> In case we see !folio_test_hugetlb(), we should just move on.
>>
> 
> The main reason for acquiring the hugetlb_lock here is to obtain a valid
> hstate, as the alloc_and_dissolve_hugetlb_folio() function requires
> hstate as a parameter. This approach is indeed not performance-friendly.
> However, in the patch available at
> https://lore.kernel.org/lkml/1747987559-23082-1-git-send-email-yangge1116@126.com/,
> all these operations will be removed.

No, you still take locks on anything that is !folio_ref_count(folio).

We should really have an early folio_test_hugetlb(folio) check.

hugetlb is on most systems out there the absolute *corner case*.

-- 
Cheers,

David / dhildenb