On 24.01.25 22:37, Luiz Capitulino wrote:
The page_ext_next() function assumes that page extension objects for a
page order allocation always reside in the same memory section, which
may not be true and could lead to crashes. Use the page_ext_iter API
instead.
Fixes: e98337d11bbd ("mm/contig_alloc: support __GFP_COMP")
Signed-off-by: Luiz Capitulino <luizcap@xxxxxxxxxx>
---
[...]
void __folio_copy_owner(struct folio *newfolio, struct folio *old)
@@ -364,24 +376,26 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
int i;
struct page_ext *old_ext;
struct page_ext *new_ext;
+ struct page_ext_iter old_iter;
+ struct page_ext_iter new_iter;
struct page_owner *old_page_owner;
struct page_owner *new_page_owner;
depot_stack_handle_t migrate_handle;
- old_ext = page_ext_get(&old->page);
+ old_ext = page_ext_iter_begin(&old_iter, &old->page);
if (unlikely(!old_ext))
return;
- new_ext = page_ext_get(&newfolio->page);
+ new_ext = page_ext_iter_begin(&new_iter, &newfolio->page);
if (unlikely(!new_ext)) {
- page_ext_put(old_ext);
+ page_ext_iter_end(&old_iter);
return;
}
old_page_owner = get_page_owner(old_ext);
new_page_owner = get_page_owner(new_ext);
migrate_handle = new_page_owner->handle;
- __update_page_owner_handle(new_ext, old_page_owner->handle,
+ __update_page_owner_handle(&new_iter, old_page_owner->handle,
old_page_owner->order, old_page_owner->gfp_mask,
old_page_owner->last_migrate_reason,
old_page_owner->ts_nsec, old_page_owner->pid,
@@ -390,8 +404,13 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
* Do not proactively clear PAGE_EXT_OWNER{_ALLOCATED} bits as the folio
* will be freed after migration. Keep them until then as they may be
* useful.
+ *
+ * Note that we need to re-grab the page_ext iterator since
+ * __update_page_owner_handle changed it.
*/
- __update_page_owner_free_handle(new_ext, 0, old_page_owner->order,
+ page_ext_iter_end(&new_iter);
+ page_ext_iter_begin(&new_iter, &newfolio->page);
So a page_ext_iter_reset() could be helpful, that wouldn't drop the RCU
lock. With that, we could probably also drop the comment.
+ __update_page_owner_free_handle(&new_iter, 0, old_page_owner->order,
old_page_owner->free_pid,
old_page_owner->free_tgid,
old_page_owner->free_ts_nsec);
@@ -402,12 +421,12 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
*/
for (i = 0; i < (1 << new_page_owner->order); i++) {
old_page_owner->handle = migrate_handle;
- old_ext = page_ext_next(old_ext);
+ old_ext = page_ext_iter_next(&old_iter);
old_page_owner = get_page_owner(old_ext);
}
- page_ext_put(new_ext);
- page_ext_put(old_ext);
+ page_ext_iter_end(&new_iter);
+ page_ext_iter_end(&old_iter);
In general, we should look into implementing the iterator without
temporarily dropping the RCU lock I think.
Nothing jumped at me from a quick glimpse, but yes, this usage is not
that easy.
--
Cheers,
David / dhildenb
