linux-kernel - Re: [PATCH v5 4/6] mm: make vma cache SLAB_TYPESAFE_BY

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]

Message-ID: <6b29b2a5-c244-4930-a5a0-1a24a04e7e35@suse.cz>
Date: Tue, 10 Dec 2024 15:21:26 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Suren Baghdasaryan <surenb@...gle.com>, akpm@...ux-foundation.org
Cc: willy@...radead.org, liam.howlett@...cle.com, lorenzo.stoakes@...cle.com,
 mhocko@...e.com, hannes@...xchg.org, mjguzik@...il.com,
 oliver.sang@...el.com, mgorman@...hsingularity.net, david@...hat.com,
 peterx@...hat.com, oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org,
 brauner@...nel.org, dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com,
 minchan@...gle.com, jannh@...gle.com, shakeel.butt@...ux.dev,
 souravpanda@...gle.com, pasha.tatashin@...een.com, corbet@....net,
 linux-doc@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 kernel-team@...roid.com
Subject: Re: [PATCH v5 4/6] mm: make vma cache SLAB_TYPESAFE_BY_RCU

On 12/6/24 23:52, Suren Baghdasaryan wrote:
> To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensure that
> object reuse before RCU grace period is over will be detected inside
> lock_vma_under_rcu().
> lock_vma_under_rcu() enters RCU read section, finds the vma at the
> given address, locks the vma and checks if it got detached or remapped
> to cover a different address range. These last checks are there
> to ensure that the vma was not modified after we found it but before
> locking it.
> vma reuse introduces several new possibilities:
> 1. vma can be reused after it was found but before it is locked;
> 2. vma can be reused and reinitialized (including changing its vm_mm)
> while being locked in vma_start_read();
> 3. vma can be reused and reinitialized after it was found but before
> it is locked, then attached at a new address or to a new mm while
> read-locked;
> For case #1 current checks will help detecting cases when:
> - vma was reused but not yet added into the tree (detached check)
> - vma was reused at a different address range (address check);
> We are missing the check for vm_mm to ensure the reused vma was not
> attached to a different mm. This patch adds the missing check.
> For case #2, we pass mm to vma_start_read() to prevent access to
> unstable vma->vm_mm. This might lead to vma_start_read() returning
> a false locked result but that's not critical if it's rare because
> it will only lead to a retry under mmap_lock.
> For case #3, we ensure the order in which vma->detached flag and
> vm_start/vm_end/vm_mm are set and checked. vma gets attached after
> vm_start/vm_end/vm_mm were set and lock_vma_under_rcu() should check
> vma->detached before checking vm_start/vm_end/vm_mm. This is required
> because attaching vma happens without vma write-lock, as opposed to
> vma detaching, which requires vma write-lock. This patch adds memory
> barriers inside is_vma_detached() and vma_mark_attached() needed to
> order reads and writes to vma->detached vs vm_start/vm_end/vm_mm.
> After these provisions, SLAB_TYPESAFE_BY_RCU is added to vm_area_cachep.
> This will facilitate vm_area_struct reuse and will minimize the number
> of call_rcu() calls.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>

I'm wondering about the vma freeing path. Consider vma_complete():

vma_mark_detached(vp->remove);
  vma->detached = true; - plain write
vm_area_free(vp->remove);
  vma->vm_lock_seq = UINT_MAX; - plain write
  kmem_cache_free(vm_area_cachep)
...
potential reallocation

against:

lock_vma_under_rcu()
- mas_walk finds a stale vma due to race
vma_start_read()
  if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(mm->mm_lock_seq.sequence))
  - can be false, the vma was not being locked on the freeing side?
  down_read_trylock(&vma->vm_lock.lock) - suceeds, wasn't locked
    this is acquire, but was there any release?
  is_vma_detached() - false negative as the write above didn't propagate
    here yet; a read barrier but where is the write barrier?
  checks for vma->vm_mm, vm_start, vm_end - nobody reset them yet so false
    positive, or they got reset on reallocation but writes didn't propagate

Am I missing something that would prevent lock_vma_under_rcu() falsely
succeeding here?