linux-kernel - Re: [PATCH v3 2/8] binder: concurrent page installation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Zy6QC71iEy3w5FYA@google.com>
Date: Fri, 8 Nov 2024 22:26:19 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Christian Brauner <brauner@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	linux-kernel@...r.kernel.org, kernel-team@...roid.com,
	David Hildenbrand <david@...hat.com>,
	Barry Song <v-songbaohua@...o.com>
Subject: Re: [PATCH v3 2/8] binder: concurrent page installation

On Fri, Nov 08, 2024 at 03:41:41PM -0500, Liam R. Howlett wrote:
> * Carlos Llamas <cmllamas@...gle.com> [241108 14:11]:
> > Allow multiple callers to install pages simultaneously by downgrading
> > the mmap_sem to non-exclusive mode.
> 
> Since we actually allow downgrading of a lock, it would be more clear to
> say that you are changing from a read lock to a write lock.  I was
> searching for the lock downgrade in this patch :)

ah, I can see how the wording here can be misleading. I'll rephrase that
to avoid confusion.

> > -	ret = vm_insert_page(alloc->vma, addr, page);
> > -	if (ret) {
> > +	mmap_read_lock(alloc->mm);
> 
> Ah, I debate bring this up, but you can do this without the mmap read
> lock.  You can use rcu and the per-vma locking like in the page fault
> path.  If you look at how that is done using the lock_vma_under_rcu()
> (mm/memory.c) then you can see that pages are installed today without
> the mmap locking (on some architectures - which includes x86_64 and
> arm64).

Right, per-vma locking is implemented in patch 7 of this series:
https://lore.kernel.org/all/20241108191057.3288442-8-cmllamas@google.com/

> userfaultfd had to do something like this as well, and created some
> abstraction to facilitate either mmap or rcu, based on the arch support.
> Going to rcu really helped performance there [1].  There was also a
> chance of the vma being missed, so it is checked again under the mmap
> read lock, but realistically that never happens and exists to ensure
> correctness.

hmm, if there are more users of this pattern in the future then perhaps
it might be worth adding a common helper?

> You also mention the shrinker and using the alloc->mutex, well zapping
> pages can also be done under the vma specific lock - if that helps?
> 
> Freeing page tables is different though, that needs more locking, but I
> don't think this is an issue for you.
> 
> [1]. https://lore.kernel.org/all/20240215182756.3448972-1-lokeshgidra@google.com/

ha, I was not aware of this. I'll have a look thanks.

> 
> > +	vma = vma_lookup(alloc->mm, addr);
> 
> Thank you for not saving a pointer anymore.

lol, thanks to you for initiating this effort!

Cheers,
Carlos Llamas