On Thu, Oct 10, 2024 at 03:28:38PM +0100, Fuad Tabba wrote:
> On Thu, 10 Oct 2024 at 13:21, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> >
> > On Thu, Oct 10, 2024 at 11:23:55AM +0100, Fuad Tabba wrote:
> > > Hi Kirill,
> > >
> > > On Thu, 10 Oct 2024 at 11:14, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Oct 10, 2024 at 09:59:23AM +0100, Fuad Tabba wrote:
> > > > > +out:
> > > > > + if (ret != VM_FAULT_LOCKED) {
> > > > > + folio_put(folio);
> > > > > + folio_unlock(folio);
> > > >
> > > > Hm. Here and in few other places you return reference before unlocking.
> > > >
> > > > I think it is safe because nobody can (or can they?) remove the page from
> > > > pagecache while the page is locked so we have at least one refcount on the
> > > > folie, but it *looks* like a use-after-free bug.
> > > >
> > > > Please follow the usual pattern: _unlock() then _put().
> > >
> > > That is deliberate, since these patches rely on the refcount to check
> > > whether the host has any mappings, and the folio lock in order not to
> > > race. It's not that it's not safe to decrement the refcount after
> > > unlocking, but by doing that i cannot rely on the folio lock to ensure
> > > that there aren't any races between the code added to check whether a
> > > folio is mappable, and the code that checks whether the refcount is
> > > safe. It's a tiny window, but it's there.
> > >
> > > What do you think?
> >
> > I don't think your scheme is race-free either. gmem_clear_mappable() is
> > going to fail with -EPERM if there's any transient pin on the page. For
> > instance from any physical memory scanner.
>
> I remember discussing this before. One question that I have is, is it
> possible to get a transient pin while the folio lock is held, or would
> that have happened before taking the lock?
Yes.
The normal pattern is to get the pin on the page before attempting to lock
it.
In case of physical scanners it happens like this:
1. pfn_to_page()/pfn_folio()
2. get_page_unless_zero()/folio_get_nontail_page()
3. lock_page()/folio_lock() if needed
--
Kiryl Shutsemau / Kirill A. Shutemov
