[PATCH 0/4] Assorted UBIFS fixes

[PATCH 0/4] Assorted UBIFS fixes

[Richard Weinberger]

I'm currently hunting down a filesystem corruption, while reviewing
various parts of UBIFS I've found some other bugs.
This patches fix these bugs. In another series I'll add a feature to
be able to remove stale fscrypt contexts and wrong directory size counters.

Richard Weinberger (4):
  ubifs: Correctly set inode size in ubifs_link()
  ubifs: Don't add fscrypt context to xattrs
  ubifs: Update directory size when creating whiteouts
  ubifs: Harden ubifs_jnl_write_inode()

 fs/ubifs/dir.c     | 31 ++++++++++++++++++++-----------
 fs/ubifs/journal.c |  6 +++++-
 fs/ubifs/ubifs.h   |  2 +-
 fs/ubifs/xattr.c   |  2 +-
 4 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 1/4] ubifs: Correctly set inode size in ubifs_link()

[Richard Weinberger]

We need to use fscrypt filename handling wrappers
when calculating the size of a directory entry, otherwise
UBIFS will report the wrong value (size of plain instead of cihper text)
for st_size of a directory if fscrypt is enabled.

Cc: stable@vger.kernel.org
Fixes: f4f61d2cc6d8 ("ubifs: Implement encrypted filenames")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 9a6b8660425a..04912dedca48 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -693,7 +693,7 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 	struct inode *inode = d_inode(old_dentry);
 	struct ubifs_inode *ui = ubifs_inode(inode);
 	struct ubifs_inode *dir_ui = ubifs_inode(dir);
-	int err, sz_change = CALC_DENT_SIZE(dentry->d_name.len);
+	int err, sz_change;
 	struct ubifs_budget_req req = { .new_dent = 1, .dirtied_ino = 2,
 				.dirtied_ino_d = ALIGN(ui->data_len, 8) };
 	struct fscrypt_name nm;
@@ -731,6 +731,8 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 	if (inode->i_nlink == 0)
 		ubifs_delete_orphan(c, inode->i_ino);
 
+	sz_change = CALC_DENT_SIZE(fname_len(&nm));
+
 	inc_nlink(inode);
 	ihold(inode);
 	inode->i_ctime = current_time(inode);
-- 
2.26.2

Re: [PATCH 1/4] ubifs: Correctly set inode size in ubifs_link()

[Zhihao Cheng]

在 2021/1/23 5:22, Richard Weinberger 写道:
Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 9a6b8660425a..04912dedca48 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -693,7 +693,7 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>   	struct inode *inode = d_inode(old_dentry);
>   	struct ubifs_inode *ui = ubifs_inode(inode);
>   	struct ubifs_inode *dir_ui = ubifs_inode(dir);
> -	int err, sz_change = CALC_DENT_SIZE(dentry->d_name.len);
> +	int err, sz_change;
>   	struct ubifs_budget_req req = { .new_dent = 1, .dirtied_ino = 2,
>   				.dirtied_ino_d = ALIGN(ui->data_len, 8) };
>   	struct fscrypt_name nm;
> @@ -731,6 +731,8 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>   	if (inode->i_nlink == 0)
>   		ubifs_delete_orphan(c, inode->i_ino);
>   
> +	sz_change = CALC_DENT_SIZE(fname_len(&nm));
> +
>   	inc_nlink(inode);
>   	ihold(inode);
>   	inode->i_ctime = current_time(inode);
> 

[PATCH 2/4] ubifs: Don't add fscrypt context to xattrs

[Richard Weinberger]

In UBIFS xattrs are inodes too, make sure that these inodes
don't get a fscrypt context.
Otherwise we will end up with undeletable xattrs which will
waste memory on the flash.

Cc: stable@vger.kernel.org
Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c   | 23 +++++++++++++----------
 fs/ubifs/ubifs.h |  2 +-
 fs/ubifs/xattr.c |  2 +-
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 04912dedca48..8a34e0118ee9 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -68,13 +68,14 @@ static int inherit_flags(const struct inode *dir, umode_t mode)
  * @c: UBIFS file-system description object
  * @dir: parent directory inode
  * @mode: inode mode flags
+ * @omit_fscrypt_ctx: Don't create a fscrypt context for this inode
  *
  * This function finds an unused inode number, allocates new inode and
  * initializes it. Returns new inode in case of success and an error code in
  * case of failure.
  */
 struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
-			      umode_t mode)
+			      umode_t mode, int omit_fscrypt_ctx)
 {
 	int err;
 	struct inode *inode;
@@ -99,10 +100,12 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 			 current_time(inode);
 	inode->i_mapping->nrpages = 0;
 
-	err = fscrypt_prepare_new_inode(dir, inode, &encrypted);
-	if (err) {
-		ubifs_err(c, "fscrypt_prepare_new_inode failed: %i", err);
-		goto out_iput;
+	if (!omit_fscrypt_ctx) {
+		err = fscrypt_prepare_new_inode(dir, inode, &encrypted);
+		if (err) {
+			ubifs_err(c, "fscrypt_prepare_new_inode failed: %i", err);
+			goto out_iput;
+		}
 	}
 
 	switch (mode & S_IFMT) {
@@ -309,7 +312,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 
 	sz_change = CALC_DENT_SIZE(fname_len(&nm));
 
-	inode = ubifs_new_inode(c, dir, mode);
+	inode = ubifs_new_inode(c, dir, mode, 0);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
 		goto out_fname;
@@ -385,7 +388,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
 		return err;
 	}
 
-	inode = ubifs_new_inode(c, dir, mode);
+	inode = ubifs_new_inode(c, dir, mode, 0);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
 		goto out_budg;
@@ -971,7 +974,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 
 	sz_change = CALC_DENT_SIZE(fname_len(&nm));
 
-	inode = ubifs_new_inode(c, dir, S_IFDIR | mode);
+	inode = ubifs_new_inode(c, dir, S_IFDIR | mode, 0);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
 		goto out_fname;
@@ -1058,7 +1061,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 
 	sz_change = CALC_DENT_SIZE(fname_len(&nm));
 
-	inode = ubifs_new_inode(c, dir, mode);
+	inode = ubifs_new_inode(c, dir, mode, 0);
 	if (IS_ERR(inode)) {
 		kfree(dev);
 		err = PTR_ERR(inode);
@@ -1140,7 +1143,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 
 	sz_change = CALC_DENT_SIZE(fname_len(&nm));
 
-	inode = ubifs_new_inode(c, dir, S_IFLNK | S_IRWXUGO);
+	inode = ubifs_new_inode(c, dir, S_IFLNK | S_IRWXUGO, 0);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
 		goto out_fname;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index fc2cdde3b549..dc9b6ba41e77 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1994,7 +1994,7 @@ int ubifs_update_time(struct inode *inode, struct timespec64 *time, int flags);
 
 /* dir.c */
 struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
-			      umode_t mode);
+			      umode_t mode, int omit_fscrypt_ctx);
 int ubifs_getattr(const struct path *path, struct kstat *stat,
 		  u32 request_mask, unsigned int flags);
 int ubifs_check_dir_empty(struct inode *dir);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index a0b9b349efe6..430abe9e56af 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -110,7 +110,7 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 	if (err)
 		return err;
 
-	inode = ubifs_new_inode(c, host, S_IFREG | S_IRWXUGO);
+	inode = ubifs_new_inode(c, host, S_IFREG | S_IRWXUGO, 1);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
 		goto out_budg;
-- 
2.26.2

[PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Richard Weinberger]

Although whiteouts are unlinked files they will get re-linked later,
therefore the size of the parent directory needs to be updated too.

Cc: stable@vger.kernel.org
Fixes: 9e0a1fff8db5 ("ubifs: Implement RENAME_WHITEOUT")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 8a34e0118ee9..b5d523e5854f 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -361,6 +361,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
 	struct ubifs_budget_req ino_req = { .dirtied_ino = 1 };
 	struct ubifs_inode *ui, *dir_ui = ubifs_inode(dir);
 	int err, instantiated = 0;
+	int sz_change = 0;
 	struct fscrypt_name nm;
 
 	/*
@@ -411,6 +412,8 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
 		mark_inode_dirty(inode);
 		drop_nlink(inode);
 		*whiteout = inode;
+		sz_change = CALC_DENT_SIZE(fname_len(&nm));
+		dir->i_size += sz_change;
 	} else {
 		d_tmpfile(dentry, inode);
 	}
@@ -430,6 +433,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
 	return 0;
 
 out_cancel:
+	dir->i_size -= sz_change;
 	mutex_unlock(&dir_ui->ui_mutex);
 out_inode:
 	make_bad_inode(inode);
-- 
2.26.2

Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Zhihao Cheng]

在 2021/1/23 5:22, Richard Weinberger 写道:
> Although whiteouts are unlinked files they will get re-linked later,
I just want to make sure, is this where the count is increased?
do_rename -> inc_nlink(whiteout)
> therefore the size of the parent directory needs to be updated too.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9e0a1fff8db5 ("ubifs: Implement RENAME_WHITEOUT")
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>   fs/ubifs/dir.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 8a34e0118ee9..b5d523e5854f 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -361,6 +361,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>   	struct ubifs_budget_req ino_req = { .dirtied_ino = 1 };
>   	struct ubifs_inode *ui, *dir_ui = ubifs_inode(dir);
>   	int err, instantiated = 0;
> +	int sz_change = 0;
>   	struct fscrypt_name nm;
>   
>   	/*
> @@ -411,6 +412,8 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>   		mark_inode_dirty(inode);
>   		drop_nlink(inode);
>   		*whiteout = inode;
> +		sz_change = CALC_DENT_SIZE(fname_len(&nm));
> +		dir->i_size += sz_change;
>   	} else {
>   		d_tmpfile(dentry, inode);
>   	}
> @@ -430,6 +433,7 @@ static int do_tmpfile(struct inode *dir, struct dentry *dentry,
>   	return 0;
>   
>   out_cancel:
Does this need a judgment? Like this,
if (whiteout)
     dir->i_size -= sz_change;
> +	dir->i_size -= sz_change;
>   	mutex_unlock(&dir_ui->ui_mutex);
>   out_inode:
>   	make_bad_inode(inode);
> 

Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
> An: "richard" <richard@nod.at>, "linux-mtd" <linux-mtd@lists.infradead.org>
> CC: "david" <david@sigma-star.at>, "linux-kernel" <linux-kernel@vger.kernel.org>, "stable" <stable@vger.kernel.org>
> Gesendet: Samstag, 23. Januar 2021 03:45:15
> Betreff: Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

> 在 2021/1/23 5:22, Richard Weinberger 写道:
>> Although whiteouts are unlinked files they will get re-linked later,
> I just want to make sure, is this where the count is increased?
> do_rename -> inc_nlink(whiteout)

Exactly. The logic is a little wicked, I agree.
Let me think again whether there isn't a better way to address 
the problem.

Thanks,
//richard

P.s: Thanks a lot for reviewing!

Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Zhihao Cheng]

在 2021/1/23 10:45, Zhihao Cheng 写道:

>> @@ -430,6 +433,7 @@ static int do_tmpfile(struct inode *dir, struct 
>> dentry *dentry,
>>       return 0;
>>   out_cancel:
Still one question:
> Does this need a judgment? Like this,
> if (whiteout)
>      dir->i_size -= sz_change;

>> +    dir->i_size -= sz_change;
>>       mutex_unlock(&dir_ui->ui_mutex);
>>   out_inode:
>>       make_bad_inode(inode);
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Richard Weinberger]

On Mon, Jan 25, 2021 at 2:12 AM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> 在 2021/1/23 10:45, Zhihao Cheng 写道:
>
> >> @@ -430,6 +433,7 @@ static int do_tmpfile(struct inode *dir, struct
> >> dentry *dentry,
> >>       return 0;
> >>   out_cancel:
> Still one question:
> > Does this need a judgment? Like this,

The idea was that in the !whiteout case, sz_change is always 0.

-- 
Thanks,
//richard

Re: [PATCH 3/4] ubifs: Update directory size when creating whiteouts

[Zhihao Cheng]

在 2021/1/25 15:55, Richard Weinberger 写道:

> 
> The idea was that in the !whiteout case, sz_change is always 0.
> 
Oh, sz_change was initialized to 0, I missed it.
Thanks.

[PATCH 4/4] ubifs: Harden ubifs_jnl_write_inode()

[Richard Weinberger]

Make sure that ubifs_jnl_write_inode() cannot be abused and will
not write less data then expected.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/journal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 03410ae0813a..1b28fcc5b9fe 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -844,10 +844,12 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode)
 	struct ubifs_ino_node *ino, *ino_start;
 	struct ubifs_inode *ui = ubifs_inode(inode);
 	int sync = 0, write_len = 0, ilen = UBIFS_INO_NODE_SZ;
-	int last_reference = !inode->i_nlink;
+	int last_reference = !inode->i_nlink, xattr_inos_written = 0;
 	int kill_xattrs = ui->xattr_cnt && last_reference;
 	u8 hash[UBIFS_HASH_ARR_SZ];
 
+	ubifs_assert(c, !ui->xattr);
+
 	dbg_jnl("ino %lu, nlink %u", inode->i_ino, inode->i_nlink);
 
 	/*
@@ -917,12 +919,14 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode)
 			pack_inode(c, ino, xino, 0);
 			ino = (void *)ino + UBIFS_INO_NODE_SZ;
 			iput(xino);
+			xattr_inos_written++;
 
 			kfree(pxent);
 			pxent = xent;
 			key_read(c, &xent->key, &key);
 		}
 		kfree(pxent);
+		ubifs_assert(c, xattr_inos_written == ui->xattr_cnt);
 	}
 
 	pack_inode(c, ino, inode, 1);
-- 
2.26.2