Commit e4bd6a02 authored by Michal Hocko's avatar Michal Hocko Committed by Linus Torvalds

mm, memcg: fix potential undefined behaviour in page stat accounting

Since commit d7365e78 ("mm: memcontrol: fix missed end-writeback
page accounting") mem_cgroup_end_page_stat consumes locked and flags
variables directly rather than via pointers which might trigger C
undefined behavior as those variables are initialized only in the slow
path of mem_cgroup_begin_page_stat.

Although mem_cgroup_end_page_stat handles parameters correctly and
touches them only when they hold a sensible value it is caller which
loads a potentially uninitialized value which then might allow compiler
to do crazy things.

I haven't seen any warning from gcc and it seems that the current
version (4.9) doesn't exploit this type undefined behavior but Sasha has
reported the following:

  UBSan: Undefined behaviour in mm/rmap.c:1084:2
  load of value 255 is not a valid value for type '_Bool'
  CPU: 4 PID: 8304 Comm: rngd Not tainted 3.18.0-rc2-next-20141029-sasha-00039-g77ed13d-dirty #1427
  Call Trace:
    dump_stack (lib/dump_stack.c:52)
    ubsan_epilogue (lib/ubsan.c:159)
    __ubsan_handle_load_invalid_value (lib/ubsan.c:482)
    page_remove_rmap (mm/rmap.c:1084 mm/rmap.c:1096)
    unmap_page_range (./arch/x86/include/asm/atomic.h:27 include/linux/mm.h:463 mm/memory.c:1146 mm/memory.c:1258 mm/memory.c:1279 mm/memory.c:1303)
    unmap_single_vma (mm/memory.c:1348)
    unmap_vmas (mm/memory.c:1377 (discriminator 3))
    exit_mmap (mm/mmap.c:2837)
    mmput (kernel/fork.c:659)
    do_exit (./arch/x86/include/asm/thread_info.h:168 kernel/exit.c:462 kernel/exit.c:747)
    do_group_exit (include/linux/sched.h:775 kernel/exit.c:873)
    SyS_exit_group (kernel/exit.c:901)
    tracesys_phase2 (arch/x86/kernel/entry_64.S:529)

Fix this by using pointer parameters for both locked and flags and be
more robust for future compiler changes even though the current code is
implemented correctly.
Signed-off-by: default avatarMichal Hocko <mhocko@suse.cz>
Reported-by: default avatarSasha Levin <sasha.levin@oracle.com>
Acked-by: default avatarJohannes Weiner <hannes@cmpxchg.org>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent c164e038
...@@ -141,8 +141,8 @@ static inline bool mem_cgroup_disabled(void) ...@@ -141,8 +141,8 @@ static inline bool mem_cgroup_disabled(void)
struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page, bool *locked, struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page, bool *locked,
unsigned long *flags); unsigned long *flags);
void mem_cgroup_end_page_stat(struct mem_cgroup *memcg, bool locked, void mem_cgroup_end_page_stat(struct mem_cgroup *memcg, bool *locked,
unsigned long flags); unsigned long *flags);
void mem_cgroup_update_page_stat(struct mem_cgroup *memcg, void mem_cgroup_update_page_stat(struct mem_cgroup *memcg,
enum mem_cgroup_stat_index idx, int val); enum mem_cgroup_stat_index idx, int val);
...@@ -297,7 +297,7 @@ static inline struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page, ...@@ -297,7 +297,7 @@ static inline struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page,
} }
static inline void mem_cgroup_end_page_stat(struct mem_cgroup *memcg, static inline void mem_cgroup_end_page_stat(struct mem_cgroup *memcg,
bool locked, unsigned long flags) bool *locked, unsigned long *flags)
{ {
} }
......
...@@ -2053,11 +2053,11 @@ struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page, ...@@ -2053,11 +2053,11 @@ struct mem_cgroup *mem_cgroup_begin_page_stat(struct page *page,
* @locked: value received from mem_cgroup_begin_page_stat() * @locked: value received from mem_cgroup_begin_page_stat()
* @flags: value received from mem_cgroup_begin_page_stat() * @flags: value received from mem_cgroup_begin_page_stat()
*/ */
void mem_cgroup_end_page_stat(struct mem_cgroup *memcg, bool locked, void mem_cgroup_end_page_stat(struct mem_cgroup *memcg, bool *locked,
unsigned long flags) unsigned long *flags)
{ {
if (memcg && locked) if (memcg && *locked)
spin_unlock_irqrestore(&memcg->move_lock, flags); spin_unlock_irqrestore(&memcg->move_lock, *flags);
rcu_read_unlock(); rcu_read_unlock();
} }
......
...@@ -2357,7 +2357,7 @@ int test_clear_page_writeback(struct page *page) ...@@ -2357,7 +2357,7 @@ int test_clear_page_writeback(struct page *page)
dec_zone_page_state(page, NR_WRITEBACK); dec_zone_page_state(page, NR_WRITEBACK);
inc_zone_page_state(page, NR_WRITTEN); inc_zone_page_state(page, NR_WRITTEN);
} }
mem_cgroup_end_page_stat(memcg, locked, memcg_flags); mem_cgroup_end_page_stat(memcg, &locked, &memcg_flags);
return ret; return ret;
} }
...@@ -2399,7 +2399,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write) ...@@ -2399,7 +2399,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
mem_cgroup_inc_page_stat(memcg, MEM_CGROUP_STAT_WRITEBACK); mem_cgroup_inc_page_stat(memcg, MEM_CGROUP_STAT_WRITEBACK);
inc_zone_page_state(page, NR_WRITEBACK); inc_zone_page_state(page, NR_WRITEBACK);
} }
mem_cgroup_end_page_stat(memcg, locked, memcg_flags); mem_cgroup_end_page_stat(memcg, &locked, &memcg_flags);
return ret; return ret;
} }
......
...@@ -1053,7 +1053,7 @@ void page_add_file_rmap(struct page *page) ...@@ -1053,7 +1053,7 @@ void page_add_file_rmap(struct page *page)
__inc_zone_page_state(page, NR_FILE_MAPPED); __inc_zone_page_state(page, NR_FILE_MAPPED);
mem_cgroup_inc_page_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED); mem_cgroup_inc_page_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
} }
mem_cgroup_end_page_stat(memcg, locked, flags); mem_cgroup_end_page_stat(memcg, &locked, &flags);
} }
static void page_remove_file_rmap(struct page *page) static void page_remove_file_rmap(struct page *page)
...@@ -1083,7 +1083,7 @@ static void page_remove_file_rmap(struct page *page) ...@@ -1083,7 +1083,7 @@ static void page_remove_file_rmap(struct page *page)
if (unlikely(PageMlocked(page))) if (unlikely(PageMlocked(page)))
clear_page_mlock(page); clear_page_mlock(page);
out: out:
mem_cgroup_end_page_stat(memcg, locked, flags); mem_cgroup_end_page_stat(memcg, &locked, &flags);
} }
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment