| Message ID | 20151210023855.30368.37457.stgit@dwillia2-desk3.jf.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 09 Dec 2015 18:38:55 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > ZONE_DEVICE pages always have an elevated count and will never be on an > lru reclaim list. That space in 'struct page' can be redirected for > other uses, but for safety introduce a poison value that will always > trip __list_add() to assert. This allows half of the struct list_head > storage to be reclaimed with some assurance to back up the assumption > that the page count never goes to zero and a list_add() is never > attempted. > > ... > > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -108,9 +108,26 @@ static inline void list_del(struct list_head *entry) > entry->next = LIST_POISON1; > entry->prev = LIST_POISON2; > } > + > +#define list_del_poison list_del > #else > extern void __list_del_entry(struct list_head *entry); > extern void list_del(struct list_head *entry); > +extern struct list_head list_force_poison; > + > +/** > + * list_del_poison - poison an entry to always assert on list_add > + * @entry: the element to delete and poison > + * > + * Note: the assertion on list_add() only occurs when CONFIG_DEBUG_LIST=y, > + * otherwise this is identical to list_del() > + */ > +static inline void list_del_poison(struct list_head *entry) > +{ > + __list_del(entry->prev, entry->next); > + entry->next = &list_force_poison; > + entry->prev = &list_force_poison; > +} > #endif list_del() already poisons the list_head. Does this really add anything? > /** > diff --git a/lib/list_debug.c b/lib/list_debug.c > index 3859bf63561c..d730c064a4df 100644 > --- a/lib/list_debug.c > +++ b/lib/list_debug.c > @@ -12,6 +12,8 @@ > #include <linux/kernel.h> > #include <linux/rculist.h> > > +struct list_head list_force_poison; > + > /* > * Insert a new entry between two known consecutive entries. > * > @@ -23,6 +25,8 @@ void __list_add(struct list_head *new, > struct list_head *prev, > struct list_head *next) > { > + WARN(new->next == &list_force_poison || new->prev == &list_force_poison, > + "list_add attempted on force-poisoned entry\n"); I suppose that list_replace() should poison as well, and perhaps other places were missed.
On Tue, Dec 15, 2015 at 3:41 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 09 Dec 2015 18:38:55 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > >> ZONE_DEVICE pages always have an elevated count and will never be on an >> lru reclaim list. That space in 'struct page' can be redirected for >> other uses, but for safety introduce a poison value that will always >> trip __list_add() to assert. This allows half of the struct list_head >> storage to be reclaimed with some assurance to back up the assumption >> that the page count never goes to zero and a list_add() is never >> attempted. >> >> ... >> >> --- a/include/linux/list.h >> +++ b/include/linux/list.h >> @@ -108,9 +108,26 @@ static inline void list_del(struct list_head *entry) >> entry->next = LIST_POISON1; >> entry->prev = LIST_POISON2; >> } >> + >> +#define list_del_poison list_del >> #else >> extern void __list_del_entry(struct list_head *entry); >> extern void list_del(struct list_head *entry); >> +extern struct list_head list_force_poison; >> + >> +/** >> + * list_del_poison - poison an entry to always assert on list_add >> + * @entry: the element to delete and poison >> + * >> + * Note: the assertion on list_add() only occurs when CONFIG_DEBUG_LIST=y, >> + * otherwise this is identical to list_del() >> + */ >> +static inline void list_del_poison(struct list_head *entry) >> +{ >> + __list_del(entry->prev, entry->next); >> + entry->next = &list_force_poison; >> + entry->prev = &list_force_poison; >> +} >> #endif > > list_del() already poisons the list_head. Does this really add anything? The poison values that list_del() injects detect double-deletion, but it is fine to re-add a deleted entry. This version causes list_add() to warn if the list entry is ever attempted to be added. Maybe it is better called list_del_permanent(), but this really is a special case to catch code that attempts to use page->lru for a zone_device page. I think I'll make list_del_poison() private to kernel/memremap.c for now, I don't suspect any other code is doing union tricks like struct page to re-purpose fields. >> /** >> diff --git a/lib/list_debug.c b/lib/list_debug.c >> index 3859bf63561c..d730c064a4df 100644 >> --- a/lib/list_debug.c >> +++ b/lib/list_debug.c >> @@ -12,6 +12,8 @@ >> #include <linux/kernel.h> >> #include <linux/rculist.h> >> >> +struct list_head list_force_poison; >> + >> /* >> * Insert a new entry between two known consecutive entries. >> * >> @@ -23,6 +25,8 @@ void __list_add(struct list_head *new, >> struct list_head *prev, >> struct list_head *next) >> { >> + WARN(new->next == &list_force_poison || new->prev == &list_force_poison, >> + "list_add attempted on force-poisoned entry\n"); > > I suppose that list_replace() should poison as well, and perhaps other > places were missed. > At least in the case of list_replace() it has to have gone through a list_add() first.
diff --git a/include/linux/list.h b/include/linux/list.h index 5356f4d661a7..0d07bc1387aa 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -108,9 +108,26 @@ static inline void list_del(struct list_head *entry) entry->next = LIST_POISON1; entry->prev = LIST_POISON2; } + +#define list_del_poison list_del #else extern void __list_del_entry(struct list_head *entry); extern void list_del(struct list_head *entry); +extern struct list_head list_force_poison; + +/** + * list_del_poison - poison an entry to always assert on list_add + * @entry: the element to delete and poison + * + * Note: the assertion on list_add() only occurs when CONFIG_DEBUG_LIST=y, + * otherwise this is identical to list_del() + */ +static inline void list_del_poison(struct list_head *entry) +{ + __list_del(entry->prev, entry->next); + entry->next = &list_force_poison; + entry->prev = &list_force_poison; +} #endif /** diff --git a/lib/list_debug.c b/lib/list_debug.c index 3859bf63561c..d730c064a4df 100644 --- a/lib/list_debug.c +++ b/lib/list_debug.c @@ -12,6 +12,8 @@ #include <linux/kernel.h> #include <linux/rculist.h> +struct list_head list_force_poison; + /* * Insert a new entry between two known consecutive entries. * @@ -23,6 +25,8 @@ void __list_add(struct list_head *new, struct list_head *prev, struct list_head *next) { + WARN(new->next == &list_force_poison || new->prev == &list_force_poison, + "list_add attempted on force-poisoned entry\n"); WARN(next->prev != prev, "list_add corruption. next->prev should be " "prev (%p), but was %p. (next=%p).\n",
ZONE_DEVICE pages always have an elevated count and will never be on an lru reclaim list. That space in 'struct page' can be redirected for other uses, but for safety introduce a poison value that will always trip __list_add() to assert. This allows half of the struct list_head storage to be reclaimed with some assurance to back up the assumption that the page count never goes to zero and a list_add() is never attempted. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- include/linux/list.h | 17 +++++++++++++++++ lib/list_debug.c | 4 ++++ 2 files changed, 21 insertions(+)