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 > > . > -- 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/