On Wed, Apr 15, 2020 at 02:52:31PM -0700, Dave Hansen wrote: > On 3/6/20 5:25 AM, Claudio Imbrenda wrote: > > + /* > > + * We need to make the page accessible if and only if we are going > > + * to access its content (the FOLL_PIN case). Please see > > + * Documentation/core-api/pin_user_pages.rst for details. > > + */ > > + if (flags & FOLL_PIN) { > > + ret = arch_make_page_accessible(page); > > + if (ret) { > > + unpin_user_page(page); > > + page = ERR_PTR(ret); > > + goto out; > > + } > > + } > > Thanks, Claudio, for a really thorough refresher on this in private mail. > > But, I think this mechanism probably hooks into the wrong place. I > don't doubt that it *functions* on s390, but I think these calls are > misplaced. I think the end result is that no other architecture will > have a chance to use the same hooks. They're far too s390-specific even > for a concept that's not limited to s390. > > get_user_pages(FOLL_PIN) does *not* mean "the kernel will access this > page's contents". The kmap() family is really what we use for that. > kmap()s are often *preceded* by get_user_pages(), which is probably why > this works for you, though. > > Yes, the docs do say that FOLL_PIN is for accessing the pages. But, > there's a crucial thing that it leaves out: *WHO* will be accessing the > pages. For Direct IO, for instance, the CPU isn't touching the page at > all. It's always a device. Also, crucially, the page contents are > *not* accessible from the CPU's perspective after a gup. They're not > accessible until a kmap(). They're also not even accessible for > *devices* after a gup. There's a _separate_ mapping process that's > requires to make them accessible to the CPU. I think the crucial detail is that we can fail gup(), while we cannot ever fail kmap() or whatever else a device needs to do. > > --- a/mm/page-writeback.c > > +++ b/mm/page-writeback.c > > @@ -2764,7 +2764,7 @@ int test_clear_page_writeback(struct page *page) > > int __test_set_page_writeback(struct page *page, bool keep_write) > > { > > struct address_space *mapping = page_mapping(page); > > - int ret; > > + int ret, access_ret; > > > > lock_page_memcg(page); > > if (mapping && mapping_use_writeback_tags(mapping)) { > > @@ -2807,6 +2807,13 @@ int __test_set_page_writeback(struct page *page, bool keep_write) > > inc_zone_page_state(page, NR_ZONE_WRITE_PENDING); > > } > > unlock_page_memcg(page); > > + access_ret = arch_make_page_accessible(page); > > + /* > > + * If writeback has been triggered on a page that cannot be made > > + * accessible, it is too late to recover here. > > + */ > > + VM_BUG_ON_PAGE(access_ret != 0, page); > > + > > return ret; > > > > } > > I think this one really shows the cracks in the approach. Pages being > swapped *don't* have get_user_pages() done on them since we've already > got the physical page at the time writeback and aren't looking at PTEs. I suspect this happens because FOLL_TOUCH or something later does set_page_dirty() on the page, which then eventually gets it in writeback. Failing gup() ealier, should ensure the above VM_BUG never happens, unless someone is doing dodgy things. > Why do I care? > > I was looking at AMD's SEV (Secure Encrypted Virtualization) code which > is in the kernel which shares some implementation details with the > not-in-the-tree Intel MKTME. SEV currently has a concept of guest pages > being encrypted and being gibberish to the host, plus a handshake to > share guest-selected pages. Some of the side-effects of exposing the > gibberish to the host aren't great (I think it can break cache coherency > if a stray write occurs) and it would be nice to get better behavior. > > But, to get better behavior, the host kernel might need to remove pages > from its direct map, making them inaccessible. But for SEV we would actually need to fail this arch_make_page_acesssible() thing, right? The encrypted guest pages cannot be sanely accessed by the host IIRC, ever. Isn't their encryption key linked to the phys addr of the page? > I was hoping to reuse > arch_make_page_accessible() for obvious reasons. But, get_user_pages() > is not the right spot to map pages because they might not *ever* be > accessed by the CPU, only devices. I'm confused, why does it matter who accesses it? The point is that they want to access it through this vaddr/mapping.