Skip to contentSkip to conversation

arm64: mm: dump: a couple of fixes

PATCH New Mark Rutland 3 participants 8 messages Dec 5, 2014 – Dec 11, 2014 arm64

lore
Share & tools
lore

Mark Rutland

author reviewer
#0 lore raw

cover arm64: mm: dump: a couple of fixes

While using the mm dump code, I spotted a couple of issues; one build warning and one theoretical failure on systems endowed with plenty of RAM (as with ARM [1], though arm64 requires a slightly different resolution). These patches (based on next-20141205), fix those issues.

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308952.html

Mark Rutland (2):
arm64: mm: dump: fix shift warning
arm64: mm: dump: don't skip final region

 arch/arm64/mm/dump.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Signature · 1 line
1.9.1
author reviewer
#1 lore raw

PATCH 1/2 arm64: mm: dump: fix shift warning

When building with 48-bit VAs, it's possible to get the following warning when building the arm64 page table dumping code:

arch/arm64/mm/dump.c: In function ?walk_pgd?:
arch/arm64/mm/dump.c:266:2: warning: right shift count >= width of type
pgd_t *pgd = pgd_offset(mm, 0);
^

As pgd_offset is a macro and the second argument is not cast to any particular type, the zero will be given integer type by the compiler. As pgd_offset passes the pargument to pgd_index, we then try to shift the 32-bit integer by at least 39 bits (for 4k pages).

Elsewhere the pgd_offset is passed a second argument of unsigned long type, so let's do the same here by passing '0UL' rather than '0'.

Cc Kees Cook <[email protected]>
Cc Laura Abbott <[email protected]>
Cc Steve Capper <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/mm/dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/mm/dump.c +1 -1
index bf69601..a546776 100644
@@ -272,7 +272,7 @@ static void walk_pud(struct pg_state *st, pgd_t *pgd, unsigned long start)
 
 static void walk_pgd(struct pg_state *st, struct mm_struct *mm, unsigned long start)
 {
-	pgd_t *pgd = pgd_offset(mm, 0);
+	pgd_t *pgd = pgd_offset(mm, 0UL);
 	unsigned i;
 	unsigned long addr;
 
Signature · 1 line
1.9.1
#2 lore raw

PATCH 1/2 arm64: mm: dump: fix shift warning

Quoted 8 lines · Mark Rutland

When building with 48-bit VAs, it's possible to get the following warning when building the arm64 page table dumping code:

arch/arm64/mm/dump.c: In function ?walk_pgd?:
arch/arm64/mm/dump.c:266:2: warning: right shift count >= width of type
pgd_t *pgd = pgd_offset(mm, 0);
^

Quoted 8 lines

As pgd_offset is a macro and the second argument is not cast to any particular type, the zero will be given integer type by the compiler. As pgd_offset passes the pargument to pgd_index, we then try to shift the 32-bit integer by at least 39 bits (for 4k pages).

Elsewhere the pgd_offset is passed a second argument of unsigned long type, so let's do the same here by passing '0UL' rather than '0'.

Hi Mark,
Thanks, I must not have spotted that warning earlier.

author reviewer
#3 lore raw

PATCH 1/2 arm64: mm: dump: fix shift warning

Quoted 14 lines · Steve Capper
Quoted 13 lines · Mark Rutland

When building with 48-bit VAs, it's possible to get the following warning when building the arm64 page table dumping code:

arch/arm64/mm/dump.c: In function ?walk_pgd?:
arch/arm64/mm/dump.c:266:2: warning: right shift count >= width of type
pgd_t *pgd = pgd_offset(mm, 0);
^

As pgd_offset is a macro and the second argument is not cast to any particular type, the zero will be given integer type by the compiler. As pgd_offset passes the pargument to pgd_index, we then try to shift the 32-bit integer by at least 39 bits (for 4k pages).

Quoted 6 lines
Quoted 3 lines

Elsewhere the pgd_offset is passed a second argument of unsigned long type, so let's do the same here by passing '0UL' rather than '0'.

Hi Mark,
Thanks, I must not have spotted that warning earlier.

Me neither, I'd tested on this configuration but somehow missed the build warning.

Quoted 1 line

Cheers!

Mark.

author reviewer
#4 lore raw

PATCH 2/2 arm64: mm: dump: don't skip final region

In reply to Mark Rutland

If the final page table entry we walk is a valid mapping, the page table dumping code will not log the region this entry is part of, as the final note_page call in ptdump_show will trigger an early return. Luckily this isn't seen on contemporary systems as they typically don't have enough RAM to extend the linear mapping right to the end of the address space.

In note_page, we log a region when we reach its end (i.e. we hit an entry immediately afterwards which has different prot bits or is invalid). The final entry has no subsequent entry, so we will not log this immediately. We try to cater for this with a subsequent call to note_page in ptdump_show, but this returns early as 0 < LOWEST_ADDR, and hence we will skip a valid mapping if it spans to the final entry we note.

Unlike 32-bit ARM, the pgd with the kernel mapping is never shared with user mappings, so we do not need the check to ensure we don't log user page tables. Due to the way addr is constructed in the walk_* functions, it can never be less than LOWEST_ADDR when walking the page tables, so it is not necessary to avoid dereferencing invalid table addresses. The existing checks for st->current_prot and st->marker[1].start_address are sufficient to ensure we will not print and/or dereference garbage when trying to log information.

This patch removes the unnecessary check against LOWEST_ADDR, ensuring we log all regions in the kernel page table, including those which span right to the end of the address space.

Cc Kees Cook <[email protected]>
Cc Laura Abbott <[email protected]>
Cc Steve Capper <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/mm/dump.c | 3 ---
 1 file changed, 3 deletions(-)
arch/arm64/mm/dump.c +0 -3
index a546776..cf33f33 100644
@@ -182,9 +182,6 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 	static const char units[] = "KMGTPE";
 	u64 prot = val & pg_level[level].mask;
 
-	if (addr < LOWEST_ADDR)
-		return;
-
 	if (!st->level) {
 		st->level = level;
 		st->current_prot = prot;
Signature · 1 line
1.9.1
#5 lore raw

PATCH 2/2 arm64: mm: dump: don't skip final region

Quoted 23 lines · Mark Rutland

If the final page table entry we walk is a valid mapping, the page table dumping code will not log the region this entry is part of, as the final note_page call in ptdump_show will trigger an early return. Luckily this isn't seen on contemporary systems as they typically don't have enough RAM to extend the linear mapping right to the end of the address space.

In note_page, we log a region when we reach its end (i.e. we hit an entry immediately afterwards which has different prot bits or is invalid). The final entry has no subsequent entry, so we will not log this immediately. We try to cater for this with a subsequent call to note_page in ptdump_show, but this returns early as 0 < LOWEST_ADDR, and hence we will skip a valid mapping if it spans to the final entry we note.

Unlike 32-bit ARM, the pgd with the kernel mapping is never shared with user mappings, so we do not need the check to ensure we don't log user page tables. Due to the way addr is constructed in the walk_* functions, it can never be less than LOWEST_ADDR when walking the page tables, so it is not necessary to avoid dereferencing invalid table addresses. The existing checks for st->current_prot and st->marker[1].start_address are sufficient to ensure we will not print and/or dereference garbage when trying to log information.

Quoted 4 lines

This patch removes the unnecessary check against LOWEST_ADDR, ensuring we log all regions in the kernel page table, including those which span right to the end of the address space.

Quoted 26 lines
Cc Kees Cook <[email protected]>
Cc Laura Abbott <[email protected]>
Cc Steve Capper <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/mm/dump.c | 3 ---
 1 file changed, 3 deletions(-)
arch/arm64/mm/dump.c +0 -4
index a546776..cf33f33 100644
@@ -182,9 +182,6 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
        static const char units[] = "KMGTPE";
        u64 prot = val & pg_level[level].mask;
 
-       if (addr < LOWEST_ADDR)
-               return;
-
        if (!st->level) {
                st->level = level;
                st->current_prot = prot;
--

1.9.1

#6 lore raw

cover arm64: mm: dump: a couple of fixes

In reply to Mark Rutland
Quoted 7 lines · Mark Rutland

While using the mm dump code, I spotted a couple of issues; one build warning and one theoretical failure on systems endowed with plenty of RAM (as with ARM [1], though arm64 requires a slightly different resolution). These patches (based on next-20141205), fix those issues.

Mark.

Quoted 9 lines

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308952.html

Mark Rutland (2):
arm64: mm: dump: fix shift warning
arm64: mm: dump: don't skip final region

  arch/arm64/mm/dump.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

If they haven't been picked up yet:

Signature · 3 lines
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author reviewer
#7 lore raw

cover arm64: mm: dump: a couple of fixes

Quoted 14 lines · Laura Abbott
Quoted 13 lines · Mark Rutland

While using the mm dump code, I spotted a couple of issues; one build warning and one theoretical failure on systems endowed with plenty of RAM (as with ARM [1], though arm64 requires a slightly different resolution). These patches (based on next-20141205), fix those issues.

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308952.html

Mark Rutland (2):
arm64: mm: dump: fix shift warning
arm64: mm: dump: don't skip final region

Quoted 7 lines
Quoted 3 lines
  arch/arm64/mm/dump.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

If they haven't been picked up yet:

Thanks!

Mark.