linux-kernel - Re: [PATCH] f2fs: fix to keep backward compatibility of flexible inline xattr feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171026103728.GA62112@jaegeuk-macbookpro.roam.corp.google.com>
Date:   Thu, 26 Oct 2017 12:37:28 +0200
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     Chao Yu <yuchao0@...wei.com>
Cc:     linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, chao@...nel.org
Subject: Re: [PATCH] f2fs: fix to keep backward compatibility of flexible
 inline xattr feature

On 10/26, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 10/26, Jaegeuk Kim wrote:
> > On 10/26, Chao Yu wrote:
> > > Hi Jaegeuk,
> > > 
> > > On 2017/10/26 16:42, Jaegeuk Kim wrote:
> > > > Hi Chao,
> > > > 
> > > > It seems this is a critical problem, so let me integrate this patch with your
> > > > initial patch "f2fs: support flexible inline xattr size".
> > > > Let me know, if you have any other concern.
> > > 
> > > Better. ;)
> > > 
> > > Please add commit message of this patch into initial patch "f2fs: support
> > > flexible inline xattr size".
> 
> BTW, I read the patch again, and couldn't catch the problem actually.
> We didn't assign inline_xattr all the time, instead do if inline_xattr
> is set. Have you done some tests with this? I'm failing tests with these
> changes.

Could you take a look at this change?

---
 fs/f2fs/f2fs.h  |  1 -
 fs/f2fs/inode.c | 20 ++++++--------------
 fs/f2fs/namei.c | 16 ++++++++++------
 3 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index f7fb295..97358d7 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2373,7 +2373,6 @@ static inline int get_extra_isize(struct inode *inode)
 static inline int f2fs_sb_has_flexible_inline_xattr(struct super_block *sb);
 static inline int get_inline_xattr_addrs(struct inode *inode)
 {
-
 	return F2FS_I(inode)->i_inline_xattr_size;
 }
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index cef2f65..eb9a21e 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -233,21 +233,14 @@ static int do_read_inode(struct inode *inode)
 	fi->i_extra_isize = f2fs_has_extra_attr(inode) ?
 					le16_to_cpu(ri->i_extra_isize) : 0;
 
-	/*
-	 * Previously, we will always reserve DEFAULT_INLINE_XATTR_ADDRS size
-	 * space for inline xattr datas, if inline xattr is not enabled, we
-	 * can expect all zero in reserved area, so for regular or symlink,
-	 * it will be safe to reuse reserved area, but for directory, we
-	 * should keep the reservation for stablizing directory structure.
-	 */
-	if (f2fs_has_extra_attr(inode) &&
-		f2fs_sb_has_flexible_inline_xattr(sbi->sb))
+	if (f2fs_sb_has_flexible_inline_xattr(sbi->sb)) {
+		f2fs_bug_on(sbi, !f2fs_has_extra_attr(inode));
 		fi->i_inline_xattr_size = le16_to_cpu(ri->i_inline_xattr_size);
-	else if (!f2fs_has_inline_xattr(inode) &&
-		(S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode)))
+	} else if (!f2fs_has_inline_xattr(inode)) {
 		fi->i_inline_xattr_size = 0;
-	else
+	} else {
 		fi->i_inline_xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
+	}
 
 	/* check data exist */
 	if (f2fs_has_inline_data(inode) && !f2fs_exist_data(inode))
@@ -401,8 +394,7 @@ int update_inode(struct inode *inode, struct page *node_page)
 	if (f2fs_has_extra_attr(inode)) {
 		ri->i_extra_isize = cpu_to_le16(F2FS_I(inode)->i_extra_isize);
 
-		if (f2fs_sb_has_flexible_inline_xattr(F2FS_I_SB(inode)->sb) &&
-			f2fs_has_inline_xattr(inode))
+		if (f2fs_sb_has_flexible_inline_xattr(F2FS_I_SB(inode)->sb))
 			ri->i_inline_xattr_size =
 				cpu_to_le16(F2FS_I(inode)->i_inline_xattr_size);
 
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index e8fdd5e..7ec86d3 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -29,6 +29,7 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 	nid_t ino;
 	struct inode *inode;
 	bool nid_free = false;
+	int xattr_size = 0;
 	int err;
 
 	inode = new_inode(dir->i_sb);
@@ -87,12 +88,15 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 	if (test_opt(sbi, INLINE_XATTR))
 		set_inode_flag(inode, FI_INLINE_XATTR);
 
-	if (f2fs_sb_has_extra_attr(sbi->sb) &&
-		f2fs_sb_has_flexible_inline_xattr(sbi->sb) &&
-		f2fs_has_inline_xattr(inode))
-		F2FS_I(inode)->i_inline_xattr_size = sbi->inline_xattr_size;
-	else
-		F2FS_I(inode)->i_inline_xattr_size = 0;
+	if (f2fs_has_inline_xattr(inode)) {
+		if (f2fs_sb_has_flexible_inline_xattr(sbi->sb)) {
+			f2fs_bug_on(sbi, !f2fs_has_extra_attr(inode));
+			xattr_size = sbi->inline_xattr_size;
+		} else {
+			xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
+		}
+	}
+	F2FS_I(inode)->i_inline_xattr_size = xattr_size;
 
 	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
 		set_inode_flag(inode, FI_INLINE_DATA);
-- 
2.7.4