| Message ID | 20200505125930.26901-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 027d0c7101f50cf03aeea9eebf484afd4920c8d3 |
| Headers | show |
| Series | arm64: hugetlb: avoid potential NULL dereference | expand |
On Tue, May 05, 2020 at 10:12:02PM +0900, Itaru Kitayama wrote: > The overhead is just negligible in the paths? Sorry, I'm not sure I understand your question. Are you asking if this is likely to affect performance? I don't expect there to be any measureable overhead here. Practically speaking the difference is a CBZ + RET, and other factors will dominate here. Regardless, this is trivial error handling that was missing from the original patch. Thanks, Mark. > On Tue, May 5, 2020 at 21:59 Mark Rutland <mark.rutland@arm.com> wrote: > > > The static analyzer in GCC 10 spotted that in huge_pte_alloc() we may > > pass a NULL pmdp into pte_alloc_map() when pmd_alloc() returns NULL: > > > > | CC arch/arm64/mm/pageattr.o > > | CC arch/arm64/mm/hugetlbpage.o > > | from arch/arm64/mm/hugetlbpage.c:10: > > | arch/arm64/mm/hugetlbpage.c: In function ‘huge_pte_alloc’: > > | ./arch/arm64/include/asm/pgtable-types.h:28:24: warning: dereference of > > NULL ‘pmdp’ [CWE-690] [-Wanalyzer-null-dereference] > > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro > > ‘pmd_val’ > > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro > > ‘pte_alloc_map’ > > | |arch/arm64/mm/hugetlbpage.c:232:10: > > | |./arch/arm64/include/asm/pgtable-types.h:28:24: > > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro > > ‘pmd_val’ > > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro > > ‘pte_alloc_map’ > > > > This can only occur when the kernel cannot allocate a page, and so is > > unlikely to happen in practice before other systems start failing. > > > > We can avoid this by bailing out if pmd_alloc() fails, as we do earlier > > in the function if pud_alloc() fails. > > > > Fixes: 66b3923a1a0f77a5 ("arm64: hugetlb: add support for PTE contiguous > > bit)" > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Reported-by: Kyrill Tkachov <kyrylo.tkachov@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > --- > > arch/arm64/mm/hugetlbpage.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c > > index bbeb6a5a6ba6..0be3355e3499 100644 > > --- a/arch/arm64/mm/hugetlbpage.c > > +++ b/arch/arm64/mm/hugetlbpage.c > > @@ -230,6 +230,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, > > ptep = (pte_t *)pudp; > > } else if (sz == (CONT_PTE_SIZE)) { > > pmdp = pmd_alloc(mm, pudp, addr); > > + if (!pmdp) > > + return NULL; > > > > WARN_ON(addr & (sz - 1)); > > /* > > -- > > 2.11.0 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >
Hi Mark, I looked in the mm and arch/arm64 directories and all the calls to pmd_alloc() are tested whether it returns a NULL or not, so your patch just fixes the hugetlb code to follow the standard. On Tue, May 5, 2020 at 11:31 PM Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, May 05, 2020 at 10:12:02PM +0900, Itaru Kitayama wrote: > > The overhead is just negligible in the paths? > > Sorry, I'm not sure I understand your question. Are you asking if this > is likely to affect performance? > > I don't expect there to be any measureable overhead here. Practically > speaking the difference is a CBZ + RET, and other factors will dominate > here. > > Regardless, this is trivial error handling that was missing from the > original patch. > > Thanks, > Mark. > > > On Tue, May 5, 2020 at 21:59 Mark Rutland <mark.rutland@arm.com> wrote: > > > > > The static analyzer in GCC 10 spotted that in huge_pte_alloc() we may > > > pass a NULL pmdp into pte_alloc_map() when pmd_alloc() returns NULL: > > > > > > | CC arch/arm64/mm/pageattr.o > > > | CC arch/arm64/mm/hugetlbpage.o > > > | from arch/arm64/mm/hugetlbpage.c:10: > > > | arch/arm64/mm/hugetlbpage.c: In function ‘huge_pte_alloc’: > > > | ./arch/arm64/include/asm/pgtable-types.h:28:24: warning: dereference of > > > NULL ‘pmdp’ [CWE-690] [-Wanalyzer-null-dereference] > > > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro > > > ‘pmd_val’ > > > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro > > > ‘pte_alloc_map’ > > > | |arch/arm64/mm/hugetlbpage.c:232:10: > > > | |./arch/arm64/include/asm/pgtable-types.h:28:24: > > > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro > > > ‘pmd_val’ > > > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro > > > ‘pte_alloc_map’ > > > > > > This can only occur when the kernel cannot allocate a page, and so is > > > unlikely to happen in practice before other systems start failing. > > > > > > We can avoid this by bailing out if pmd_alloc() fails, as we do earlier > > > in the function if pud_alloc() fails. > > > > > > Fixes: 66b3923a1a0f77a5 ("arm64: hugetlb: add support for PTE contiguous > > > bit)" > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Reported-by: Kyrill Tkachov <kyrylo.tkachov@arm.com> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > --- > > > arch/arm64/mm/hugetlbpage.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c > > > index bbeb6a5a6ba6..0be3355e3499 100644 > > > --- a/arch/arm64/mm/hugetlbpage.c > > > +++ b/arch/arm64/mm/hugetlbpage.c > > > @@ -230,6 +230,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, > > > ptep = (pte_t *)pudp; > > > } else if (sz == (CONT_PTE_SIZE)) { > > > pmdp = pmd_alloc(mm, pudp, addr); > > > + if (!pmdp) > > > + return NULL; > > > > > > WARN_ON(addr & (sz - 1)); > > > /* > > > -- > > > 2.11.0 > > > > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > >
On Tue, May 05, 2020 at 01:59:30PM +0100, Mark Rutland wrote: > The static analyzer in GCC 10 spotted that in huge_pte_alloc() we may > pass a NULL pmdp into pte_alloc_map() when pmd_alloc() returns NULL: > > | CC arch/arm64/mm/pageattr.o > | CC arch/arm64/mm/hugetlbpage.o > | from arch/arm64/mm/hugetlbpage.c:10: > | arch/arm64/mm/hugetlbpage.c: In function ‘huge_pte_alloc’: > | ./arch/arm64/include/asm/pgtable-types.h:28:24: warning: dereference of NULL ‘pmdp’ [CWE-690] [-Wanalyzer-null-dereference] > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro ‘pmd_val’ > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro ‘pte_alloc_map’ > | |arch/arm64/mm/hugetlbpage.c:232:10: > | |./arch/arm64/include/asm/pgtable-types.h:28:24: > | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro ‘pmd_val’ > | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro ‘pte_alloc_map’ > > This can only occur when the kernel cannot allocate a page, and so is > unlikely to happen in practice before other systems start failing. > > We can avoid this by bailing out if pmd_alloc() fails, as we do earlier > in the function if pud_alloc() fails. > > Fixes: 66b3923a1a0f77a5 ("arm64: hugetlb: add support for PTE contiguous bit)" > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Reported-by: Kyrill Tkachov <kyrylo.tkachov@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> Queued for 5.7-rc5. Thanks.
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c index bbeb6a5a6ba6..0be3355e3499 100644 --- a/arch/arm64/mm/hugetlbpage.c +++ b/arch/arm64/mm/hugetlbpage.c @@ -230,6 +230,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, ptep = (pte_t *)pudp; } else if (sz == (CONT_PTE_SIZE)) { pmdp = pmd_alloc(mm, pudp, addr); + if (!pmdp) + return NULL; WARN_ON(addr & (sz - 1)); /*
The static analyzer in GCC 10 spotted that in huge_pte_alloc() we may pass a NULL pmdp into pte_alloc_map() when pmd_alloc() returns NULL: | CC arch/arm64/mm/pageattr.o | CC arch/arm64/mm/hugetlbpage.o | from arch/arm64/mm/hugetlbpage.c:10: | arch/arm64/mm/hugetlbpage.c: In function ‘huge_pte_alloc’: | ./arch/arm64/include/asm/pgtable-types.h:28:24: warning: dereference of NULL ‘pmdp’ [CWE-690] [-Wanalyzer-null-dereference] | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro ‘pmd_val’ | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro ‘pte_alloc_map’ | |arch/arm64/mm/hugetlbpage.c:232:10: | |./arch/arm64/include/asm/pgtable-types.h:28:24: | ./arch/arm64/include/asm/pgtable.h:436:26: note: in expansion of macro ‘pmd_val’ | arch/arm64/mm/hugetlbpage.c:242:10: note: in expansion of macro ‘pte_alloc_map’ This can only occur when the kernel cannot allocate a page, and so is unlikely to happen in practice before other systems start failing. We can avoid this by bailing out if pmd_alloc() fails, as we do earlier in the function if pud_alloc() fails. Fixes: 66b3923a1a0f77a5 ("arm64: hugetlb: add support for PTE contiguous bit)" Signed-off-by: Mark Rutland <mark.rutland@arm.com> Reported-by: Kyrill Tkachov <kyrylo.tkachov@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/mm/hugetlbpage.c | 2 ++ 1 file changed, 2 insertions(+)