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

[Jaegeuk Kim]

Thank you guys.
I merged two patches. :)

--
Jaegeuk Kim

2014-07-24 22:49 GMT-07:00 Gu Zheng <guz.fnst@xxxxxxxxxxxxxx>:
> 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: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/