mm: vmap: avoid -Wsequence-point warning

Commit Message

Arnd Bergmann Nov. 4, 2021, 1:35 p.m. UTC

From: Arnd Bergmann <[e-mail]>

gcc warns about potentially undefined behavior in an array index:

mm/vmalloc.c: In function 'vmap_pfn_apply':
mm/vmalloc.c:2800:58: error: operation on 'data->idx' may be undefined [-Werror=sequence-point]
 2800 |         *pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot));
      |                                                 ~~~~~~~~~^~
arch/arm64/include/asm/pgtable-types.h:25:37: note: in definition of macro '__pte'
   25 | #define __pte(x)        ((pte_t) { (x) } )
      |                                     ^
arch/arm64/include/asm/pgtable.h:80:15: note: in expansion of macro '__phys_to_pte_val'
   80 |         __pte(__phys_to_pte_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
      |               ^~~~~~~~~~~~~~~~~
mm/vmalloc.c:2800:30: note: in expansion of macro 'pfn_pte'
 2800 |         *pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot));
      |                              ^~~~~~~

This only appeared in one randconfig build so far, and I don't know
what caused it, but moving the index increment out of the expression
at least addresses the warning.

Fixes: 3e9a9e256b1e ("mm: add a vmap_pfn function")
Signed-off-by: Arnd Bergmann <[e-mail]>
---
Not sure if it's worth doing a deeper analysis of how the compiler
thinks this might go wrong, it would appear that it thinks 'pte'
might be an alias for 'data' here, but only in some configurations.

If you want to try reproducing it, the .config that triggered it is
https://pastebin.com/caeKD1Wv
---
 mm/vmalloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Matthew Wilcox Nov. 4, 2021, 1:57 p.m. UTC | #1

On Thu, Nov 04, 2021 at 02:35:40PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[e-mail]>
> 
> gcc warns about potentially undefined behavior in an array index:
> 
> mm/vmalloc.c: In function 'vmap_pfn_apply':
> mm/vmalloc.c:2800:58: error: operation on 'data->idx' may be undefined [-Werror=sequence-point]
>  2800 |         *pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot));
>       |                                                 ~~~~~~~~~^~
> arch/arm64/include/asm/pgtable-types.h:25:37: note: in definition of macro '__pte'
>    25 | #define __pte(x)        ((pte_t) { (x) } )
>       |                                     ^
> arch/arm64/include/asm/pgtable.h:80:15: note: in expansion of macro '__phys_to_pte_val'
>    80 |         __pte(__phys_to_pte_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
>       |               ^~~~~~~~~~~~~~~~~
> mm/vmalloc.c:2800:30: note: in expansion of macro 'pfn_pte'
>  2800 |         *pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot));
>       |                              ^~~~~~~
> 
> This only appeared in one randconfig build so far, and I don't know
> what caused it, but moving the index increment out of the expression
> at least addresses the warning.

Would that randconfig include CONFIG_ARM64_PA_BITS_52?

#define __phys_to_pte_val(phys) (((phys) | ((phys) >> 36)) & PTE_ADDR_MASK)

because that's going to double-increment idx.  Or single increment.
Or whatever else the compiler feels like doing.

Arnd Bergmann Nov. 4, 2021, 2:39 p.m. UTC | #2

On Thu, Nov 4, 2021 at 2:57 PM Matthew Wilcox <[e-mail]> wrote:
> On Thu, Nov 04, 2021 at 02:35:40PM +0100, Arnd Bergmann wrote:
> >
> > This only appeared in one randconfig build so far, and I don't know
> > what caused it, but moving the index increment out of the expression
> > at least addresses the warning.
>
> Would that randconfig include CONFIG_ARM64_PA_BITS_52?
>
> #define __phys_to_pte_val(phys) (((phys) | ((phys) >> 36)) & PTE_ADDR_MASK)
>
> because that's going to double-increment idx.  Or single increment.
> Or whatever else the compiler feels like doing.

Ok, got it. I've got a new patch turning that into an inline function now,
which seems like a more reliable fix. I still don't see why the warning only
showed up now, as both the caller and the definition of __phys_to_pte_val()
are not that new, and I've been testing with gcc-11 for a while now.

       Arnd

Patch

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d2a00ad4e1dd..cdac02aab6b1 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2797,7 +2797,9 @@  static int vmap_pfn_apply(pte_t *pte, unsigned long addr, void *private)
 
 	if (WARN_ON_ONCE(pfn_valid(data->pfns[data->idx])))
 		return -EINVAL;
-	*pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot));
+	*pte = pte_mkspecial(pfn_pte(data->pfns[data->idx], data->prot));
+	data->idx++;
+
 	return 0;
 }