From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f70.google.com (mail-it0-f70.google.com [209.85.214.70]) by kanga.kvack.org (Postfix) with ESMTP id EC4DC6B0005 for ; Fri, 15 Jul 2016 04:52:02 -0400 (EDT) Received: by mail-it0-f70.google.com with SMTP id d65so28274820ith.1 for ; Fri, 15 Jul 2016 01:52:02 -0700 (PDT) Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0114.outbound.protection.outlook.com. [104.47.2.114]) by mx.google.com with ESMTPS id 190si5913932oib.247.2016.07.15.01.52.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 15 Jul 2016 01:52:02 -0700 (PDT) Subject: Re: [PATCH] radix-tree: fix radix_tree_iter_retry() for tagged iterators. References: <1468495196-10604-1-git-send-email-aryabinin@virtuozzo.com> <20160714222527.GA26136@linux.intel.com> From: Andrey Ryabinin Message-ID: <5788A46A.70106@virtuozzo.com> Date: Fri, 15 Jul 2016 11:52:58 +0300 MIME-Version: 1.0 In-Reply-To: <20160714222527.GA26136@linux.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Ross Zwisler Cc: Andrew Morton , Jan Kara , "Kirill A. Shutemov" , linux-mm@kvack.org, Greg Thelen , Suleiman Souhlal , syzkaller@googlegroups.com, Kostya Serebryany , Alexander Potapenko , Sasha Levin , linux-kernel@vger.kernel.org, Konstantin Khlebnikov , Matthew Wilcox , Hugh Dickins , stable@vger.kernel.org On 07/15/2016 01:25 AM, Ross Zwisler wrote: > On Thu, Jul 14, 2016 at 02:19:56PM +0300, Andrey Ryabinin wrote: >> radix_tree_iter_retry() resets slot to NULL, but it doesn't reset tags. >> Then NULL slot and non-zero iter.tags passed to radix_tree_next_slot() >> leading to crash: >> >> RIP: [< inline >] radix_tree_next_slot include/linux/radix-tree.h:473 >> [] find_get_pages_tag+0x334/0x930 mm/filemap.c:1452 >> .... >> Call Trace: >> [] pagevec_lookup_tag+0x3a/0x80 mm/swap.c:960 >> [] mpage_prepare_extent_to_map+0x321/0xa90 fs/ext4/inode.c:2516 >> [] ext4_writepages+0x10be/0x2b20 fs/ext4/inode.c:2736 >> [] do_writepages+0x97/0x100 mm/page-writeback.c:2364 >> [] __filemap_fdatawrite_range+0x248/0x2e0 mm/filemap.c:300 >> [] filemap_write_and_wait_range+0x121/0x1b0 mm/filemap.c:490 >> [] ext4_sync_file+0x34d/0xdb0 fs/ext4/fsync.c:115 >> [] vfs_fsync_range+0x10a/0x250 fs/sync.c:195 >> [< inline >] vfs_fsync fs/sync.c:209 >> [] do_fsync+0x42/0x70 fs/sync.c:219 >> [< inline >] SYSC_fdatasync fs/sync.c:232 >> [] SyS_fdatasync+0x19/0x20 fs/sync.c:230 >> [] entry_SYSCALL_64_fastpath+0x23/0xc1 arch/x86/entry/entry_64.S:207 >> >> We must reset iterator's tags to bail out from radix_tree_next_slot() and >> go to the slow-path in radix_tree_next_chunk(). > > This analysis doesn't make sense to me. In find_get_pages_tag(), when we call > radix_tree_iter_retry(), this sets the local 'slot' variable to NULL, then > does a 'continue'. This will hop to the next iteration of the > radix_tree_for_each_tagged() loop, which will very check the exit condition of > the for() loop: > > #define radix_tree_for_each_tagged(slot, root, iter, start, tag) \ > for (slot = radix_tree_iter_init(iter, start) ; \ > slot || (slot = radix_tree_next_chunk(root, iter, \ > RADIX_TREE_ITER_TAGGED | tag)) ; \ > slot = radix_tree_next_slot(slot, iter, \ > RADIX_TREE_ITER_TAGGED)) > > So, we'll run the > slot || (slot = radix_tree_next_chunk(root, iter, \ > RADIX_TREE_ITER_TAGGED | tag)) ; \ > > bit first. This is not the way how the for() loop works. slot = radix_tree_next_slot() executed first and only after that goes the condition statement. > 'slot' is NULL, so we'll set it via radix_tree_next_chunk(). At > this point radix_tree_next_slot() hasn't been called. > > radix_tree_next_chunk() will set up the iter->index, iter->next_index and > iter->tags before it returns. The next iteration of the loop in > find_get_pages_tag() will use the non-NULL slot provided by > radix_tree_next_chunk(), and only after that iteration will we call > radix_tree_next_slot() again. By then iter->tags should be up to date. > > Do you have a test setup that reliably fails without this code but passes when > you zero out iter->tags? > Yup, I run Dmitry's reproducer in a parallel loop: $ while true; do ./a.out & done Usually it takes just couple minutes maximum. > I've been looking at this as well, but haven't been able to get a reliable > reproducer in my test setup. > >> Fixes: 46437f9a554f ("radix-tree: fix race in gang lookup") >> Signed-off-by: Andrey Ryabinin >> Reported-by: Dmitry Vyukov >> Cc: Konstantin Khlebnikov >> Cc: Matthew Wilcox >> Cc: Hugh Dickins >> Cc: >> --- >> include/linux/radix-tree.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h >> index cb4b7e8..eca6f62 100644 >> --- a/include/linux/radix-tree.h >> +++ b/include/linux/radix-tree.h >> @@ -407,6 +407,7 @@ static inline __must_check >> void **radix_tree_iter_retry(struct radix_tree_iter *iter) >> { >> iter->next_index = iter->index; >> + iter->tags = 0; >> return NULL; >> } >> >> -- >> 2.7.3 >> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org