Re: [PATCH v2] locking/rwsem: add acquire barrier to read_slowpath exit when queue is empty

[Waiman Long]

On 7/17/19 9:13 AM, Will Deacon wrote:
> On Wed, Jul 17, 2019 at 02:02:20PM +0200, Jan Stancek wrote:
>> LTP mtest06 has been observed to rarely hit "still mapped when deleted"
>> and following BUG_ON on arm64:
>>   page:ffff7e02fa37e480 refcount:3 mapcount:1 mapping:ffff80be3d678ab0 index:0x0
>>   xfs_address_space_operations [xfs]
>>   flags: 0xbfffe000000037(locked|referenced|uptodate|lru|active)
>>   page dumped because: VM_BUG_ON_PAGE(page_mapped(page))
>>   ------------[ cut here ]------------
>>   kernel BUG at mm/filemap.c:171!
>>   Internal error: Oops - BUG: 0 [#1] SMP
>>   CPU: 220 PID: 154292 Comm: mmap1 Not tainted 5.2.0-0ecfebd.cki #1
>>   Hardware name: HPE Apollo 70 /C01_APACHE_MB , BIOS L50_5.13_1.10 05/17/2019
>>   pstate: 40400089 (nZcv daIf +PAN -UAO)
>>   pc : unaccount_page_cache_page+0x17c/0x1a0
>>   lr : unaccount_page_cache_page+0x17c/0x1a0
>>   Call trace:
>>   unaccount_page_cache_page+0x17c/0x1a0
>>   delete_from_page_cache_batch+0xa0/0x300
>>   truncate_inode_pages_range+0x1b8/0x640
>>   truncate_inode_pages_final+0x88/0xa8
>>   evict+0x1a0/0x1d8
>>   iput+0x150/0x240
>>   dentry_unlink_inode+0x120/0x130
>>   __dentry_kill+0xd8/0x1d0
>>   dentry_kill+0x88/0x248
>>   dput+0x168/0x1b8
>>   __fput+0xe8/0x208
>>   ____fput+0x20/0x30
>>   task_work_run+0xc0/0xf0
>>   do_notify_resume+0x2b0/0x328
>>   work_pending+0x8/0x10
>>
>> The extra mapcount originated from pagefault handler, which handled
>> pagefault for vma that has already been detached. vma is detached
>> under mmap_sem write lock by detach_vmas_to_be_unmapped(), which
>> also invalidates vmacache.
>>
>> When pagefault handler (under mmap_sem read lock) called find_vma(),
>> vmacache_valid() wrongly reported vmacache as valid.
>>
>> After rwsem down_read() returns via 'queue empty' path (as of v5.2),
>> it does so without issuing read_acquire on sem->count:
>>   down_read
>>     __down_read
>>       rwsem_down_read_failed
>>         __rwsem_down_read_failed_common
>>           raw_spin_lock_irq(&sem->wait_lock);
>>           if (list_empty(&sem->wait_list)) {
>>             if (atomic_long_read(&sem->count) >= 0) {
>>               raw_spin_unlock_irq(&sem->wait_lock);
>>               return sem;
>>
>> Suspected problem here is that last *_acquire on down_read() side
>> happens before write side issues *_release:
>>   1. writer: has the lock
>>   2. reader: down_read() issues *read_acquire on entry
>>   3. writer: mm->vmacache_seqnum++; downgrades lock (*fetch_add_release)
>>   4. reader: __rwsem_down_read_failed_common() finds it can take lock and returns
>>   5. reader: observes stale mm->vmacache_seqnum
>>
>> I can reproduce the problem by running LTP mtest06 in a loop and building
>> kernel (-j $NCPUS) in parallel. It does reproduce since v4.20 up to v5.2
>> on arm64 HPE Apollo 70 (224 CPUs, 256GB RAM, 2 nodes). It triggers reliably
>> within ~hour. Patched kernel ran fine for 10+ hours with clean dmesg.
>> Tests were done against v5.2, since commit cf69482d62d9 ("locking/rwsem:
>> Enable readers spinning on writer") makes it much harder to reproduce.
>>
>> v2: Move barrier after test (Waiman Long)
>>     Use smp_acquire__after_ctrl_dep() (Peter Zijlstra)
>>
>> Related: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/mtest06/mmap1.c
>> Related: commit dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
>> Fixes: 4b486b535c33 ("locking/rwsem: Exit read lock slowpath if queue empty & no writer")
>>
>> Signed-off-by: Jan Stancek <jstancek@xxxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx # v4.20+
>> Cc: Waiman Long <longman@xxxxxxxxxx>
>> Cc: Davidlohr Bueso <dbueso@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> ---
>>  kernel/locking/rwsem.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 37524a47f002..5ac72b60608b 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -1032,6 +1032,7 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
>>  		 */
>>  		if (adjustment && !(atomic_long_read(&sem->count) &
>>  		     (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
>> +			smp_acquire__after_ctrl_dep();
>>  			raw_spin_unlock_irq(&sem->wait_lock);
>>  			rwsem_set_reader_owned(sem);
>>  			lockevent_inc(rwsem_rlock_fast);
> If you add a comment to the code outlining the issue (preferably as a litmus
> test involving sem->count and some shared data which happens to be
> vmacache_seqnum in your test)), then:
>
> Reviewed-by: Will Deacon <will@xxxxxxxxxx>
>
> Thanks,
>
> Will

Agreed. A comment just above smp_acquire__after_ctrl_dep() on why this
is needed will be great.

Other than that,

Acked-by: Waiman Long <longman@xxxxxxxxxx>