linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Check content after reading from quota file
@ 2022-09-22 13:03 Zhihao Cheng
  2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:03 UTC (permalink / raw)
  To: jack, tytso, brauner
  Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3

1. Fix invalid memory access of dquot.
2. Cleanup, replace places of block number checking with helper
   function.
3. Add more sanity checking for the content read from quota file.

v1->v2:
  Add CC-stable tag in first patch.
  Rename check_free_block() -> check_dquot_block_header().

Zhihao Cheng (3):
  quota: Check next/prev free block number after reading from quota file
  quota: Replace all block number checking with helper function
  quota: Add more checking after reading from quota file

 fs/quota/quota_tree.c | 81 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 12 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/3] quota: Check next/prev free block number after reading from quota file
  2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
@ 2022-09-22 13:03 ` Zhihao Cheng
  2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
  2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
  2 siblings, 0 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:03 UTC (permalink / raw)
  To: jack, tytso, brauner
  Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3

Following process:
 Init: v2_read_file_info: <3> dqi_free_blk 0 dqi_free_entry 5 dqi_blks 6

 Step 1. chown bin f_a -> dquot_acquire -> v2_write_dquot:
  qtree_write_dquot
   do_insert_tree
    find_free_dqentry
     get_free_dqblk
      write_blk(info->dqi_blocks) // info->dqi_blocks = 6, failure. The
	   content in physical block (corresponding to blk 6) is random.

 Step 2. chown root f_a -> dquot_transfer -> dqput_all -> dqput ->
         ext4_release_dquot -> v2_release_dquot -> qtree_delete_dquot:
  dquot_release
   remove_tree
    free_dqentry
     put_free_dqblk(6)
      info->dqi_free_blk = blk    // info->dqi_free_blk = 6

 Step 3. drop cache (buffer head for block 6 is released)

 Step 4. chown bin f_b -> dquot_acquire -> commit_dqblk -> v2_write_dquot:
  qtree_write_dquot
   do_insert_tree
    find_free_dqentry
     get_free_dqblk
      dh = (struct qt_disk_dqdbheader *)buf
      blk = info->dqi_free_blk     // 6
      ret = read_blk(info, blk, buf)  // The content of buf is random
      info->dqi_free_blk = le32_to_cpu(dh->dqdh_next_free)  // random blk

 Step 5. chown bin f_c -> notify_change -> ext4_setattr -> dquot_transfer:
  dquot = dqget -> acquire_dquot -> ext4_acquire_dquot -> dquot_acquire ->
          commit_dqblk -> v2_write_dquot -> dq_insert_tree:
   do_insert_tree
    find_free_dqentry
     get_free_dqblk
      blk = info->dqi_free_blk    // If blk < 0 and blk is not an error
				     code, it will be returned as dquot

  transfer_to[USRQUOTA] = dquot  // A random negative value
  __dquot_transfer(transfer_to)
   dquot_add_inodes(transfer_to[cnt])
    spin_lock(&dquot->dq_dqb_lock)  // page fault

, which will lead to kernel page fault:
 Quota error (device sda): qtree_write_dquot: Error -8000 occurred
 while creating quota
 BUG: unable to handle page fault for address: ffffffffffffe120
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 Oops: 0002 [#1] PREEMPT SMP
 CPU: 0 PID: 5974 Comm: chown Not tainted 6.0.0-rc1-00004
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 RIP: 0010:_raw_spin_lock+0x3a/0x90
 Call Trace:
  dquot_add_inodes+0x28/0x270
  __dquot_transfer+0x377/0x840
  dquot_transfer+0xde/0x540
  ext4_setattr+0x405/0x14d0
  notify_change+0x68e/0x9f0
  chown_common+0x300/0x430
  __x64_sys_fchownat+0x29/0x40

In order to avoid accessing invalid quota memory address, this patch adds
block number checking of next/prev free block read from quota file.

Fetch a reproducer in [Link].

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216372
Fixes: 1da177e4c3f4152 ("Linux-2.6.12-rc2")
CC: stable@vger.kernel.org
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/quota/quota_tree.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index 5f2405994280..f89186b6db1d 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,6 +71,35 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
 	return ret;
 }
 
+static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
+{
+	if (val >= max_val) {
+		quota_error(sb, "Getting block too big (%u >= %u)",
+			    val, max_val);
+		return -EUCLEAN;
+	}
+
+	return 0;
+}
+
+static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
+				    struct qt_disk_dqdbheader *dh)
+{
+	int err = 0;
+	uint nextblk, prevblk;
+
+	nextblk = le32_to_cpu(dh->dqdh_next_free);
+	err = do_check_range(info->dqi_sb, nextblk, info->dqi_blocks);
+	if (err)
+		return err;
+	prevblk = le32_to_cpu(dh->dqdh_prev_free);
+	err = do_check_range(info->dqi_sb, prevblk, info->dqi_blocks);
+	if (err)
+		return err;
+
+	return err;
+}
+
 /* Remove empty block from list and return it */
 static int get_free_dqblk(struct qtree_mem_dqinfo *info)
 {
@@ -85,6 +114,9 @@ static int get_free_dqblk(struct qtree_mem_dqinfo *info)
 		ret = read_blk(info, blk, buf);
 		if (ret < 0)
 			goto out_buf;
+		ret = check_dquot_block_header(info, dh);
+		if (ret)
+			goto out_buf;
 		info->dqi_free_blk = le32_to_cpu(dh->dqdh_next_free);
 	}
 	else {
@@ -232,6 +264,9 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
 		*err = read_blk(info, blk, buf);
 		if (*err < 0)
 			goto out_buf;
+		*err = check_dquot_block_header(info, dh);
+		if (*err)
+			goto out_buf;
 	} else {
 		blk = get_free_dqblk(info);
 		if ((int)blk < 0) {
@@ -424,6 +459,9 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
 		goto out_buf;
 	}
 	dh = (struct qt_disk_dqdbheader *)buf;
+	ret = check_dquot_block_header(info, dh);
+	if (ret)
+		goto out_buf;
 	le16_add_cpu(&dh->dqdh_entries, -1);
 	if (!le16_to_cpu(dh->dqdh_entries)) {	/* Block got free? */
 		ret = remove_free_dqentry(info, buf, blk);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/3] quota: Replace all block number checking with helper function
  2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
  2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
@ 2022-09-22 13:04 ` Zhihao Cheng
  2022-09-23 11:48   ` Jan Kara
  2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
  2 siblings, 1 reply; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:04 UTC (permalink / raw)
  To: jack, tytso, brauner
  Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3

Cleanup all block checking places, replace them with helper function
do_check_range().

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/quota/quota_tree.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index f89186b6db1d..47711e739ddb 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
 	return ret;
 }
 
-static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
+static inline int do_check_range(struct super_block *sb, uint val,
+				 uint min_val, uint max_val)
 {
-	if (val >= max_val) {
-		quota_error(sb, "Getting block too big (%u >= %u)",
-			    val, max_val);
+	if (val < min_val || val >= max_val) {
+		quota_error(sb, "Getting block %u out of range %u-%u",
+			    val, min_val, max_val);
 		return -EUCLEAN;
 	}
 
@@ -89,11 +90,11 @@ static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
 	uint nextblk, prevblk;
 
 	nextblk = le32_to_cpu(dh->dqdh_next_free);
-	err = do_check_range(info->dqi_sb, nextblk, info->dqi_blocks);
+	err = do_check_range(info->dqi_sb, nextblk, 0, info->dqi_blocks);
 	if (err)
 		return err;
 	prevblk = le32_to_cpu(dh->dqdh_prev_free);
-	err = do_check_range(info->dqi_sb, prevblk, info->dqi_blocks);
+	err = do_check_range(info->dqi_sb, prevblk, 0, info->dqi_blocks);
 	if (err)
 		return err;
 
@@ -518,12 +519,10 @@ static int remove_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
 		goto out_buf;
 	}
 	newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
-	if (newblk < QT_TREEOFF || newblk >= info->dqi_blocks) {
-		quota_error(dquot->dq_sb, "Getting block too big (%u >= %u)",
-			    newblk, info->dqi_blocks);
-		ret = -EUCLEAN;
+	ret = do_check_range(dquot->dq_sb, newblk, QT_TREEOFF,
+			     info->dqi_blocks);
+	if (ret)
 		goto out_buf;
-	}
 
 	if (depth == info->dqi_qtree_depth - 1) {
 		ret = free_dqentry(info, dquot, newblk);
@@ -624,12 +623,9 @@ static loff_t find_tree_dqentry(struct qtree_mem_dqinfo *info,
 	blk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
 	if (!blk)	/* No reference? */
 		goto out_buf;
-	if (blk < QT_TREEOFF || blk >= info->dqi_blocks) {
-		quota_error(dquot->dq_sb, "Getting block too big (%u >= %u)",
-			    blk, info->dqi_blocks);
-		ret = -EUCLEAN;
+	ret = do_check_range(dquot->dq_sb, blk, QT_TREEOFF, info->dqi_blocks);
+	if (ret)
 		goto out_buf;
-	}
 
 	if (depth < info->dqi_qtree_depth - 1)
 		ret = find_tree_dqentry(info, dquot, blk, depth+1);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/3] quota: Add more checking after reading from quota file
  2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
  2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
  2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
@ 2022-09-22 13:04 ` Zhihao Cheng
  2022-09-23 11:44   ` Jan Kara
  2 siblings, 1 reply; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:04 UTC (permalink / raw)
  To: jack, tytso, brauner
  Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3

It would be better to do more sanity checking (eg. dqdh_entries,
block no.) for the content read from quota file, which can prevent
corrupting the quota file.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/quota/quota_tree.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index 47711e739ddb..54fe4ad71de5 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,12 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
 	return ret;
 }
 
-static inline int do_check_range(struct super_block *sb, uint val,
-				 uint min_val, uint max_val)
+static inline int do_check_range(struct super_block *sb, const char *val_name,
+				 uint val, uint min_val, uint max_val)
 {
 	if (val < min_val || val >= max_val) {
-		quota_error(sb, "Getting block %u out of range %u-%u",
-			    val, min_val, max_val);
+		quota_error(sb, "Getting %s %u out of range %u-%u",
+			    val_name, val, min_val, max_val);
 		return -EUCLEAN;
 	}
 
@@ -90,11 +90,13 @@ static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
 	uint nextblk, prevblk;
 
 	nextblk = le32_to_cpu(dh->dqdh_next_free);
-	err = do_check_range(info->dqi_sb, nextblk, 0, info->dqi_blocks);
+	err = do_check_range(info->dqi_sb, "dqdh_next_free", nextblk, 0,
+			     info->dqi_blocks);
 	if (err)
 		return err;
 	prevblk = le32_to_cpu(dh->dqdh_prev_free);
-	err = do_check_range(info->dqi_sb, prevblk, 0, info->dqi_blocks);
+	err = do_check_range(info->dqi_sb, "dqdh_prev_free", prevblk, 0,
+			     info->dqi_blocks);
 	if (err)
 		return err;
 
@@ -268,6 +270,11 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
 		*err = check_dquot_block_header(info, dh);
 		if (*err)
 			goto out_buf;
+		*err = do_check_range(info->dqi_sb, "dqdh_entries",
+				      le16_to_cpu(dh->dqdh_entries), 0,
+				      qtree_dqstr_in_blk(info));
+		if (*err)
+			goto out_buf;
 	} else {
 		blk = get_free_dqblk(info);
 		if ((int)blk < 0) {
@@ -349,6 +356,10 @@ static int do_insert_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
 	}
 	ref = (__le32 *)buf;
 	newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
+	ret = do_check_range(dquot->dq_sb, "block", newblk, 0,
+			     info->dqi_blocks);
+	if (ret)
+		goto out_buf;
 	if (!newblk)
 		newson = 1;
 	if (depth == info->dqi_qtree_depth - 1) {
@@ -461,6 +472,11 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
 	}
 	dh = (struct qt_disk_dqdbheader *)buf;
 	ret = check_dquot_block_header(info, dh);
+	if (ret)
+		goto out_buf;
+	ret = do_check_range(info->dqi_sb, "dqdh_entries",
+			     le16_to_cpu(dh->dqdh_entries), 1,
+			     qtree_dqstr_in_blk(info) + 1);
 	if (ret)
 		goto out_buf;
 	le16_add_cpu(&dh->dqdh_entries, -1);
@@ -519,7 +535,7 @@ static int remove_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
 		goto out_buf;
 	}
 	newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
-	ret = do_check_range(dquot->dq_sb, newblk, QT_TREEOFF,
+	ret = do_check_range(dquot->dq_sb, "block", newblk, QT_TREEOFF,
 			     info->dqi_blocks);
 	if (ret)
 		goto out_buf;
@@ -623,7 +639,8 @@ static loff_t find_tree_dqentry(struct qtree_mem_dqinfo *info,
 	blk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
 	if (!blk)	/* No reference? */
 		goto out_buf;
-	ret = do_check_range(dquot->dq_sb, blk, QT_TREEOFF, info->dqi_blocks);
+	ret = do_check_range(dquot->dq_sb, "block", blk, QT_TREEOFF,
+			     info->dqi_blocks);
 	if (ret)
 		goto out_buf;
 
@@ -739,7 +756,13 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
 		goto out_buf;
 	}
 	for (i = __get_index(info, *id, depth); i < epb; i++) {
-		if (ref[i] == cpu_to_le32(0)) {
+		uint blk_no = le32_to_cpu(ref[i]);
+
+		ret = do_check_range(info->dqi_sb, "block", blk_no, 0,
+				     info->dqi_blocks);
+		if (ret)
+			goto out_buf;
+		if (blk_no == 0) {
 			*id += level_inc;
 			continue;
 		}
@@ -747,7 +770,7 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
 			ret = 0;
 			goto out_buf;
 		}
-		ret = find_next_id(info, id, le32_to_cpu(ref[i]), depth + 1);
+		ret = find_next_id(info, id, blk_no, depth + 1);
 		if (ret != -ENOENT)
 			break;
 	}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 3/3] quota: Add more checking after reading from quota file
  2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
@ 2022-09-23 11:44   ` Jan Kara
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2022-09-23 11:44 UTC (permalink / raw)
  To: Zhihao Cheng
  Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3

On Thu 22-09-22 21:04:01, Zhihao Cheng wrote:
> It would be better to do more sanity checking (eg. dqdh_entries,
> block no.) for the content read from quota file, which can prevent
> corrupting the quota file.
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  fs/quota/quota_tree.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> index 47711e739ddb..54fe4ad71de5 100644
> --- a/fs/quota/quota_tree.c
> +++ b/fs/quota/quota_tree.c
> @@ -71,12 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
>  	return ret;
>  }
>  
> -static inline int do_check_range(struct super_block *sb, uint val,
> -				 uint min_val, uint max_val)
> +static inline int do_check_range(struct super_block *sb, const char *val_name,
> +				 uint val, uint min_val, uint max_val)
>  {
>  	if (val < min_val || val >= max_val) {
> -		quota_error(sb, "Getting block %u out of range %u-%u",
> -			    val, min_val, max_val);
> +		quota_error(sb, "Getting %s %u out of range %u-%u",
> +			    val_name, val, min_val, max_val);
>  		return -EUCLEAN;
>  	}

As I already wrote in my comments to v1, please create do_check_range()
already with this prototype in patch 1 so that you don't have to update it
(and all the call sites) in each of the patches. It makes review simpler.

> @@ -268,6 +270,11 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
>  		*err = check_dquot_block_header(info, dh);
>  		if (*err)
>  			goto out_buf;
> +		*err = do_check_range(info->dqi_sb, "dqdh_entries",
> +				      le16_to_cpu(dh->dqdh_entries), 0,
> +				      qtree_dqstr_in_blk(info));
> +		if (*err)
> +			goto out_buf;

The checking of dqdh_entries belongs into check_dquot_block_header(). That
was the reason why it was created. So that all the checks are together in
one function...

>  	} else {
>  		blk = get_free_dqblk(info);
>  		if ((int)blk < 0) {
> @@ -349,6 +356,10 @@ static int do_insert_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
>  	}
>  	ref = (__le32 *)buf;
>  	newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
> +	ret = do_check_range(dquot->dq_sb, "block", newblk, 0,
> +			     info->dqi_blocks);
> +	if (ret)
> +		goto out_buf;
>  	if (!newblk)
>  		newson = 1;
>  	if (depth == info->dqi_qtree_depth - 1) {
> @@ -461,6 +472,11 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
>  	}
>  	dh = (struct qt_disk_dqdbheader *)buf;
>  	ret = check_dquot_block_header(info, dh);
> +	if (ret)
> +		goto out_buf;
> +	ret = do_check_range(info->dqi_sb, "dqdh_entries",
> +			     le16_to_cpu(dh->dqdh_entries), 1,
> +			     qtree_dqstr_in_blk(info) + 1);

Again, the check of dqdh_entries should be in check_dquot_block_header().

> @@ -739,7 +756,13 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
>  		goto out_buf;
>  	}
>  	for (i = __get_index(info, *id, depth); i < epb; i++) {
> -		if (ref[i] == cpu_to_le32(0)) {
> +		uint blk_no = le32_to_cpu(ref[i]);
> +
> +		ret = do_check_range(info->dqi_sb, "block", blk_no, 0,
> +				     info->dqi_blocks);
> +		if (ret)
> +			goto out_buf;
> +		if (blk_no == 0) {
>  			*id += level_inc;
>  			continue;
>  		}

I'd leave checking for 0 first here - i.e.:
		if (ref[i] == cpu_to_le32(0)) {
  			*id += level_inc;
  			continue;
  		}

and only then do:
		blk_no = le32_to_cpu(ref[i]);
		ret = do_check_range(...);

There's no point in checking known-good value.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/3] quota: Replace all block number checking with helper function
  2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
@ 2022-09-23 11:48   ` Jan Kara
  2022-09-27  1:07     ` Zhihao Cheng
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2022-09-23 11:48 UTC (permalink / raw)
  To: Zhihao Cheng
  Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3

On Thu 22-09-22 21:04:00, Zhihao Cheng wrote:
> Cleanup all block checking places, replace them with helper function
> do_check_range().
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  fs/quota/quota_tree.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)

Thanks for the fix! One comment below:

> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> index f89186b6db1d..47711e739ddb 100644
> --- a/fs/quota/quota_tree.c
> +++ b/fs/quota/quota_tree.c
> @@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
>  	return ret;
>  }
>  
> -static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
> +static inline int do_check_range(struct super_block *sb, uint val,
> +				 uint min_val, uint max_val)
>  {
> -	if (val >= max_val) {
> -		quota_error(sb, "Getting block too big (%u >= %u)",
> -			    val, max_val);
> +	if (val < min_val || val >= max_val) {
> +		quota_error(sb, "Getting block %u out of range %u-%u",
> +			    val, min_val, max_val);
>  		return -EUCLEAN;
>  	}

It is strange that do_check_range() checks min_val() with strict inequality
and max_val with non-strict one. That's off-by-one problem waiting to
happen when we forget about this detail. Probably make max_val
non-inclusive as well (the parameter max_val suggests the passed value is
the biggest valid one anyway).

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/3] quota: Replace all block number checking with helper function
  2022-09-23 11:48   ` Jan Kara
@ 2022-09-27  1:07     ` Zhihao Cheng
  0 siblings, 0 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-27  1:07 UTC (permalink / raw)
  To: Jan Kara
  Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3

在 2022/9/23 19:48, Jan Kara 写道:
> On Thu 22-09-22 21:04:00, Zhihao Cheng wrote:
>> Cleanup all block checking places, replace them with helper function
>> do_check_range().
>>
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>>   fs/quota/quota_tree.c | 28 ++++++++++++----------------
>>   1 file changed, 12 insertions(+), 16 deletions(-)
> 
> Thanks for the fix! One comment below:
> 
>> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
>> index f89186b6db1d..47711e739ddb 100644
>> --- a/fs/quota/quota_tree.c
>> +++ b/fs/quota/quota_tree.c
>> @@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
>>   	return ret;
>>   }
>>   
>> -static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
>> +static inline int do_check_range(struct super_block *sb, uint val,
>> +				 uint min_val, uint max_val)
>>   {
>> -	if (val >= max_val) {
>> -		quota_error(sb, "Getting block too big (%u >= %u)",
>> -			    val, max_val);
>> +	if (val < min_val || val >= max_val) {
>> +		quota_error(sb, "Getting block %u out of range %u-%u",
>> +			    val, min_val, max_val);
>>   		return -EUCLEAN;
>>   	}
> 
> It is strange that do_check_range() checks min_val() with strict inequality
> and max_val with non-strict one. That's off-by-one problem waiting to
> happen when we forget about this detail. Probably make max_val
> non-inclusive as well (the parameter max_val suggests the passed value is
> the biggest valid one anyway).
> 
> 								Honza
> 

I have sent v3 series, see
https://lore.kernel.org/all/20220923134555.2623931-1-chengzhihao1@huawei.com/T/


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-27  1:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
2022-09-23 11:48   ` Jan Kara
2022-09-27  1:07     ` Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
2022-09-23 11:44   ` Jan Kara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).