RE: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem

[Chao Yu]

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:tsyvarev@xxxxxxxxx]
> Sent: Thursday, July 24, 2014 6:15 PM
> To: Gu Zheng; Chao Yu
> Cc: 'Jaegeuk Kim'; 'linux-kernel'; 'Alexey Khoroshilov';
> linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> 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:tsyvarev@xxxxxxxxx]
> >>> Sent: Tuesday, July 22, 2014 6:04 PM
> >>> To: Gu Zheng
> >>> Cc: Jaegeuk Kim; linux-kernel; Alexey Khoroshilov; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> >>> 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@xxxxxxxxxxxxxxxxxxxxx
> >>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/