* [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly
2018-10-01 14:31 [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Jann Horn
@ 2018-10-01 14:31 ` Jann Horn
2018-10-01 15:57 ` Kees Cook
2018-10-04 7:22 ` Michal Hocko
2018-10-01 14:31 ` [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size Jann Horn
` (2 subsequent siblings)
3 siblings, 2 replies; 10+ messages in thread
From: Jann Horn @ 2018-10-01 14:31 UTC (permalink / raw)
To: Andrew Morton, linux-mm, jannh
Cc: Davidlohr Bueso, Oleg Nesterov, Linus Torvalds, Christoph Lameter,
Roman Gushchin, Kemi Wang, Kees Cook, Andy Lutomirski,
Ingo Molnar
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: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
mm/vmstat.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4cea7b8f519d..7878da76abf2 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly
2018-10-01 14:31 ` [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly Jann Horn
@ 2018-10-01 15:57 ` Kees Cook
2018-10-04 7:22 ` Michal Hocko
1 sibling, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-10-01 15:57 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, Linux-MM, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Andy Lutomirski, Ingo Molnar
On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/vmstat.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4cea7b8f519d..7878da76abf2 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly
2018-10-01 14:31 ` [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly Jann Horn
2018-10-01 15:57 ` Kees Cook
@ 2018-10-04 7:22 ` Michal Hocko
1 sibling, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2018-10-04 7:22 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-mm, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Kees Cook, Andy Lutomirski, Ingo Molnar
On Mon 01-10-18 16:31:37, Jann Horn wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/vmstat.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4cea7b8f519d..7878da76abf2 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size
2018-10-01 14:31 [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Jann Horn
2018-10-01 14:31 ` [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly Jann Horn
@ 2018-10-01 14:31 ` Jann Horn
2018-10-01 15:57 ` Kees Cook
` (2 more replies)
2018-10-01 15:56 ` [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Kees Cook
2018-10-04 7:19 ` Michal Hocko
3 siblings, 3 replies; 10+ messages in thread
From: Jann Horn @ 2018-10-01 14:31 UTC (permalink / raw)
To: Andrew Morton, linux-mm, jannh
Cc: Davidlohr Bueso, Oleg Nesterov, Linus Torvalds, Christoph Lameter,
Roman Gushchin, Kemi Wang, Kees Cook, Andy Lutomirski,
Ingo Molnar
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 <jannh@google.com>
---
mm/vmstat.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 7878da76abf2..b678c607e490 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size
2018-10-01 14:31 ` [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size Jann Horn
@ 2018-10-01 15:57 ` Kees Cook
2018-10-04 7:23 ` Michal Hocko
2018-10-04 16:34 ` Roman Gushchin
2 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-10-01 15:57 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, Linux-MM, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Andy Lutomirski, Ingo Molnar
On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> 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 <jannh@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/vmstat.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 7878da76abf2..b678c607e490 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size
2018-10-01 14:31 ` [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size Jann Horn
2018-10-01 15:57 ` Kees Cook
@ 2018-10-04 7:23 ` Michal Hocko
2018-10-04 16:34 ` Roman Gushchin
2 siblings, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2018-10-04 7:23 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-mm, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Kees Cook, Andy Lutomirski, Ingo Molnar
On Mon 01-10-18 16:31:38, Jann Horn wrote:
> 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 <jannh@google.com>
We should have done that looong ago. Thanks!
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/vmstat.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 7878da76abf2..b678c607e490 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size
2018-10-01 14:31 ` [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size Jann Horn
2018-10-01 15:57 ` Kees Cook
2018-10-04 7:23 ` Michal Hocko
@ 2018-10-04 16:34 ` Roman Gushchin
2 siblings, 0 replies; 10+ messages in thread
From: Roman Gushchin @ 2018-10-04 16:34 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-mm@kvack.org, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Kemi Wang, Kees Cook,
Andy Lutomirski, Ingo Molnar
On Mon, Oct 01, 2018 at 04:31:38PM +0200, Jann Horn wrote:
> 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 <jannh@google.com>
> ---
> 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 <guro@fb.com>
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text
2018-10-01 14:31 [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Jann Horn
2018-10-01 14:31 ` [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly Jann Horn
2018-10-01 14:31 ` [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size Jann Horn
@ 2018-10-01 15:56 ` Kees Cook
2018-10-04 7:19 ` Michal Hocko
3 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-10-01 15:56 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, Linux-MM, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Andy Lutomirski, Ingo Molnar
On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/vmstat.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 8ba0870ecddd..4cea7b8f519d 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text
2018-10-01 14:31 [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Jann Horn
` (2 preceding siblings ...)
2018-10-01 15:56 ` [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text Kees Cook
@ 2018-10-04 7:19 ` Michal Hocko
3 siblings, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2018-10-04 7:19 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-mm, Davidlohr Bueso, Oleg Nesterov,
Linus Torvalds, Christoph Lameter, Roman Gushchin, Kemi Wang,
Kees Cook, Andy Lutomirski, Ingo Molnar
On Mon 01-10-18 16:31:36, Jann Horn wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/vmstat.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 8ba0870ecddd..4cea7b8f519d 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -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
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread