Messages in this thread | | | Date | Thu, 24 Jul 2014 14:14:36 +0400 | From | Andrey Tsyvarev <> | Subject | Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem |
| |
Hi,
With patch skipping invalidating pages for node_inode and meta_inode use-after-free error disappears too.
23.07.2014 7:39, Gu Zheng пишет: > Hi, > On 07/23/2014 10:12 AM, Chao Yu wrote: > >> Hi Andrey Gu, >> >>> -----Original Message----- >>> From: Andrey Tsyvarev [mailto:tsyvarev@ispras.ru] >>> Sent: Tuesday, July 22, 2014 6:04 PM >>> To: Gu Zheng >>> Cc: Jaegeuk Kim; linux-kernel; Alexey Khoroshilov; linux-f2fs-devel@lists.sourceforge.net >>> Subject: Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem >>> >>> Hi Gu, >>> >>>>> Investigation shows, that f2fs_evict_inode, when called for 'meta_inode', uses >>> invalidate_mapping_pages() for 'node_inode'. >>>>> But 'node_inode' is deleted before 'meta_inode' in f2fs_put_super via iput(). >>>>> >>>>> It seems that in common usage scenario this use-after-free is benign, because 'node_inode' >>> remains partially valid data even after kmem_cache_free(). >>>>> But things may change if, while 'meta_inode' is evicted in one f2fs filesystem, another (mounted) >>> f2fs filesystem requests inode from cache, and formely >>>>> 'node_inode' of the first filesystem is returned. >>>> The analysis seems reasonable. Have you tried to swap the reclaim order of node_inde >>>> and meta_inode? >>>> >>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>> index 870fe19..e114418 100644 >>>> --- a/fs/f2fs/super.c >>>> +++ b/fs/f2fs/super.c >>>> @@ -430,8 +430,8 @@ static void f2fs_put_super(struct super_block *sb) >>>> if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES)) >>>> write_checkpoint(sbi, true); >>>> >>>> - iput(sbi->node_inode); >>>> iput(sbi->meta_inode); >>>> + iput(sbi->node_inode); >>>> >>>> /* destroy f2fs internal modules */ >>>> destroy_node_manager(sbi); >>>> >>>> Thanks, >>>> Gu >>> With reclaim order of node_inode and meta_inode swapped, use-after-free >>> error disappears. >>> >>> But shouldn't initialization order of these inodes be swapped too? >>> As meta_inode uses node_inode, it seems logical that it should be >>> initialized after it. > The initialization order dose not affect anything, so swapping the order dose not > make more sense here. > >> IMO, it's not easy to exchange order of initialization between meta_inode and >> node_inode, because we should use meta_inode in get_valid_checkpoint for valid >> cp first for usual verification, then init node_inode. > Yeah, but I think just moving node_inode's initialization to the front of meta_inode > dose not break anything. > >> As I checked, nids for both meta_inode and node_inode are reservation, so it's not >> necessary for us to invalidate pages which will never alloced. >> >> How about skipping it as following? > It seems the right way to fix this issue. > > To Andrey: > Could you please try this one? > > Thanks, > Gu > >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> index 2cf6962..cafba3c 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -273,7 +273,7 @@ void f2fs_evict_inode(struct inode *inode) >> >> if (inode->i_ino == F2FS_NODE_INO(sbi) || >> inode->i_ino == F2FS_META_INO(sbi)) >> - goto no_delete; >> + goto out_clear; >> >> f2fs_bug_on(get_dirty_dents(inode)); >> remove_dirty_dir_inode(inode); >> @@ -295,6 +295,7 @@ void f2fs_evict_inode(struct inode *inode) >> >> sb_end_intwrite(inode->i_sb); >> no_delete: >> - clear_inode(inode); >> invalidate_mapping_pages(NODE_MAPPING(sbi), inode->i_ino, inode->i_ino); >> +out_clear: >> + clear_inode(inode); >> } >> >>> -- >>> Best regards, >>> >>> Andrey Tsyvarev >>> Linux Verification Center, ISPRAS >>> web:http://linuxtesting.org >>> >>> >>> ------------------------------------------------------------------------------ >>> Want fast and easy access to all the code in your enterprise? Index and >>> search up to 200,000 lines of code with a free copy of Black Duck >>> Code Sight - the same software that powers the world's largest code >>> search on Ohloh, the Black Duck Open Hub! Try it now. >>> http://p.sf.net/sfu/bds >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >> . >> > >
-- Best regards,
Andrey Tsyvarev Linux Verification Center, ISPRAS web:http://linuxtesting.org
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |