Thank you guys. I merged two patches. :) -- Jaegeuk Kim
2014-07-24 22:49 GMT-07:00 Gu Zheng <guz.f...@cn.fujitsu.com>: > On 07/25/2014 11:22 AM, Chao Yu wrote: > >> 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. > > Please go ahead.:) > > Thanks, > Gu > >> >> 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/
