[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3e95a35-b0e3-b733-92f4-98bcccbe7ca5@intel.com>
Date: Thu, 30 Apr 2020 15:06:46 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Christian Borntraeger <borntraeger@...ibm.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>,
viro@...iv.linux.org.uk
Cc: david@...hat.com, akpm@...ux-foundation.org, aarcange@...hat.com,
linux-mm@...ck.org, frankja@...ux.ibm.com, sfr@...b.auug.org.au,
jhubbard@...dia.com, linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org, jack@...e.cz, kirill@...temov.name,
peterz@...radead.org, sean.j.christopherson@...el.com,
Ulrich.Weigand@...ibm.com
Subject: Re: [PATCH v2 1/1] fs/splice: add missing callback for inaccessible
pages
I was also wondering if Claudio was right about the debug patch having
races. I went to go look how the s390 code avoids races when pages go
from accessible->inaccessible.
Because, if if all of the traps are in place to transform pages from
inaccessible->accessible, the code *after* those traps is still
vulnerable. What *keeps* pages accessible?
The race avoidance is this, basically:
down_read(&gmap->mm->mmap_sem);
lock_page(page);
ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
...
> expected = expected_page_refs(page);
> if (!page_ref_freeze(page, expected))
> return -EBUSY;
> set_bit(PG_arch_1, &page->flags);
> rc = uv_call(0, (u64)uvcb);
> page_ref_unfreeze(page, expected);
... up_read(mmap_sem) / unlock_page() / unlock pte
I'm assuming that after the uv_call(), the page is inaccessible and I/O
devices will go boom if they touch the page.
The page_ref_freeze() ensures that references come between the
freeze/unfreeze are noticed, but it doesn't actually *stop* new ones for
users that hold references already. For the page cache, especially,
someone could do:
page = find_get_page();
arch_make_page_accessible();
lock_page();
... make_secure_pte();
unlock_page();
get_page();
// ^ OK because I have a ref
// do DMA on inaccessible page
Because the make_secure_pte() code isn't looking for a *specific*
'expected' value, it has no way of noticing that the extra ref snuck in
there.
I _think_ expected actually needs to be checked for having a specific
(low) value so that if there's a *possibility* of a reference holder
acquiring additional references, the page is known to be off-limits.
mm/migrate.c has a few examples of this, but I'm not quite sure how
bulletproof they are. Some of it appears to just be optimizations.
Powered by blists - more mailing lists