search for: get_page_unless_zero

...or_retry(folio, vma->vm_mm, vmf->flags)) + /* + * We need a page reference to lock the page because we don't + * hold the PTL so a racing thread can remove the + * device-exclusive entry and unmap the page. If the page is + * free the entry must have been removed already. + */ + if (!get_page_unless_zero(vmf->page)) + return 0; + + if (!folio_lock_or_retry(folio, vma->vm_mm, vmf->flags)) { + put_page(vmf->page); return VM_FAULT_RETRY; + } mmu_notifier_range_init_owner(&range, MMU_NOTIFY_EXCLUSIVE, 0, vma, vma->vm_mm, vmf->address & PAGE_MASK, (vmf->addr...

...used on most systems) doesn't sound nice to me. > >>> This is the whole point which made me think about the whole reference > >>> count approach in the first place. > >> > >> Again, the race I think that is possible > >> > >> somebody: get_page_unless_zero(page) > >> virtio_mem: page_ref_dec(pfn_to_page(pfn) > >> somebody: put_page() -> straight to the buddy > > > > Who is that somebody? I thought that it is only the owner/driver to have > > a control over the page. Also the above is not possible as long as the...

...used on most systems) doesn't sound nice to me. > >>> This is the whole point which made me think about the whole reference > >>> count approach in the first place. > >> > >> Again, the race I think that is possible > >> > >> somebody: get_page_unless_zero(page) > >> virtio_mem: page_ref_dec(pfn_to_page(pfn) > >> somebody: put_page() -> straight to the buddy > > > > Who is that somebody? I thought that it is only the owner/driver to have > > a control over the page. Also the above is not possible as long as the...

...>flags)) > + /* > + * We need a page reference to lock the page because we don't > + * hold the PTL so a racing thread can remove the > + * device-exclusive entry and unmap the page. If the page is > + * free the entry must have been removed already. > + */ > + if (!get_page_unless_zero(vmf->page)) > + return 0; >From a folio point of view: what the hell are you doing here? Tail pages don't have individual refcounts; all the refcounts are actually taken on the folio. So this should be: if (!folio_try_get(folio)) return 0; (you can fix up the comment yourself)...

On 3/28/23 20:16, Matthew Wilcox wrote: ... >> + if (!get_page_unless_zero(vmf->page)) >> + return 0; > > From a folio point of view: what the hell are you doing here? Tail > pages don't have individual refcounts; all the refcounts are actually ohh, and I really should have caught that too. I plead spending too much time recently in a somewhat m...

John Hubbard <jhubbard at nvidia.com> writes: > On 3/28/23 20:16, Matthew Wilcox wrote: > ... >>> + if (!get_page_unless_zero(vmf->page)) >>> + return 0; >> From a folio point of view: what the hell are you doing here? Tail >> pages don't have individual refcounts; all the refcounts are actually I had stuck with using the page because none of this stuff (yet) supports compound pages anyway...

...if (page->mapping->a_ops->isolatepage(page, > + isolate_mode)) { Dereferencing page->mapping isn't safe here. Page could be "truncated" from mapping at any time. As you can see balloon_page_isolate() calls get_page_unless_zero, trylock_page and only after that checks balloon_page_movable again. Existing code already does similar unsafe dereference in __isolate_lru_page(): page->mapping->a_ops->migratepage > /* Successfully isolated */ >...

...if (page->mapping->a_ops->isolatepage(page, > + isolate_mode)) { Dereferencing page->mapping isn't safe here. Page could be "truncated" from mapping at any time. As you can see balloon_page_isolate() calls get_page_unless_zero, trylock_page and only after that checks balloon_page_movable again. Existing code already does similar unsafe dereference in __isolate_lru_page(): page->mapping->a_ops->migratepage > /* Successfully isolated */ >...

...hot path with > > something that is barely used on most systems) doesn't sound nice to me. > > This is the whole point which made me think about the whole reference > > count approach in the first place. > > Again, the race I think that is possible > > somebody: get_page_unless_zero(page) > virtio_mem: page_ref_dec(pfn_to_page(pfn) > somebody: put_page() -> straight to the buddy Who is that somebody? I thought that it is only the owner/driver to have a control over the page. Also the above is not possible as long as the owner/driver keeps a reference to the PageOffli...

...hot path with > > something that is barely used on most systems) doesn't sound nice to me. > > This is the whole point which made me think about the whole reference > > count approach in the first place. > > Again, the race I think that is possible > > somebody: get_page_unless_zero(page) > virtio_mem: page_ref_dec(pfn_to_page(pfn) > somebody: put_page() -> straight to the buddy Who is that somebody? I thought that it is only the owner/driver to have a control over the page. Also the above is not possible as long as the owner/driver keeps a reference to the PageOffli...

...h: >> >> "If you want to use the refcount field, it must be used in such a way >> that other CPUs temporarily incrementing and then decrementing the >> refcount does not cause problems" >> >> And that made me think "anybody can go ahead and try get_page_unless_zero()". >> >> If I am missing something here and this can indeed not happen (e.g., >> because PageOffline() pages are never mapped to user space), then I'll >> happily remove this code. > > The point is that if the owner of the page is holding the only reference...

...ed under the PTL > so no futher bad page accesses occur once it is locked. Maybe change that last sentence to something like this: "Fix this by taking a page reference before starting to remove a device exclusive pte. This is done safely in a lock-free way by first getting a reference via get_page_unless_zero(), and then re-checking after acquiring the PTL, that the page is the correct one." ? ...well, maybe that's not all that much help. But it does at least provide the traditional description of what the patch *does*, at the end of the commit description. But please treat this as just an op...

...9;s flags operation(i.e., PG_movable, PG_isolated) > atomicity. Another thing is for stability for page->mapping->a_ops. > > For example, > > isolate_migratepages_block > if (PageMovable(page)) > isolate_movable_page > get_page_unless_zero <--- 1 > trylock_page > page->mapping->a_ops->isolate_page <--- 2 > > Between 1 and 2, driver can nullify page->mapping so we need PG_lock Hmm I see, that really doesn't seem easily solvable without page_lock. My i...

...9;s flags operation(i.e., PG_movable, PG_isolated) > atomicity. Another thing is for stability for page->mapping->a_ops. > > For example, > > isolate_migratepages_block > if (PageMovable(page)) > isolate_movable_page > get_page_unless_zero <--- 1 > trylock_page > page->mapping->a_ops->isolate_page <--- 2 > > Between 1 and 2, driver can nullify page->mapping so we need PG_lock Hmm I see, that really doesn't seem easily solvable without page_lock. My i...

...> something that is barely used on most systems) doesn't sound nice to me. >>> This is the whole point which made me think about the whole reference >>> count approach in the first place. >> >> Again, the race I think that is possible >> >> somebody: get_page_unless_zero(page) >> virtio_mem: page_ref_dec(pfn_to_page(pfn) >> somebody: put_page() -> straight to the buddy > > Who is that somebody? I thought that it is only the owner/driver to have > a control over the page. Also the above is not possible as long as the > owner/driver keeps...

...t the > > reference count of those pages? > > Oh, I forgot to answer this questions. The __put_page() change is necessary > for the following race I identified: > > Page has a refcount of 1 (e.g., allocated by virtio-mem using > alloc_contig_range()). > > a) kernel: get_page_unless_zero(page): refcount = 2 > b) virtio-mem: set page PG_offline, reduce refcount): refocunt = 1 > c) kernel: put_page(page): refcount = 0 > > The page would suddenly be given to the buddy. which is bad. But why cannot you keep the reference count at 1 (do get_page when offlining the page)? I...

...t the > > reference count of those pages? > > Oh, I forgot to answer this questions. The __put_page() change is necessary > for the following race I identified: > > Page has a refcount of 1 (e.g., allocated by virtio-mem using > alloc_contig_range()). > > a) kernel: get_page_unless_zero(page): refcount = 2 > b) virtio-mem: set page PG_offline, reduce refcount): refocunt = 1 > c) kernel: put_page(page): refcount = 0 > > The page would suddenly be given to the buddy. which is bad. But why cannot you keep the reference count at 1 (do get_page when offlining the page)? I...

...rqsave(&b_dev_info->pages_lock, flags); + while (!list_empty(&b_dev_info->pages)) { + page = list_first_entry(&b_dev_info->pages, typeof(*page), lru); + /* move to processed list to avoid going over it another time */ + list_move(&page->lru, &processed); + + if (!get_page_unless_zero(page)) + continue; + /* + * pages_lock nests within page lock, + * so drop it before trylock_page + */ + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags); + /* * Block others from accessing the 'page' while we get around * establishing additional references...

...rqsave(&b_dev_info->pages_lock, flags); + while (!list_empty(&b_dev_info->pages)) { + page = list_first_entry(&b_dev_info->pages, typeof(*page), lru); + /* move to processed list to avoid going over it another time */ + list_move(&page->lru, &processed); + + if (!get_page_unless_zero(page)) + continue; + /* + * pages_lock nests within page lock, + * so drop it before trylock_page + */ + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags); + /* * Block others from accessing the 'page' while we get around * establishing additional references...

...k into __put_page? Why do we even care about the > reference count of those pages? Oh, I forgot to answer this questions. The __put_page() change is necessary for the following race I identified: Page has a refcount of 1 (e.g., allocated by virtio-mem using alloc_contig_range()). a) kernel: get_page_unless_zero(page): refcount = 2 b) virtio-mem: set page PG_offline, reduce refcount): refocunt = 1 c) kernel: put_page(page): refcount = 0 The page would suddenly be given to the buddy. which is bad. -- Thanks, David / dhildenb