| Message ID | 20220922130401.1792256-3-chengzhihao1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check content after reading from quota file | expand |
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
在 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/
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);
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(-)