mm/vmstat: fix outdated vmstat_text

PATCH In review Jann Horn 4 participants 10 messages Oct 1, 2018 – Oct 4, 2018

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Jann Horn

author

2018-10-01 14:31 UTC #0 lore raw

PATCH v2 1/3 mm/vmstat: fix outdated vmstat_text

commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely") removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the corresponding entry in vmstat_text. This causes an out-of-bounds access in vmstat_show().

Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is probably very rare.

Fixes 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")

Cc [email protected]

Signed-off-by Jann Horn <[email protected]>

---
 mm/vmstat.c | 1 -
 1 file changed, 1 deletion(-)

mm/vmstat.c +0 -1

index 8ba0870ecddd..4cea7b8f519d 100644
@@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_DEBUG_VM_VMACACHE
 	"vmacache_find_calls",
 	"vmacache_find_hits",
-	"vmacache_full_flushes",
 #endif
 #ifdef CONFIG_SWAP
 	"swap_ra",

Signature · 1 line

2.19.0.605.g01d371f741-goog

Jann Horn

author

2018-10-01 14:31 UTC #1 lore raw

PATCH v2 2/3 mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH* counters inside the kernel unconditional to reduce #ifdef soup, but (either to avoid showing dummy zero counters to userspace, or because that code was missed) didn't update the vmstat_array, meaning that all following counters would be shown with incorrect values.

This only affects kernel builds with CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.

Fixes 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")

Cc [email protected]

Signed-off-by Jann Horn <[email protected]>

---
 mm/vmstat.c | 3 +++
 1 file changed, 3 insertions(+)

mm/vmstat.c +3 -0

index 4cea7b8f519d..7878da76abf2 100644
@@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_SMP
 	"nr_tlb_remote_flush",
 	"nr_tlb_remote_flush_received",
+#else
+	"", /* nr_tlb_remote_flush */
+	"", /* nr_tlb_remote_flush_received */
 #endif /* CONFIG_SMP */
 	"nr_tlb_local_flush_all",
 	"nr_tlb_local_flush_one",

Signature · 1 line

2.19.0.605.g01d371f741-goog

Kees Cook

Reviewed-by on v2 2/3

2018-10-01 15:57 UTC #2 lore raw

Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

Quoted 7 lines · Jann Horn

commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH* counters inside the kernel unconditional to reduce #ifdef soup, but (either to avoid showing dummy zero counters to userspace, or because that code was missed) didn't update the vmstat_array, meaning that all following counters would be shown with incorrect values.

Quoted 6 lines

This only affects kernel builds with CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.
Fixes 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")
Cc [email protected]
Signed-off-by Jann Horn <[email protected]>

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 21 lines

---
 mm/vmstat.c | 3 +++
 1 file changed, 3 insertions(+)

mm/vmstat.c +3 -1

index 4cea7b8f519d..7878da76abf2 100644
@@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_SMP
        "nr_tlb_remote_flush",
        "nr_tlb_remote_flush_received",
+#else
+       "", /* nr_tlb_remote_flush */
+       "", /* nr_tlb_remote_flush_received */
 #endif /* CONFIG_SMP */
        "nr_tlb_local_flush_all",
        "nr_tlb_local_flush_one",
--

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Kees Cook
Pixel Security

Michal Hocko

Acked-by on v2 2/3

2018-10-04 07:22 UTC #3 lore raw

Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

In reply to Jann Horn

Quoted 7 lines · Jann Horn

commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH* counters inside the kernel unconditional to reduce #ifdef soup, but (either to avoid showing dummy zero counters to userspace, or because that code was missed) didn't update the vmstat_array, meaning that all following counters would be shown with incorrect values.

Quoted 6 lines

This only affects kernel builds with CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.
Fixes 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")
Cc [email protected]
Signed-off-by Jann Horn <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 20 lines

---
 mm/vmstat.c | 3 +++
 1 file changed, 3 insertions(+)

mm/vmstat.c +3 -0

index 4cea7b8f519d..7878da76abf2 100644
@@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_SMP
 	"nr_tlb_remote_flush",
 	"nr_tlb_remote_flush_received",
+#else
+	"", /* nr_tlb_remote_flush */
+	"", /* nr_tlb_remote_flush_received */
 #endif /* CONFIG_SMP */
 	"nr_tlb_local_flush_all",
 	"nr_tlb_local_flush_one",

Signature · 1 line

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Michal Hocko
SUSE Labs

Jann Horn

author

2018-10-01 14:31 UTC #4 lore raw

PATCH v2 3/3 mm/vmstat: assert that vmstat_text is in sync with stat_items_size

In reply to Jann Horn

As evidenced by the previous two patches, having two gigantic arrays that must manually be kept in sync, including ifdefs, isn't exactly robust. To make it easier to catch such issues in the future, add a BUILD_BUG_ON().

Signed-off-by Jann Horn <[email protected]>

---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

mm/vmstat.c +2 -0

index 7878da76abf2..b678c607e490 100644
@@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 	stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+	BUILD_BUG_ON(stat_items_size !=
+		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
 	v = kmalloc(stat_items_size, GFP_KERNEL);
 	m->private = v;
 	if (!v)

Signature · 1 line

2.19.0.605.g01d371f741-goog

Kees Cook

Reviewed-by on v2 3/3

2018-10-01 15:57 UTC #5 lore raw

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

Quoted 5 lines · Jann Horn

As evidenced by the previous two patches, having two gigantic arrays that must manually be kept in sync, including ifdefs, isn't exactly robust. To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
Signed-off-by Jann Horn <[email protected]>

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 20 lines

---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

mm/vmstat.c +2 -1

index 7878da76abf2..b678c607e490 100644
@@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
        stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+       BUILD_BUG_ON(stat_items_size !=
+                    ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
        v = kmalloc(stat_items_size, GFP_KERNEL);
        m->private = v;
        if (!v)
--

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Kees Cook
Pixel Security

Michal Hocko

Acked-by on v2 3/3

2018-10-04 07:23 UTC #6 lore raw

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

In reply to Jann Horn

Quoted 5 lines · Jann Horn

As evidenced by the previous two patches, having two gigantic arrays that must manually be kept in sync, including ifdefs, isn't exactly robust. To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
Signed-off-by Jann Horn <[email protected]>

We should have done that looong ago. Thanks!

Acked-by Michal Hocko <[email protected]>

Quoted 19 lines

---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

mm/vmstat.c +2 -0

index 7878da76abf2..b678c607e490 100644
@@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 	stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+	BUILD_BUG_ON(stat_items_size !=
+		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
 	v = kmalloc(stat_items_size, GFP_KERNEL);
 	m->private = v;
 	if (!v)

Signature · 1 line

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Michal Hocko
SUSE Labs

Roman Gushchin

Acked-by on v2 3/3

2018-10-04 16:34 UTC #7 lore raw

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

In reply to Jann Horn

Quoted 8 lines · Jann Horn

As evidenced by the previous two patches, having two gigantic arrays that must manually be kept in sync, including ifdefs, isn't exactly robust. To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
Signed-off-by Jann Horn <[email protected]>
---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

I agree with Michal here, we had to do this long time ago.

For patches 1-3:

Acked-by Roman Gushchin <[email protected]>

BTW, don't we want to split this huge array into smaller parts? This will make the code more clear and easier to modify.

Thank you!

Kees Cook

Reviewed-by on v2 1/3

2018-10-01 15:57 UTC #8 lore raw

In reply to Jann Horn

Quoted 11 lines · Jann Horn

commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely") removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the corresponding entry in vmstat_text. This causes an out-of-bounds access in vmstat_show().
Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is probably very rare.
Fixes 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
Cc [email protected]
Signed-off-by Jann Horn <[email protected]>

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 19 lines

---
 mm/vmstat.c | 1 -
 1 file changed, 1 deletion(-)

mm/vmstat.c +0 -2

index 8ba0870ecddd..4cea7b8f519d 100644
@@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_DEBUG_VM_VMACACHE
        "vmacache_find_calls",
        "vmacache_find_hits",
-       "vmacache_full_flushes",
 #endif
 #ifdef CONFIG_SWAP
        "swap_ra",
--

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Kees Cook
Pixel Security

Michal Hocko

Acked-by on v2 1/3

2018-10-04 07:19 UTC #9 lore raw

In reply to Jann Horn

Quoted 11 lines · Jann Horn

commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely") removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the corresponding entry in vmstat_text. This causes an out-of-bounds access in vmstat_show().
Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is probably very rare.
Fixes 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
Cc [email protected]
Signed-off-by Jann Horn <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 18 lines

---
 mm/vmstat.c | 1 -
 1 file changed, 1 deletion(-)

mm/vmstat.c +0 -1

index 8ba0870ecddd..4cea7b8f519d 100644
@@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_DEBUG_VM_VMACACHE
 	"vmacache_find_calls",
 	"vmacache_find_hits",
-	"vmacache_full_flushes",
 #endif
 #ifdef CONFIG_SWAP
 	"swap_ra",

Signature · 1 line

2.19.0.605.g01d371f741-goog

Signature · 2 lines

Michal Hocko
SUSE Labs