| Message ID | 20200804183943.1244828-1-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, dump_page: do not crash with bad compound_page() | expand |
On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote: > +static inline int __compound_mapcount(struct page *page) > +{ > + page = compound_head(page); > + return atomic_read(compound_mapcount_ptr(page)) + 1; > +} I'd suggest instead: static inline int head_mapcount(struct page *head) { return atomic_read(compound_mapcount_ptr(head)) + 1; } > +static inline int dump_page_compound_mapcount(struct page *page) > +{ > + if (WARN_ON_ONCE(!PageCompound(page))) > + return 0; > + return __compound_mapcount(page); > } And just dropping this ... it shouldn't be used outside mm/debug.c anyway. Thinking about it, we'll get the hint later that this is not to be trusted when the flags from the page do not include 'head'.
On 8/4/20 11:48 AM, Matthew Wilcox wrote: > On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote: >> +static inline int __compound_mapcount(struct page *page) >> +{ >> + page = compound_head(page); >> + return atomic_read(compound_mapcount_ptr(page)) + 1; >> +} > > I'd suggest instead: > > static inline int head_mapcount(struct page *head) > { > return atomic_read(compound_mapcount_ptr(head)) + 1; > } > >> +static inline int dump_page_compound_mapcount(struct page *page) >> +{ >> + if (WARN_ON_ONCE(!PageCompound(page))) >> + return 0; >> + return __compound_mapcount(page); >> } > > And just dropping this ... it shouldn't be used outside mm/debug.c anyway. Yes, that's cleaner. I'll send a v2 that is not a reply, so that Andrew can spot it more easily. OK to add your "Suggested-by:" to that? > > Thinking about it, we'll get the hint later that this is not to be trusted > when the flags from the page do not include 'head'. > Good point. I think this will work out pretty well, then. thanks,
On Tue, Aug 04, 2020 at 12:17:34PM -0700, John Hubbard wrote: > On 8/4/20 11:48 AM, Matthew Wilcox wrote: > > On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote: > > > +static inline int __compound_mapcount(struct page *page) > > > +{ > > > + page = compound_head(page); > > > + return atomic_read(compound_mapcount_ptr(page)) + 1; > > > +} > > > > I'd suggest instead: > > > > static inline int head_mapcount(struct page *head) > > { > > return atomic_read(compound_mapcount_ptr(head)) + 1; > > } > > > > > +static inline int dump_page_compound_mapcount(struct page *page) > > > +{ > > > + if (WARN_ON_ONCE(!PageCompound(page))) > > > + return 0; > > > + return __compound_mapcount(page); > > > } > > > > And just dropping this ... it shouldn't be used outside mm/debug.c anyway. > > Yes, that's cleaner. I'll send a v2 that is not a reply, so that Andrew can > spot it more easily. OK to add your "Suggested-by:" to that? Of course! Thanks! > > Thinking about it, we'll get the hint later that this is not to be trusted > > when the flags from the page do not include 'head'. > > > > Good point. I think this will work out pretty well, then. > > thanks, > -- > John Hubbard > NVIDIA
diff --git a/include/linux/mm.h b/include/linux/mm.h index dc7b87310c10..e3991fbb42c0 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -779,6 +779,13 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags) extern void kvfree(const void *addr); extern void kvfree_sensitive(const void *addr, size_t len); + +static inline int __compound_mapcount(struct page *page) +{ + page = compound_head(page); + return atomic_read(compound_mapcount_ptr(page)) + 1; +} + /* * Mapcount of compound page as a whole, does not include mapped sub-pages. * @@ -787,8 +794,14 @@ extern void kvfree_sensitive(const void *addr, size_t len); static inline int compound_mapcount(struct page *page) { VM_BUG_ON_PAGE(!PageCompound(page), page); - page = compound_head(page); - return atomic_read(compound_mapcount_ptr(page)) + 1; + return __compound_mapcount(page); +} + +static inline int dump_page_compound_mapcount(struct page *page) +{ + if (WARN_ON_ONCE(!PageCompound(page))) + return 0; + return __compound_mapcount(page); } /* diff --git a/mm/debug.c b/mm/debug.c index 4f376514744d..eab4244aabd8 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -91,7 +91,7 @@ void __dump_page(struct page *page, const char *reason) "compound_mapcount:%d compound_pincount:%d\n", page, page_ref_count(head), mapcount, mapping, page_to_pgoff(page), head, - compound_order(head), compound_mapcount(page), + compound_order(head), dump_page_compound_mapcount(page), compound_pincount(page)); } else { pr_warn("page:%px refcount:%d mapcount:%d mapping:%p " @@ -99,7 +99,7 @@ void __dump_page(struct page *page, const char *reason) "compound_mapcount:%d\n", page, page_ref_count(head), mapcount, mapping, page_to_pgoff(page), head, - compound_order(head), compound_mapcount(page)); + compound_order(head), dump_page_compound_mapcount(page)); } else pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
If a compound page is being split while dump_page() is being run on that page, we can end up calling compound_mapcount() on a page that is no longer compound. This leads to a crash (already seen at least once in the field), due to the VM_BUG_ON_PAGE() assertion inside compound_mapcount(). (The above is from Matthew Wilcox's analysis of Qian Cai's bug report.) In order to avoid this kind of crash, make dump_page() slightly more robust, by providing a version of compound_page() that doesn't assert, but just warns the first time that such a thing happens. And the first time is usually enough. For debug tools, we don't want to go *too* far in this direction, but this is a simple small fix, and the crash has already been seen, so it's a good trade-off. Reported-by: Qian Cai <cai@lca.pw> Cc: Matthew Wilcox <willy@infradead.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- include/linux/mm.h | 17 +++++++++++++++-- mm/debug.c | 4 ++-- 2 files changed, 17 insertions(+), 4 deletions(-)