| Message ID | 20190501202433.GC28500@bombadil.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | compound_head() vs uninitialized struct page poisoning | expand |
On Wed, May 1, 2019 at 4:24 PM Matthew Wilcox <willy@infradead.org> wrote: > > > Hi Pavel, > > This strikes me as wrong: > > #define PF_HEAD(page, enforce) PF_POISONED_CHECK(compound_head(page)) > > If we hit a page which is poisoned, PAGE_POISON_PATTERN is ~0, so PageTail > is set, and compound_head will return() 0xfff..ffe. PagePoisoned() > will then try to derefence that pointer and we'll get an oops that isn't > obviously PagePoisoned. > > I think this should have been: > > #define PF_HEAD(page, enforce) compound_head(PF_POISONED_CHECK(page)) Yes, I agree, this makes sense. > > One could make the argument for double-checking: > > #define PF_HEAD(page, enforce) PF_POISONED_CHECK(compound_head(PF_POISONED_CHECK(page))) > > but I think this is overkill; if a tail page is initialised, then there's > no way that its head page should have been uninitialised. Also agree, no need to check head if subpage is initialized. > > Would a patch something along these lines make sense? Compile-tested only. Yes, I like the re-ordering PF_POISONED_CHECK()s to be before the other accesses to PPs. Thank you, Pasha
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 9f8712a4b1a5..1d25d0899854 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -227,16 +227,18 @@ static inline void page_init_poison(struct page *page, size_t size) VM_BUG_ON_PGFLAGS(PagePoisoned(page), page); \ page; }) #define PF_ANY(page, enforce) PF_POISONED_CHECK(page) -#define PF_HEAD(page, enforce) PF_POISONED_CHECK(compound_head(page)) +#define PF_HEAD(page, enforce) compound_head(PF_POISONED_CHECK(page)) #define PF_ONLY_HEAD(page, enforce) ({ \ + PF_POISONED_CHECK(page); \ VM_BUG_ON_PGFLAGS(PageTail(page), page); \ - PF_POISONED_CHECK(page); }) + page; }) #define PF_NO_TAIL(page, enforce) ({ \ VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page); \ - PF_POISONED_CHECK(compound_head(page)); }) + compound_head(PF_POISONED_CHECK(page)); }) #define PF_NO_COMPOUND(page, enforce) ({ \ + PF_POISONED_CHECK(page); \ VM_BUG_ON_PGFLAGS(enforce && PageCompound(page), page); \ - PF_POISONED_CHECK(page); }) + page; }) /* * Macros to create function definitions for page flags