Messages in this thread | | | Date | Wed, 23 Jul 2014 11:39:13 +0800 | From | Gu Zheng <> | Subject | Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem |
| |
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 > > . >
| |