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

From: Andrey Tsyvarev
Date: Thu Jul 24 2014 - 06:14:13 EST


With patch skipping invalidating pages for node_inode and meta_inode use-after-free error disappears too.

23.07.2014 7:39, Gu Zheng ÐÐÑÐÑ:
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->node_inode);

/* destroy f2fs internal modules */

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?


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;
@@ -295,6 +295,7 @@ void f2fs_evict_inode(struct inode *inode)
- clear_inode(inode);
invalidate_mapping_pages(NODE_MAPPING(sbi), inode->i_ino, inode->i_ino);
+ clear_inode(inode);

Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS

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.
Linux-f2fs-devel mailing list

Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at