linux-kernel - Re: [PATCH 3/4] mm: replace rw_semaphore with atomic_t in vma

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: <20241112003527.ogtrnknjwvtqfewm@offworld>
Date: Mon, 11 Nov 2024 16:35:27 -0800
From: Davidlohr Bueso <dave@...olabs.net>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, liam.howlett@...cle.com,
	lorenzo.stoakes@...cle.com, mhocko@...e.com, vbabka@...e.cz,
	hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com,
	mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com,
	oleg@...hat.com, 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,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	kernel-team@...roid.com
Subject: Re: [PATCH 3/4] mm: replace rw_semaphore with atomic_t in vma_lock

On Mon, 11 Nov 2024, Suren Baghdasaryan wrote:

>@@ -787,7 +893,10 @@ static inline void vma_start_write(struct vm_area_struct *vma)
>	 * we should use WRITE_ONCE() for cleanliness and to keep KCSAN happy.
>	 */
>	WRITE_ONCE(vma->vm_lock_seq, mm_lock_seq);
>-	up_write(&vma->vm_lock.lock);
>+	/* Write barrier to ensure vm_lock_seq change is visible before count */
>+	smp_wmb();
>+	rwsem_release(&vma->vm_lock.dep_map, _RET_IP_);
>+	atomic_set(&vma->vm_lock.count, VMA_LOCK_UNLOCKED);

Too many barriers here. Just do atomic_set_release and remove that
smp_wmb(). And what you are doing is really ensuring nothing leaks out
of the critical region, so that comment should also be more generic.

I would expect regression testing to catch this sort of thing.

...

> #ifdef CONFIG_PER_VMA_LOCK
>+		struct wait_queue_head vma_writer_wait;

You might want to use rcuwait here instead, which is much more
optimized for the single waiter requirement vmas have.

Thanks,
Davidlohr