On 2025-02-11 10:15, David Hildenbrand wrote:
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.
That's a good suggestion, I'll give it a try.
+ __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.
OK.
Nothing jumped at me from a quick glimpse, but yes, this usage is not that easy.
