[PATCH] mm/vmstat: fix outdated vmstat_text

[PATCH] mm/vmstat: fix outdated vmstat_text

[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.

Having two gigantic arrays that must be kept in sync isn't exactly robust.
To make it easier to catch such issues in the future, add a BUILD_BUG_ON().

Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/vmstat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 8ba0870ecddd..db6379a3f8bf 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",
@@ -1661,6 +1660,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

Re: [PATCH] mm/vmstat: fix outdated vmstat_text

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

Hi Jann,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jann-Horn/mm-vmstat-fix-outdated-vmstat_text/20180929-102147
config: i386-randconfig-x005-201838 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/export.h:45:0,
                    from include/linux/linkage.h:7,
                    from include/linux/fs.h:5,
                    from mm/vmstat.c:12:
   mm/vmstat.c: In function 'vmstat_start':
>> include/linux/compiler.h:358:38: error: call to '__compiletime_assert_1664' declared with attribute error: BUILD_BUG_ON failed: stat_items_size != ARRAY_SIZE(vmstat_text) * sizeof(unsigned long)
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:338:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:358:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
     ^~~~~~~~~~~~~~~~
>> mm/vmstat.c:1663:2: note: in expansion of macro 'BUILD_BUG_ON'
     BUILD_BUG_ON(stat_items_size !=
     ^~~~~~~~~~~~

vim +/__compiletime_assert_1664 +358 include/linux/compiler.h

9a8ab1c3 Daniel Santos 2013-02-21  344  
9a8ab1c3 Daniel Santos 2013-02-21  345  #define _compiletime_assert(condition, msg, prefix, suffix) \
9a8ab1c3 Daniel Santos 2013-02-21  346  	__compiletime_assert(condition, msg, prefix, suffix)
9a8ab1c3 Daniel Santos 2013-02-21  347  
9a8ab1c3 Daniel Santos 2013-02-21  348  /**
9a8ab1c3 Daniel Santos 2013-02-21  349   * compiletime_assert - break build and emit msg if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  350   * @condition: a compile-time constant condition to check
9a8ab1c3 Daniel Santos 2013-02-21  351   * @msg:       a message to emit if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  352   *
9a8ab1c3 Daniel Santos 2013-02-21  353   * In tradition of POSIX assert, this macro will break the build if the
9a8ab1c3 Daniel Santos 2013-02-21  354   * supplied condition is *false*, emitting the supplied error message if the
9a8ab1c3 Daniel Santos 2013-02-21  355   * compiler has support to do so.
9a8ab1c3 Daniel Santos 2013-02-21  356   */
9a8ab1c3 Daniel Santos 2013-02-21  357  #define compiletime_assert(condition, msg) \
9a8ab1c3 Daniel Santos 2013-02-21 @358  	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
9a8ab1c3 Daniel Santos 2013-02-21  359  

:::::: The code at line 358 was first introduced by commit
:::::: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG

:::::: TO: Daniel Santos <daniel.santos@pobox.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36100 bytes --]

Re: [PATCH] mm/vmstat: fix outdated vmstat_text

[Jann Horn]

On Sat, Sep 29, 2018 at 5:07 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Jann,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.19-rc5 next-20180928]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Jann-Horn/mm-vmstat-fix-outdated-vmstat_text/20180929-102147
> config: i386-randconfig-x005-201838 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All error/warnings (new ones prefixed by >>):
>
>    In file included from include/linux/export.h:45:0,
>                     from include/linux/linkage.h:7,
>                     from include/linux/fs.h:5,
>                     from mm/vmstat.c:12:
>    mm/vmstat.c: In function 'vmstat_start':
> >> include/linux/compiler.h:358:38: error: call to '__compiletime_assert_1664' declared with attribute error: BUILD_BUG_ON failed: stat_items_size != ARRAY_SIZE(vmstat_text) * sizeof(unsigned long)

Nice. Looks like the 0day test bot indeed found another mismatch:

#ifdef CONFIG_DEBUG_TLBFLUSH
#ifdef CONFIG_SMP
       "nr_tlb_remote_flush",
       "nr_tlb_remote_flush_received",
#endif /* CONFIG_SMP */
       "nr_tlb_local_flush_all",
       "nr_tlb_local_flush_one",
#endif /* CONFIG_DEBUG_TLBFLUSH */

vs

#ifdef CONFIG_DEBUG_TLBFLUSH
               NR_TLB_REMOTE_FLUSH,    /* cpu tried to flush others' tlbs */
               NR_TLB_REMOTE_FLUSH_RECEIVED,/* cpu received ipi for flush */
               NR_TLB_LOCAL_FLUSH_ALL,
               NR_TLB_LOCAL_FLUSH_ONE,
#endif /* CONFIG_DEBUG_TLBFLUSH */

So if you build with CONFIG_VM_EVENT_COUNTERS=y &&
CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n, vmstat output is bogus.

I like having my decisions to add asserts immediately validated by the
0day test bot. ^^

I'll send a v2 with that fixed up.

Re: [PATCH] mm/vmstat: fix outdated vmstat_text

[Michal Hocko]

On Sat 29-09-18 03:36:11, 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.
> 
> Having two gigantic arrays that must be kept in sync isn't exactly robust.
> To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
> 
> Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Those could be two separate patches but anyway
Acked-by: Michal Hocko <mhocko@suse.com>

to both changes. I have burned myself on this in the past as well. Build
bugon would save me a lot of debugging.

> ---
>  mm/vmstat.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 8ba0870ecddd..db6379a3f8bf 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",
> @@ -1661,6 +1660,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

Re: [PATCH] mm/vmstat: fix outdated vmstat_text

[Jann Horn]

On Wed, Oct 3, 2018 at 6:29 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Sat 29-09-18 03:36:11, 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.
> >
> > Having two gigantic arrays that must be kept in sync isn't exactly robust.
> > To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
> >
> > Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Those could be two separate patches but anyway
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> to both changes. I have burned myself on this in the past as well. Build
> bugon would save me a lot of debugging.

I actually sent a v2 that splits this into two patches, and adds
another fix for nr_tlb_remote_flush and nr_tlb_remote_flush_received
for systems with CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y
&& CONFIG_SMP=n. akpm has already added the v2 patches to the mm tree.