Messages in this thread Patch in this message | | | From | Chao Yu <> | Subject | RE: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem | Date | Wed, 23 Jul 2014 10:12:52 +0800 |
| |
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.
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.
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?
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
| |