[PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

split_contpmd() modifies the pmd entries in-place by clearing the CONT
bit, but the local 'pmd' variable still holds the old snapshot with CONT
set. The subsequent split_pmd() call uses this stale value to derive the
pgprot for the new PTE entries via pmd_pgprot(), causing the resulting
PTEs to be populated with incorrect protection bits.

Fix this by re-reading the pmd from memory after split_contpmd() returns
in both call sites: split_kernel_leaf_mapping_locked() and
split_to_ptes_pmd_entry().

Fixes: a166563e7ec3 ("arm64: mm: support large block mapping when rodata=full")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 arch/arm64/mm/mmu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 12e862c..e510336 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -746,8 +746,10 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
 	if (!pmd_present(pmd))
 		goto out;
 	if (pmd_leaf(pmd)) {
-		if (pmd_cont(pmd))
+		if (pmd_cont(pmd)) {
 			split_contpmd(pmdp);
+			pmd = pmdp_get(pmdp);
+		}
 		/*
 		 * PMD: If addr is PMD aligned then addr already describes a
 		 * leaf boundary. Otherwise, split to contpte.
@@ -891,8 +893,10 @@ static int split_to_ptes_pmd_entry(pmd_t *pmdp, unsigned long addr,
 	int ret = 0;
 
 	if (pmd_leaf(pmd)) {
-		if (pmd_cont(pmd))
+		if (pmd_cont(pmd)) {
 			split_contpmd(pmdp);
+			pmd = pmdp_get(pmdp);
+		}
 		ret = split_pmd(pmdp, pmd, gfp, false);
 
 		/*
-- 
2.9.4

Re: [PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[Dev Jain]

On 25/06/26 5:09 pm, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> split_contpmd() modifies the pmd entries in-place by clearing the CONT
> bit, but the local 'pmd' variable still holds the old snapshot with CONT
> set. The subsequent split_pmd() call uses this stale value to derive the
> pgprot for the new PTE entries via pmd_pgprot(), causing the resulting
> PTEs to be populated with incorrect protection bits.

Since the block was CONTPMD, it means the pgprot was uniform on that block,
so after splitting, it should be safe to derive the pgprot from individual pmd's
right?

> 
> Fix this by re-reading the pmd from memory after split_contpmd() returns
> in both call sites: split_kernel_leaf_mapping_locked() and
> split_to_ptes_pmd_entry().
> 
> Fixes: a166563e7ec3 ("arm64: mm: support large block mapping when rodata=full")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/arm64/mm/mmu.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 12e862c..e510336 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -746,8 +746,10 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
>  	if (!pmd_present(pmd))
>  		goto out;
>  	if (pmd_leaf(pmd)) {
> -		if (pmd_cont(pmd))
> +		if (pmd_cont(pmd)) {
>  			split_contpmd(pmdp);
> +			pmd = pmdp_get(pmdp);
> +		}
>  		/*
>  		 * PMD: If addr is PMD aligned then addr already describes a
>  		 * leaf boundary. Otherwise, split to contpte.
> @@ -891,8 +893,10 @@ static int split_to_ptes_pmd_entry(pmd_t *pmdp, unsigned long addr,
>  	int ret = 0;
>  
>  	if (pmd_leaf(pmd)) {
> -		if (pmd_cont(pmd))
> +		if (pmd_cont(pmd)) {
>  			split_contpmd(pmdp);
> +			pmd = pmdp_get(pmdp);
> +		}
>  		ret = split_pmd(pmdp, pmd, gfp, false);
>  
>  		/*

Re: [PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[David Hildenbrand (Arm)]

On 6/26/26 15:03, Dev Jain wrote:
> 
> 
> On 25/06/26 5:09 pm, lirongqing wrote:
>> From: Li RongQing <lirongqing@baidu.com>
>>
>> split_contpmd() modifies the pmd entries in-place by clearing the CONT
>> bit, but the local 'pmd' variable still holds the old snapshot with CONT
>> set. The subsequent split_pmd() call uses this stale value to derive the
>> pgprot for the new PTE entries via pmd_pgprot(), causing the resulting
>> PTEs to be populated with incorrect protection bits.
> 
> Since the block was CONTPMD, it means the pgprot was uniform on that block,
> so after splitting, it should be safe to derive the pgprot from individual pmd's
> right?

I'm also confused by that, can we get some details why (and how) the cont bit
misguides  pmd_pgprot?

-- 
Cheers,

David

Re: [PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[Yang Shi]

On 6/25/26 4:39 AM, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
>
> split_contpmd() modifies the pmd entries in-place by clearing the CONT
> bit, but the local 'pmd' variable still holds the old snapshot with CONT
> set. The subsequent split_pmd() call uses this stale value to derive the
> pgprot for the new PTE entries via pmd_pgprot(), causing the resulting
> PTEs to be populated with incorrect protection bits.

If I read the code correctly, CONT bit is cleared by split_pmd(), then 
the bit may be set again for PTEs if we want to have cont ptes. So I 
don't see any problem, did I miss something?

Thanks,
Yang

>
> Fix this by re-reading the pmd from memory after split_contpmd() returns
> in both call sites: split_kernel_leaf_mapping_locked() and
> split_to_ptes_pmd_entry().
>
> Fixes: a166563e7ec3 ("arm64: mm: support large block mapping when rodata=full")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>   arch/arm64/mm/mmu.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 12e862c..e510336 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -746,8 +746,10 @@ static int split_kernel_leaf_mapping_locked(unsigned long addr)
>   	if (!pmd_present(pmd))
>   		goto out;
>   	if (pmd_leaf(pmd)) {
> -		if (pmd_cont(pmd))
> +		if (pmd_cont(pmd)) {
>   			split_contpmd(pmdp);
> +			pmd = pmdp_get(pmdp);
> +		}
>   		/*
>   		 * PMD: If addr is PMD aligned then addr already describes a
>   		 * leaf boundary. Otherwise, split to contpte.
> @@ -891,8 +893,10 @@ static int split_to_ptes_pmd_entry(pmd_t *pmdp, unsigned long addr,
>   	int ret = 0;
>   
>   	if (pmd_leaf(pmd)) {
> -		if (pmd_cont(pmd))
> +		if (pmd_cont(pmd)) {
>   			split_contpmd(pmdp);
> +			pmd = pmdp_get(pmdp);
> +		}
>   		ret = split_pmd(pmdp, pmd, gfp, false);
>   
>   		/*

答复: [外部邮件] Re: [PATCH] arm64: mm: refresh stale pmd snapshot after split_contpmd()

[Li,Rongqing]

> 
> 
> 
> On 6/25/26 4:39 AM, lirongqing wrote:
> > From: Li RongQing <lirongqing@baidu.com>
> >
> > split_contpmd() modifies the pmd entries in-place by clearing the CONT
> > bit, but the local 'pmd' variable still holds the old snapshot with
> > CONT set. The subsequent split_pmd() call uses this stale value to
> > derive the pgprot for the new PTE entries via pmd_pgprot(), causing
> > the resulting PTEs to be populated with incorrect protection bits.
> 
> If I read the code correctly, CONT bit is cleared by split_pmd(), then the bit
> may be set again for PTEs if we want to have cont ptes. So I don't see any
> problem, did I miss something?
> 

You are right, there's no functional issue with the current code.
However, I think explicitly re-reading the pmd is the safer and clearer 
approach — it makes the intent obvious (we need the post-modification state) 
rather than relying on the implicit assumption that "CONT bit doesn't affect pgprot."

Thanks


[Li,Rongqing] 


> Thanks,
> Yang
> 
> >
> > Fix this by re-reading the pmd from memory after split_contpmd()
> > returns in both call sites: split_kernel_leaf_mapping_locked() and
> > split_to_ptes_pmd_entry().
> >
> > Fixes: a166563e7ec3 ("arm64: mm: support large block mapping when
> > rodata=full")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >   arch/arm64/mm/mmu.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index
> > 12e862c..e510336 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -746,8 +746,10 @@ static int
> split_kernel_leaf_mapping_locked(unsigned long addr)
> >   	if (!pmd_present(pmd))
> >   		goto out;
> >   	if (pmd_leaf(pmd)) {
> > -		if (pmd_cont(pmd))
> > +		if (pmd_cont(pmd)) {
> >   			split_contpmd(pmdp);
> > +			pmd = pmdp_get(pmdp);
> > +		}
> >   		/*
> >   		 * PMD: If addr is PMD aligned then addr already describes a
> >   		 * leaf boundary. Otherwise, split to contpte.
> > @@ -891,8 +893,10 @@ static int split_to_ptes_pmd_entry(pmd_t *pmdp,
> unsigned long addr,
> >   	int ret = 0;
> >
> >   	if (pmd_leaf(pmd)) {
> > -		if (pmd_cont(pmd))
> > +		if (pmd_cont(pmd)) {
> >   			split_contpmd(pmdp);
> > +			pmd = pmdp_get(pmdp);
> > +		}
> >   		ret = split_pmd(pmdp, pmd, gfp, false);
> >
> >   		/*