Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-15 Thread Jan Kara
On Thu 07-01-21 13:53:18, John Hubbard wrote: > On 1/7/21 1:29 PM, Linus Torvalds wrote: > > On Thu, Jan 7, 2021 at 12:59 PM Andrea Arcangeli > > wrote: > > > > > > The problem is it's not even possible to detect reliably if there's > > > really a long term GUP pin because of speculative

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Andrea Arcangeli
On Fri, Jan 08, 2021 at 11:25:21AM -0800, Linus Torvalds wrote: > On Fri, Jan 8, 2021 at 9:53 AM Andrea Arcangeli wrote: > > > > Do you intend to eventually fix the zygote vmsplice case or not? > > Because in current upstream it's not fixed currently using the > > enterprise default config. > >

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Linus Torvalds
On Fri, Jan 8, 2021 at 9:53 AM Andrea Arcangeli wrote: > > Do you intend to eventually fix the zygote vmsplice case or not? > Because in current upstream it's not fixed currently using the > enterprise default config. Is this the hugepage case? Neither of your patches actually touched that, so

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Andrea Arcangeli
On Fri, Jan 08, 2021 at 09:39:56AM -0800, Linus Torvalds wrote: > page_count() is simply the right and efficient thing to do. > > You talk about all these theoretical inefficiencies for cases like > zygote and page pinning, which have never ever been seen except as a > possible attack vector. Do

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Linus Torvalds
On Fri, Jan 8, 2021 at 8:14 AM Andrea Arcangeli wrote: > > page_count in do_wp_page is a fix for the original security issue Not just that. page_count() is simply the right and efficient thing to do. You talk about all these theoretical inefficiencies for cases like zygote and page pinning,

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Linus Torvalds
On Fri, Jan 8, 2021 at 4:48 AM Will Deacon wrote: > > It certainly looks simple and correct to me, although it means we're now > taking the mmap sem for write in the case where we only want to clear the > access flag, which should be fine with the thing only held for read, no? When I was looking

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Andrea Arcangeli
Hello everyone, On Fri, Jan 08, 2021 at 12:48:16PM +, Will Deacon wrote: > On Thu, Jan 07, 2021 at 04:25:54PM -0800, Linus Torvalds wrote: > > Please. Why is the correct patch not the attached one (apart from the > > obvious fact that I haven't tested it and maybe just screwed up > >

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-08 Thread Will Deacon
On Thu, Jan 07, 2021 at 04:25:54PM -0800, Linus Torvalds wrote: > Please. Why is the correct patch not the attached one (apart from the > obvious fact that I haven't tested it and maybe just screwed up > completely - but you get the idea)? It certainly looks simple and correct to me, although it

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 3:48 PM Andrea Arcangeli wrote: > > > The alternate fix remains "make sure we always flush the TLB before > > releasing the page table lock, and make COW do the copy under the page > > table lock". But I really would prefer to just have this code follow > The copy under PT

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Andrea Arcangeli
On Thu, Jan 07, 2021 at 02:51:24PM -0800, Linus Torvalds wrote: > Ho humm. I had obviously not looked very much at that code. I had done > a quick git grep, but now that I look closer, it *does* get the > mmap_sem for writing, but only for that VM_SOFTDIRTY bit clearing, and > then it does a

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Andrea Arcangeli
On Thu, Jan 07, 2021 at 02:42:17PM -0800, Linus Torvalds wrote: > On Thu, Jan 7, 2021 at 2:31 PM Andrea Arcangeli wrote: > > > > Random memory corruption will still silently materialize as result of > > the speculative lookups in the above scenario. > > Explain. > > Yes, you'll get random

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 2:42 PM Linus Torvalds wrote: > > But I thought we agreed earlier that that is a bug. And I thought the > softdirty code already got it for writing. Ho humm. I had obviously not looked very much at that code. I had done a quick git grep, but now that I look closer, it

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 2:31 PM Andrea Arcangeli wrote: > > Random memory corruption will still silently materialize as result of > the speculative lookups in the above scenario. Explain. Yes, you'll get random memory corruption if you keep doing wrprotect() without mmap_sem held for writing.

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread John Hubbard
On 1/7/21 2:24 PM, Linus Torvalds wrote: On Thu, Jan 7, 2021 at 2:20 PM Linus Torvalds wrote: Hmm, add a check for the page being PageAnon(), perhaps? If it's a shared vma, then the page can be pinned shared with multiple mappings, I agree. Or check the vma directly for whether it's a COW

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Andrea Arcangeli
On Thu, Jan 07, 2021 at 01:29:43PM -0800, Linus Torvalds wrote: > On Thu, Jan 7, 2021 at 12:59 PM Andrea Arcangeli wrote: > > > > The problem is it's not even possible to detect reliably if there's > > really a long term GUP pin because of speculative pagecache lookups. > > So none of the normal

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 2:20 PM Linus Torvalds wrote: > > Hmm, add a check for the page being PageAnon(), perhaps? > > If it's a shared vma, then the page can be pinned shared with multiple > mappings, I agree. Or check the vma directly for whether it's a COW vma. That's probably a more obvious

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 2:14 PM John Hubbard wrote: > > > > Literally just adding a " && page_mapcount(page) == 1" in there > > (probably best done inside page_maybe_dma_pinned() itself) > > Well, that means that pages that are used for pinned DMA like this, can > not be shared with other

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread John Hubbard
On 1/7/21 2:00 PM, Linus Torvalds wrote: On Thu, Jan 7, 2021 at 1:53 PM John Hubbard wrote: Now, I do agree that from a QoI standpoint, it would be really lovely if we actually enforced it. I'm not entirely sure we can, but maybe it would be reasonable to use that mm->has_pinned &&

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 1:53 PM John Hubbard wrote: > > > > > Now, I do agree that from a QoI standpoint, it would be really lovely > > if we actually enforced it. I'm not entirely sure we can, but maybe it > > would be reasonable to use that > > > >mm->has_pinned &&

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread John Hubbard
On 1/7/21 1:29 PM, Linus Torvalds wrote: On Thu, Jan 7, 2021 at 12:59 PM Andrea Arcangeli wrote: The problem is it's not even possible to detect reliably if there's really a long term GUP pin because of speculative pagecache lookups. So none of the normal code _needs_ that any more these

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread kernel test robot
Hi Andrea, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linux/master] [also build test WARNING on linus/master hnaz-linux-mm/master v5.11-rc2 next-20210104] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch,

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 12:59 PM Andrea Arcangeli wrote: > > The problem is it's not even possible to detect reliably if there's > really a long term GUP pin because of speculative pagecache lookups. So none of the normal code _needs_ that any more these days, which is what I think is so nice.

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Andrea Arcangeli
Hi Linus, On Thu, Jan 07, 2021 at 12:17:40PM -0800, Linus Torvalds wrote: > On Thu, Jan 7, 2021 at 12:04 PM Andrea Arcangeli wrote: > > > > However there are two cases that could wrprotecting exclusive anon > > pages with only the mmap_read_lock: > > I still think the real fix is "Don't do that

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 12:17 PM Linus Torvalds wrote: > > I still think the real fix is "Don't do that then", and just take the > write lock. The alternative, of course, is to just make sure the page table flush is done inside the page table lock (and then we make COW do the copy inside of it).

Re: [PATCH 2/2] mm: soft_dirty: userfaultfd: introduce wrprotect_tlb_flush_pending

2021-01-07 Thread Linus Torvalds
On Thu, Jan 7, 2021 at 12:04 PM Andrea Arcangeli wrote: > > However there are two cases that could wrprotecting exclusive anon > pages with only the mmap_read_lock: I still think the real fix is "Don't do that then", and just take the write lock. The UFFDIO_WRITEPROTECT case simply isn't that