On 6/12/26 08:41, Vlastimil Babka (SUSE) wrote:

quoted

quoted

If the vma lookup fails because the mmap write lock is held, but the vma
actually exists (has not been unmapped), then this code might "successfully"
remove the page without invoking zap_vma_range(). This means that the
page does not actually get freed and will just hang around forever until
the process owning the vma exits or Binder needs this page and maps a
new page on top of the page.

Yeah, I think if lock_vma_under_rcu() returns NULL you just need to
jump to err_mmap_read_lock_failed, like we currently do if
mmap_read_trylock() fails.

I don't think that will be enough as well, as the current code AFAICS does
something meaninfgul when mmap_read_trylock() suceeds but vma_lookup returns
NULL because there's no vma at that address. Now we would just assume the
trylock failed even if the reason was that vma lookup found nothing for the
address. The problem is that lock_vma_under_rcu() can't distinguish those
two outcomes, so we would need something that does?

I spent way too much time staring at this yesterday.

I think the key to distinguishing between:

	vma==NULL because there's no VMA
and
	vma==NULL because of a trylock failure

is binder_alloc_is_mapped(). It won't return false until vm_ops->close()
finishes. vm_ops->close() shouldn't be able to happen while
lock_vma_under_rcu() is held. So if you've got a non-NULL VMA, you've
also got a stable is binder_alloc_is_mapped().

So, if you've got a vma!=NULL *and* binder_alloc_is_mapped()==true, I
think you can be pretty sure you've got the right VMA.

If you have vma==NULL and binder_alloc_is_mapped()==true, you can be
pretty sure that you hit some kind of transient lock_vma_under_rcu()
failure.

I came up with the attached patch. More eyeballs would be welcome.
There's a _lot_ going on here.