linux-kernel - Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Oct 2019 16:17:08 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thomas Hellström (VMware) 
        <thomas_os@...pmail.org>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        torvalds@...ux-foundation.org, kirill@...temov.name,
        Thomas Hellstrom <thellstrom@...are.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Rik van Riel <riel@...riel.com>,
        Minchan Kim <minchan@...nel.org>,
        Michal Hocko <mhocko@...e.com>,
        Huang Ying <ying.huang@...el.com>,
        Jérôme Glisse <jglisse@...hat.com>
Subject: Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for
 address space ranges

On Thu, Oct 10, 2019 at 03:24:47PM +0200, Thomas Hellström (VMware) wrote:
> On 10/10/19 3:05 PM, Peter Zijlstra wrote:
> > On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:

> > > +/**
> > > + * wp_shared_mapping_range - Write-protect all ptes in an address space range
> > > + * @mapping: The address_space we want to write protect
> > > + * @first_index: The first page offset in the range
> > > + * @nr: Number of incremental page offsets to cover
> > > + *
> > > + * Note: This function currently skips transhuge page-table entries, since
> > > + * it's intended for dirty-tracking on the PTE level. It will warn on
> > > + * encountering transhuge write-enabled entries, though, and can easily be
> > > + * extended to handle them as well.
> > > + *
> > > + * Return: The number of ptes actually write-protected. Note that
> > > + * already write-protected ptes are not counted.
> > > + */
> > > +unsigned long wp_shared_mapping_range(struct address_space *mapping,
> > > +				      pgoff_t first_index, pgoff_t nr)
> > > +{
> > > +	struct wp_walk wpwalk = { .total = 0 };
> > > +
> > > +	i_mmap_lock_read(mapping);
> > > +	WARN_ON(walk_page_mapping(mapping, first_index, nr, &wp_walk_ops,
> > > +				  &wpwalk));
> > > +	i_mmap_unlock_read(mapping);
> > > +
> > > +	return wpwalk.total;
> > > +}

> > That's a read lock, this means there's concurrency to self. What happens
> > if someone does two concurrent wp_shared_mapping_range() on the same
> > mapping?
> > 
> > The thing is, because of pte_wrprotect() the iteration that starts last
> > will see a smaller pte_write range, if it completes first and does
> > flush_tlb_range(), it will only flush a partial range.
> > 
> > This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
> > not using mm_tlb_flush_nested() to detect the situation and do a bigger
> > flush.
> > 
> > Or if you're not needing that, then I'm missing why.
> 
> Good catch. Thanks,
> 
> Yes the read lock is not intended to protect against concurrent users but to
> protect the vmas from disappearing under us. Since it fundamentally makes no
> sense having two concurrent threads picking up dirty ptes on the same
> address_space range we have an external range-based lock to protect against
> that.

Nothing mandates/verifies the function you expose is used exclusively.
Therefore you cannot make assumptions on that range lock your user has.

> However, that external lock doesn't protect other code  from concurrently
> modifying ptes and having the mm's  tlb_flush_pending increased, so I guess
> we unconditionally need to test for that and do a full range flush if
> necessary?

Yes, something like:

	if (mm_tlb_flush_nested(mm))
		flush_tlb_range(walk->vma, walk->vma->vm_start, walk->vma->vm_end);
	else  if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
		flush_tlb_range(walk->vma, wpwalk->tlbflush_start, wpwalk->tlbflush_end);