Hi, To Andrey: Thanks for your test on this patch!
To Gu: If you do not object, let me make and resend a patch base on the one which skip invalidating pages. Regards, Yu > -----Original Message----- > From: Andrey Tsyvarev [mailto:tsyva...@ispras.ru] > Sent: Thursday, July 24, 2014 6:15 PM > To: Gu Zheng; Chao Yu > Cc: 'Jaegeuk Kim'; 'linux-kernel'; 'Alexey Khoroshilov'; > linux-f2fs-de...@lists.sourceforge.net > 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:tsyva...@ispras.ru] > >>> Sent: Tuesday, July 22, 2014 6:04 PM > >>> To: Gu Zheng > >>> Cc: Jaegeuk Kim; linux-kernel; Alexey Khoroshilov; > >>> linux-f2fs-de...@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-de...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
