Message ID | 20180929013611.163130-1-jannh@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mm/vmstat: fix outdated vmstat_text | expand |
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
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.
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
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.
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)
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(-)