Skip to contentSkip to conversation

f2fs: fix stale ATOMIC_WRITTEN_PAGE private pointer

PATCH In review Jaegeuk Kim 2 participants 5 messages Mar 17, 2017

Subsystems (2)
VFS F2FS file system
lore
Share & tools
lore

Jaegeuk Kim

author maintainer
#0 lore raw

PATCH 1/2 f2fs: fix stale ATOMIC_WRITTEN_PAGE private pointer

When I forced to enable atomic operations intentionally, I could hit the below panic, since we didn't clear page->private in f2fs_invalidate_page called by file truncation.

The panic occurs due to NULL mapping having page->private.

BUG: unable to handle kernel paging request at ffffffffffffffff
IP: drop_buffers+0x38/0xe0
PGD 5d00c067
PUD 5d00e067
PMD 0
CPU: 3 PID: 1648 Comm: fsstress Tainted: G      D    OE   4.10.0+ #5
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
task: ffff9151952863c0 task.stack: ffffaaec40db4000
RIP: 0010:drop_buffers+0x38/0xe0
RSP: 0018:ffffaaec40db74c8 EFLAGS: 00010292
Call Trace:
 ? page_referenced+0x8b/0x170
 try_to_free_buffers+0xc5/0xe0
 try_to_release_page+0x49/0x50
 shrink_page_list+0x8bc/0x9f0
 shrink_inactive_list+0x1dd/0x500
 ? shrink_active_list+0x2c0/0x430
 shrink_node_memcg+0x5eb/0x7c0
 shrink_node+0xe1/0x320
 do_try_to_free_pages+0xef/0x2e0
 try_to_free_pages+0xe9/0x190
 __alloc_pages_slowpath+0x390/0xe70
 __alloc_pages_nodemask+0x291/0x2b0
 alloc_pages_current+0x95/0x140
 __page_cache_alloc+0xc4/0xe0
 pagecache_get_page+0xab/0x2a0
 grab_cache_page_write_begin+0x20/0x40
 get_read_data_page+0x2e6/0x4c0 [f2fs]
 ? f2fs_mark_inode_dirty_sync+0x16/0x30 [f2fs]
 ? truncate_data_blocks_range+0x238/0x2b0 [f2fs]
 get_lock_data_page+0x30/0x190 [f2fs]
 __exchange_data_block+0xaaf/0xf40 [f2fs]
 f2fs_fallocate+0x418/0xd00 [f2fs]
 vfs_fallocate+0x157/0x220
 SyS_fallocate+0x48/0x80
Signed-off-by Yunlei He <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c    |  2 +-
 fs/f2fs/f2fs.h    |  1 +
 fs/f2fs/segment.c | 28 +++++++++++++++++++++-------
 3 files changed, 23 insertions(+), 8 deletions(-)
fs/f2fs/data.c +1 -1
index e07c60fb1033..f4b3006e3ecb 100644
@@ -1967,7 +1967,7 @@ void f2fs_invalidate_page(struct page *page, unsigned int offset,
 
 	/* This is atomic written page, keep Private */
 	if (IS_ATOMIC_WRITTEN_PAGE(page))
-		return;
+		drop_inmem_page(inode, page);
 
 	set_page_private(page, 0);
 	ClearPagePrivate(page);
fs/f2fs/f2fs.h +1 -0
index e7b1a6c30d9e..9a06d0720351 100644
@@ -2173,6 +2173,7 @@ void destroy_node_manager_caches(void);
  */
 void register_inmem_page(struct inode *inode, struct page *page);
 void drop_inmem_pages(struct inode *inode);
+void drop_inmem_page(struct inode *inode, struct page *page);
 int commit_inmem_pages(struct inode *inode);
 void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need);
 void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi);
fs/f2fs/segment.c +21 -7
index 42727e40d17c..3e8d08722151 100644
@@ -192,7 +192,7 @@ void register_inmem_page(struct inode *inode, struct page *page)
 	trace_f2fs_register_inmem_page(page, INMEM);
 }
 
-static int __revoke_inmem_pages(struct inode *inode,
+static int __revoke_inmem_pages(struct inode *inode, struct page *p,
 				struct list_head *head, bool drop, bool recover)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);

@@ -205,7 +205,10 @@ static int __revoke_inmem_pages(struct inode *inode,
 		if (drop)
 			trace_f2fs_commit_inmem_page(page, INMEM_DROP);
 
-		lock_page(page);
+		if (!p)
+			lock_page(page);
+		else if (p != page)
+			continue;
 
 		if (recover) {
 			struct dnode_of_data dn;

@@ -229,7 +232,7 @@ static int __revoke_inmem_pages(struct inode *inode,
 			ClearPageUptodate(page);
 		set_page_private(page, 0);
 		ClearPagePrivate(page);
-		f2fs_put_page(page, 1);
+		f2fs_put_page(page, p ? 0 : 1);
 
 		list_del(&cur->list);
 		kmem_cache_free(inmem_entry_slab, cur);

@@ -243,13 +246,22 @@ void drop_inmem_pages(struct inode *inode)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 
 	mutex_lock(&fi->inmem_lock);
-	__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
+	__revoke_inmem_pages(inode, NULL, &fi->inmem_pages, true, false);
 	mutex_unlock(&fi->inmem_lock);
 
 	clear_inode_flag(inode, FI_ATOMIC_FILE);
 	stat_dec_atomic_write(inode);
 }
 
+void drop_inmem_page(struct inode *inode, struct page *page)
+{
+	struct f2fs_inode_info *fi = F2FS_I(inode);
+
+	mutex_lock(&fi->inmem_lock);
+	__revoke_inmem_pages(inode, page, &fi->inmem_pages, true, false);
+	mutex_unlock(&fi->inmem_lock);
+}
+
 static int __commit_inmem_pages(struct inode *inode,
 					struct list_head *revoke_list)
 {

@@ -300,7 +312,7 @@ static int __commit_inmem_pages(struct inode *inode,
 							DATA, WRITE);
 
 	if (!err)
-		__revoke_inmem_pages(inode, revoke_list, false, false);
+		__revoke_inmem_pages(inode, NULL, revoke_list, false, false);
 
 	return err;
 }

@@ -330,12 +342,14 @@ int commit_inmem_pages(struct inode *inode)
 		 * recovery or rewrite & commit last transaction. For other
 		 * error number, revoking was done by filesystem itself.
 		 */
-		ret = __revoke_inmem_pages(inode, &revoke_list, false, true);
+		ret = __revoke_inmem_pages(inode, NULL, &revoke_list,
+							false, true);
 		if (ret)
 			err = ret;
 
 		/* drop all uncommitted pages */
-		__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
+		__revoke_inmem_pages(inode, NULL, &fi->inmem_pages,
+							true, false);
 	}
 	mutex_unlock(&fi->inmem_lock);
Signature · 6 lines
2.11.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
author maintainer
#1 lore raw

PATCH 2/2 f2fs: don't allow atomic writes for not regular files

The atomic writes only supports regular files for database.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)
fs/f2fs/file.c +3 -0
index da6d33d1bb34..d486e02b43c2 100644
@@ -1522,6 +1522,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	if (f2fs_is_atomic_file(inode))
 		goto out;
 
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+
 	ret = f2fs_convert_inline_inode(inode);
 	if (ret)
 		goto out;
Signature · 6 lines
2.11.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
#2 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: don't allow atomic writes for not regular files

Quoted 12 lines · Jaegeuk Kim

The atomic writes only supports regular files for database.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)
fs/f2fs/file.c +0 -0
index da6d33d1bb34..d486e02b43c2 100644

fs/f2fs/file.c static int f2fs_ioc_start_atomic_write(struct file *filp) · line 1526

Quoted 5 lines
fs/f2fs/file.c +2 -0
@@ -1522,6 +1522,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	if (f2fs_is_atomic_file(inode))
 		goto out;
 
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;

goto out;

Thanks,

Quoted 5 lines
fs/f2fs/file.c +1 -0
+
 	ret = f2fs_convert_inline_inode(inode);
 	if (ret)
 		goto out;
author maintainer
#3 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: don't allow atomic writes for not regular files

Quoted 16 lines · Chao Yu
Quoted 15 lines · Jaegeuk Kim

The atomic writes only supports regular files for database.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)
fs/f2fs/file.c +0 -0
index da6d33d1bb34..d486e02b43c2 100644
@@ -1522,6 +1522,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	if (f2fs_is_atomic_file(inode))
 		goto out;
Quoted 4 lines

fs/f2fs/file.c static int f2fs_ioc_start_atomic_write(struct file *filp) · line 1526

Quoted 2 lines
fs/f2fs/file.c +2 -0
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;

goto out;

Oh, I just move it to the beginning of the function.

Thanks,

Quoted 8 lines

Thanks,

Quoted 5 lines
fs/f2fs/file.c +1 -0
+
 	ret = f2fs_convert_inline_inode(inode);
 	if (ret)
 		goto out;

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Reviewed-by on v1 1/2
#4 lore raw
In reply to Jaegeuk Kim
Quoted 42 lines · Jaegeuk Kim

When I forced to enable atomic operations intentionally, I could hit the below panic, since we didn't clear page->private in f2fs_invalidate_page called by file truncation.

The panic occurs due to NULL mapping having page->private.

BUG: unable to handle kernel paging request at ffffffffffffffff
IP: drop_buffers+0x38/0xe0
PGD 5d00c067
PUD 5d00e067
PMD 0
CPU: 3 PID: 1648 Comm: fsstress Tainted: G      D    OE   4.10.0+ #5
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
task: ffff9151952863c0 task.stack: ffffaaec40db4000
RIP: 0010:drop_buffers+0x38/0xe0
RSP: 0018:ffffaaec40db74c8 EFLAGS: 00010292
Call Trace:
 ? page_referenced+0x8b/0x170
 try_to_free_buffers+0xc5/0xe0
 try_to_release_page+0x49/0x50
 shrink_page_list+0x8bc/0x9f0
 shrink_inactive_list+0x1dd/0x500
 ? shrink_active_list+0x2c0/0x430
 shrink_node_memcg+0x5eb/0x7c0
 shrink_node+0xe1/0x320
 do_try_to_free_pages+0xef/0x2e0
 try_to_free_pages+0xe9/0x190
 __alloc_pages_slowpath+0x390/0xe70
 __alloc_pages_nodemask+0x291/0x2b0
 alloc_pages_current+0x95/0x140
 __page_cache_alloc+0xc4/0xe0
 pagecache_get_page+0xab/0x2a0
 grab_cache_page_write_begin+0x20/0x40
 get_read_data_page+0x2e6/0x4c0 [f2fs]
 ? f2fs_mark_inode_dirty_sync+0x16/0x30 [f2fs]
 ? truncate_data_blocks_range+0x238/0x2b0 [f2fs]
 get_lock_data_page+0x30/0x190 [f2fs]
 __exchange_data_block+0xaaf/0xf40 [f2fs]
 f2fs_fallocate+0x418/0xd00 [f2fs]
 vfs_fallocate+0x157/0x220
 SyS_fallocate+0x48/0x80
Quoted 2 lines
Signed-off-by Yunlei He <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
Reviewed-by Chao Yu <[email protected]>