[PATCH 0/3] udf: refactor udf_current_aext()/udf_next_aext() to handle error

[PATCH 0/3] udf: refactor udf_current_aext()/udf_next_aext() to handle error

[Zhao Mengmeng]

From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

syzbot reports a udf slab-out-of-bounds at [1] and I proposed a fix patch,
after talking with Jan, a better way to fix this is to refactor 
udf_current_aext() and udf_next_aext() to differentiate between error and
"hit EOF".
This series refactor udf_current_aext(), udf_next_aext() and inode_bmap(),
they take pointer to etype to store the extent type and just return 0 on 
success, <0 on error. It has passed the syz repro test.



[1]. https://lore.kernel.org/all/0000000000005093590621340ecf@google.com/

Zhao Mengmeng (3):
  udf: refactor udf_current_aext() to handle error
  udf: refactor udf_next_aext() to handle error
  udf: refactor inode_bmap() to handle error

 fs/udf/balloc.c    |   6 +--
 fs/udf/directory.c |  20 +++++---
 fs/udf/inode.c     | 112 ++++++++++++++++++++++++++-------------------
 fs/udf/partition.c |   6 ++-
 fs/udf/super.c     |   3 +-
 fs/udf/truncate.c  |  16 +++----
 fs/udf/udfdecl.h   |  15 +++---
 7 files changed, 104 insertions(+), 74 deletions(-)

-- 
2.43.0

[PATCH 1/3] udf: refactor udf_current_aext() to handle error

[Zhao Mengmeng]

From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

As Jan suggested in links below, refactor udf_current_aext() to
differentiate between error and "hit EOF", it now takes pointer to etype
to store the extent type, return 0 when get etype success; return -ENODATA
when hit EOF; return -EINVAL when i_alloc_type invalid.

Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/

Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
 fs/udf/inode.c    | 37 +++++++++++++++++++++++--------------
 fs/udf/truncate.c |  3 +--
 fs/udf/udfdecl.h  |  5 +++--
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 4726a4d014b6..6d41ca0e7dba 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1955,6 +1955,7 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	struct extent_position nepos;
 	struct kernel_lb_addr neloc;
 	int ver, adsize;
+	int err = 0;
 
 	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
 		adsize = sizeof(struct short_ad);
@@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	if (epos->offset + adsize > sb->s_blocksize) {
 		struct kernel_lb_addr cp_loc;
 		uint32_t cp_len;
-		int cp_type;
+		int8_t cp_type;
 
 		epos->offset -= adsize;
-		cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
+		err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
+		if (err < 0)
+			goto err_out;
 		cp_len |= ((uint32_t)cp_type) << 30;
 
 		__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
@@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	*epos = nepos;
 
 	return 0;
+err_out:
+	brelse(epos->bh);
+	return err;
 }
 
 /*
@@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 {
 	int8_t etype;
 	unsigned int indirections = 0;
+	int err = 0;
+
+	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
+		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
+			break;
 
-	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
-	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
 		udf_pblk_t block;
 
 		if (++indirections > UDF_MAX_INDIR_EXTS) {
@@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 		}
 	}
 
-	return etype;
+	return err;
 }
 
-int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
-			struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
+int udf_current_aext(struct inode *inode, struct extent_position *epos,
+		     struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
+		     int inc)
 {
 	int alen;
-	int8_t etype;
 	uint8_t *ptr;
 	struct short_ad *sad;
 	struct long_ad *lad;
@@ -2224,8 +2233,8 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
 	case ICBTAG_FLAG_AD_SHORT:
 		sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
 		if (!sad)
-			return -1;
-		etype = le32_to_cpu(sad->extLength) >> 30;
+			return -ENODATA;
+		*etype = le32_to_cpu(sad->extLength) >> 30;
 		eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
 		eloc->partitionReferenceNum =
 				iinfo->i_location.partitionReferenceNum;
@@ -2234,17 +2243,17 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
 	case ICBTAG_FLAG_AD_LONG:
 		lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
 		if (!lad)
-			return -1;
-		etype = le32_to_cpu(lad->extLength) >> 30;
+			return -ENODATA;
+		*etype = le32_to_cpu(lad->extLength) >> 30;
 		*eloc = lelb_to_cpu(lad->extLocation);
 		*elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
 		break;
 	default:
 		udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
-		return -1;
+		return -EINVAL;
 	}
 
-	return etype;
+	return 0;
 }
 
 static int udf_insert_aext(struct inode *inode, struct extent_position epos,
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index a686c10fd709..91b6e2698e7e 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -217,8 +217,7 @@ int udf_truncate_extents(struct inode *inode)
 	else
 		lenalloc -= sizeof(struct allocExtDesc);
 
-	while ((etype = udf_current_aext(inode, &epos, &eloc,
-					 &elen, 0)) != -1) {
+	while (!udf_current_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
 		if (etype == (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
 			udf_write_aext(inode, &epos, &neloc, nelen, 0);
 			if (indirect_ext_len) {
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 88692512a466..d893db95ac70 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -171,8 +171,9 @@ extern void udf_write_aext(struct inode *, struct extent_position *,
 extern int8_t udf_delete_aext(struct inode *, struct extent_position);
 extern int8_t udf_next_aext(struct inode *, struct extent_position *,
 			    struct kernel_lb_addr *, uint32_t *, int);
-extern int8_t udf_current_aext(struct inode *, struct extent_position *,
-			       struct kernel_lb_addr *, uint32_t *, int);
+extern int udf_current_aext(struct inode *inode, struct extent_position *epos,
+			    struct kernel_lb_addr *eloc, uint32_t *elen,
+			    int8_t *etype, int inc);
 extern void udf_update_extra_perms(struct inode *inode, umode_t mode);
 
 /* misc.c */
-- 
2.43.0

[PATCH 2/3] udf: refactor udf_next_aext() to handle error

[Zhao Mengmeng]

From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

Same as udf_current_aext(), take pointer to etype to store the extent
type, while return 0 for success and <0 on error.

Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
 fs/udf/balloc.c    |  6 +++---
 fs/udf/directory.c |  7 +++++--
 fs/udf/inode.c     | 50 +++++++++++++++++++++++-----------------------
 fs/udf/super.c     |  3 ++-
 fs/udf/truncate.c  |  8 ++++----
 fs/udf/udfdecl.h   |  5 +++--
 6 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index d8fc11765d61..b216c43cf433 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -384,7 +384,7 @@ static void udf_table_free_blocks(struct super_block *sb,
 	epos.bh = oepos.bh = NULL;
 
 	while (count &&
-	       (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+	       !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
 		if (((eloc.logicalBlockNum +
 			(elen >> sb->s_blocksize_bits)) == start)) {
 			if ((0x3FFFFFFF - elen) <
@@ -517,7 +517,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
 	eloc.logicalBlockNum = 0xFFFFFFFF;
 
 	while (first_block != eloc.logicalBlockNum &&
-	       (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+	       !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
 		udf_debug("eloc=%u, elen=%u, first_block=%u\n",
 			  eloc.logicalBlockNum, elen, first_block);
 		; /* empty loop body */
@@ -584,7 +584,7 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
 	epos.bh = goal_epos.bh = NULL;
 
 	while (spread &&
-	       (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+	       !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
 		if (goal >= eloc.logicalBlockNum) {
 			if (goal < eloc.logicalBlockNum +
 					(elen >> sb->s_blocksize_bits))
diff --git a/fs/udf/directory.c b/fs/udf/directory.c
index 93153665eb37..f865538c985d 100644
--- a/fs/udf/directory.c
+++ b/fs/udf/directory.c
@@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
  */
 static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
 {
+	int8_t etype;
+	int err = 0;
 	iter->loffset++;
 	if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
 		return 0;
 
 	iter->loffset = 0;
-	if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
-			!= (EXT_RECORDED_ALLOCATED >> 30)) {
+	err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
+			    &etype, 1);
+	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
 		if (iter->pos == iter->dir->i_size) {
 			iter->elen = 0;
 			return 0;
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 6d41ca0e7dba..ba980ce5e13a 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -545,6 +545,7 @@ static int udf_do_extend_file(struct inode *inode,
 	} else {
 		struct kernel_lb_addr tmploc;
 		uint32_t tmplen;
+		int8_t tmptype;
 
 		udf_write_aext(inode, last_pos, &last_ext->extLocation,
 				last_ext->extLength, 1);
@@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
 		 * empty indirect extent.
 		 */
 		if (new_block_bytes)
-			udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
+			udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
 	}
 	iinfo->i_lenExtents += add;
 
@@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
 		extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
 	} else {
 		epos.offset -= adsize;
-		etype = udf_next_aext(inode, &epos, &extent.extLocation,
-				      &extent.extLength, 0);
+		udf_next_aext(inode, &epos, &extent.extLocation,
+				&extent.extLength, &etype, 0);
 		extent.extLength |= etype << 30;
 	}
 
@@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 	loff_t lbcount = 0, b_off = 0;
 	udf_pblk_t newblocknum;
 	sector_t offset = 0;
-	int8_t etype;
+	int8_t etype, tmpetype;
 	struct udf_inode_info *iinfo = UDF_I(inode);
 	udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
 	int lastblock = 0;
@@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 		prev_epos.offset = cur_epos.offset;
 		cur_epos.offset = next_epos.offset;
 
-		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
-		if (etype == -1)
+		ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
+		if (ret)
 			break;
 
 		c = !c;
@@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 	 * Move prev_epos and cur_epos into indirect extent if we are at
 	 * the pointer to it
 	 */
-	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
-	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
+	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
+	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
 
 	/* if the extent is allocated and recorded, return the block
 	   if the extent is not a multiple of the blocksize, round up */
@@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 	}
 
 	/* Are we beyond EOF and preallocated extent? */
-	if (etype == -1) {
+	if (ret < 0) {
 		loff_t hole_len;
 
 		isBeyondEOF = true;
@@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
 
 		/* if the current block is located in an extent,
 		   read the next extent */
-		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
-		if (etype != -1) {
+		if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
 			laarr[c + 1].extLength = (etype << 30) | elen;
 			laarr[c + 1].extLocation = eloc;
 			count++;
@@ -1172,6 +1172,7 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
 	int start = 0, i;
 	struct kernel_lb_addr tmploc;
 	uint32_t tmplen;
+	int8_t tmptype;
 	int err;
 
 	if (startnum > endnum) {
@@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
 			if (err < 0)
 				return err;
 			udf_next_aext(inode, epos, &laarr[i].extLocation,
-				      &laarr[i].extLength, 1);
+				      &laarr[i].extLength, &tmptype, 1);
 			start++;
 		}
 	}
 
 	for (i = start; i < endnum; i++) {
-		udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
+		udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
 		udf_write_aext(inode, epos, &laarr[i].extLocation,
 			       laarr[i].extLength, 1);
 	}
@@ -2168,15 +2169,15 @@ void udf_write_aext(struct inode *inode, struct extent_position *epos,
  */
 #define UDF_MAX_INDIR_EXTS 16
 
-int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
-		     struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
+int udf_next_aext(struct inode *inode, struct extent_position *epos,
+		  struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
+		  int inc)
 {
-	int8_t etype;
 	unsigned int indirections = 0;
 	int err = 0;
 
-	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
-		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
+	while ((err = udf_current_aext(inode, epos, eloc, elen, etype, inc))) {
+		if (err || *etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
 			break;
 
 		udf_pblk_t block;
@@ -2185,7 +2186,7 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 			udf_err(inode->i_sb,
 				"too many indirect extents in inode %lu\n",
 				inode->i_ino);
-			return -1;
+			return -EFSCORRUPTED;
 		}
 
 		epos->block = *eloc;
@@ -2195,7 +2196,7 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 		epos->bh = sb_bread(inode->i_sb, block);
 		if (!epos->bh) {
 			udf_debug("reading block %u failed!\n", block);
-			return -1;
+			return -EIO;
 		}
 	}
 
@@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
 	if (epos.bh)
 		get_bh(epos.bh);
 
-	while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
+	while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
 		udf_write_aext(inode, &epos, &neloc, nelen, 1);
 		neloc = oeloc;
 		nelen = (etype << 30) | oelen;
@@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
 		adsize = 0;
 
 	oepos = epos;
-	if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
+	if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
 		return -1;
 
-	while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
+	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
 		udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
 		if (oepos.bh != epos.bh) {
 			oepos.block = epos.block;
@@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
 	}
 	*elen = 0;
 	do {
-		etype = udf_next_aext(inode, pos, eloc, elen, 1);
-		if (etype == -1) {
+		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
 			*offset = (bcount - lbcount) >> blocksize_bits;
 			iinfo->i_lenExtents = lbcount;
 			return -1;
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 3460ecc826d1..8c34224e1aee 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2482,13 +2482,14 @@ static unsigned int udf_count_free_table(struct super_block *sb,
 	uint32_t elen;
 	struct kernel_lb_addr eloc;
 	struct extent_position epos;
+	int8_t etype;
 
 	mutex_lock(&UDF_SB(sb)->s_alloc_mutex);
 	epos.block = UDF_I(table)->i_location;
 	epos.offset = sizeof(struct unallocSpaceEntry);
 	epos.bh = NULL;
 
-	while (udf_next_aext(table, &epos, &eloc, &elen, 1) != -1)
+	while (!udf_next_aext(table, &epos, &eloc, &elen, &etype, 1))
 		accum += (elen >> table->i_sb->s_blocksize_bits);
 
 	brelse(epos.bh);
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index 91b6e2698e7e..b7361222f988 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
 		BUG();
 
 	/* Find the last extent in the file */
-	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
+	while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
 		etype = netype;
 		lbcount += elen;
 		if (lbcount > inode->i_size) {
@@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
 			epos.offset -= adsize;
 			extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
 			epos.offset += adsize;
-			if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
+			if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
 				udf_err(inode->i_sb,
 					"Extent after EOF in inode %u\n",
 					(unsigned)inode->i_ino);
@@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
 	epos.block = iinfo->i_location;
 
 	/* Find the last extent in the file */
-	while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
+	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
 		brelse(prev_epos.bh);
 		prev_epos = epos;
 		if (prev_epos.bh)
 			get_bh(prev_epos.bh);
 
-		etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
+		udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
 		lbcount += elen;
 	}
 	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index d893db95ac70..5067ed68a8b4 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -169,8 +169,9 @@ extern int udf_add_aext(struct inode *, struct extent_position *,
 extern void udf_write_aext(struct inode *, struct extent_position *,
 			   struct kernel_lb_addr *, uint32_t, int);
 extern int8_t udf_delete_aext(struct inode *, struct extent_position);
-extern int8_t udf_next_aext(struct inode *, struct extent_position *,
-			    struct kernel_lb_addr *, uint32_t *, int);
+extern int udf_next_aext(struct inode *inode, struct extent_position *epos,
+			 struct kernel_lb_addr *eloc, uint32_t *elen,
+			 int8_t *etype, int inc);
 extern int udf_current_aext(struct inode *inode, struct extent_position *epos,
 			    struct kernel_lb_addr *eloc, uint32_t *elen,
 			    int8_t *etype, int inc);
-- 
2.43.0

[PATCH 3/3] udf: refactor inode_bmap() to handle error

[Zhao Mengmeng]

From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

Same as udf_current_aext(), take pointer to etype to store the extent
type, while return 0 for success and <0 on error. On situations like
ftruncate, udf_extend_file() can detect errors and bail out early
without resorting to checking for particular offsets and
assuming internal behavior of these functions.

Reported-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
Tested-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
 fs/udf/directory.c | 13 ++++++++-----
 fs/udf/inode.c     | 31 ++++++++++++++++++++-----------
 fs/udf/partition.c |  6 ++++--
 fs/udf/truncate.c  |  5 +++--
 fs/udf/udfdecl.h   |  5 +++--
 5 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/fs/udf/directory.c b/fs/udf/directory.c
index f865538c985d..cb40e1ade9f6 100644
--- a/fs/udf/directory.c
+++ b/fs/udf/directory.c
@@ -243,6 +243,7 @@ int udf_fiiter_init(struct udf_fileident_iter *iter, struct inode *dir,
 {
 	struct udf_inode_info *iinfo = UDF_I(dir);
 	int err = 0;
+	int8_t etype;
 
 	iter->dir = dir;
 	iter->bh[0] = iter->bh[1] = NULL;
@@ -262,9 +263,9 @@ int udf_fiiter_init(struct udf_fileident_iter *iter, struct inode *dir,
 		goto out;
 	}
 
-	if (inode_bmap(dir, iter->pos >> dir->i_blkbits, &iter->epos,
-		       &iter->eloc, &iter->elen, &iter->loffset) !=
-	    (EXT_RECORDED_ALLOCATED >> 30)) {
+	err = inode_bmap(dir, iter->pos >> dir->i_blkbits, &iter->epos, &iter->eloc,
+		   &iter->elen, &iter->loffset, &etype);
+	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
 		if (pos == dir->i_size)
 			return 0;
 		udf_err(dir->i_sb,
@@ -460,6 +461,7 @@ int udf_fiiter_append_blk(struct udf_fileident_iter *iter)
 	sector_t block;
 	uint32_t old_elen = iter->elen;
 	int err;
+	int8_t etype;
 
 	if (WARN_ON_ONCE(iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB))
 		return -EINVAL;
@@ -474,8 +476,9 @@ int udf_fiiter_append_blk(struct udf_fileident_iter *iter)
 		udf_fiiter_update_elen(iter, old_elen);
 		return err;
 	}
-	if (inode_bmap(iter->dir, block, &iter->epos, &iter->eloc, &iter->elen,
-		       &iter->loffset) != (EXT_RECORDED_ALLOCATED >> 30)) {
+	err = inode_bmap(iter->dir, block, &iter->epos, &iter->eloc, &iter->elen,
+		   &iter->loffset, &etype);
+	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
 		udf_err(iter->dir->i_sb,
 			"block %llu not allocated in directory (ino %lu)\n",
 			(unsigned long long)block, iter->dir->i_ino);
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index ba980ce5e13a..f1b8f0a0d202 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -418,10 +418,11 @@ static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
 		uint32_t elen;
 		sector_t offset;
 		struct extent_position epos = {};
+		int8_t etype;
 
 		down_read(&iinfo->i_data_sem);
-		if (inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset)
-				== (EXT_RECORDED_ALLOCATED >> 30)) {
+		inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset, &etype);
+		if (etype == (EXT_RECORDED_ALLOCATED >> 30)) {
 			map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc,
 							offset);
 			map->oflags |= UDF_BLK_MAPPED;
@@ -660,8 +661,10 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
 	 */
 	udf_discard_prealloc(inode);
 
-	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
-	within_last_ext = (etype != -1);
+	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
+	if (err < 0 && err != -ENODATA)
+		goto out;
+	within_last_ext = (!err);
 	/* We don't expect extents past EOF... */
 	WARN_ON_ONCE(within_last_ext &&
 		     elen > ((loff_t)offset + 1) << inode->i_blkbits);
@@ -2363,14 +2366,19 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
 	return (elen >> 30);
 }
 
-int8_t inode_bmap(struct inode *inode, sector_t block,
-		  struct extent_position *pos, struct kernel_lb_addr *eloc,
-		  uint32_t *elen, sector_t *offset)
+/*
+ * return 0 when iudf_next_aext() loop success.
+ * return err < 0 and err != -ENODATA indicates error.
+ * return err == -ENODATA indicates hit EOF.
+ */
+int inode_bmap(struct inode *inode, sector_t block, struct extent_position *pos,
+	       struct kernel_lb_addr *eloc, uint32_t *elen, sector_t *offset,
+	       int8_t *etype)
 {
 	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
 	loff_t lbcount = 0, bcount = (loff_t) block << blocksize_bits;
-	int8_t etype;
 	struct udf_inode_info *iinfo;
+	int err = 0;
 
 	iinfo = UDF_I(inode);
 	if (!udf_read_extent_cache(inode, bcount, &lbcount, pos)) {
@@ -2380,10 +2388,11 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
 	}
 	*elen = 0;
 	do {
-		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
+		err = udf_next_aext(inode, pos, eloc, elen, etype, 1);
+		if (err < 0) {
 			*offset = (bcount - lbcount) >> blocksize_bits;
 			iinfo->i_lenExtents = lbcount;
-			return -1;
+			return err;
 		}
 		lbcount += *elen;
 	} while (lbcount <= bcount);
@@ -2391,5 +2400,5 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
 	udf_update_extent_cache(inode, lbcount - *elen, pos);
 	*offset = (bcount + *elen - lbcount) >> blocksize_bits;
 
-	return etype;
+	return 0;
 }
diff --git a/fs/udf/partition.c b/fs/udf/partition.c
index af877991edc1..c441d4ae1f96 100644
--- a/fs/udf/partition.c
+++ b/fs/udf/partition.c
@@ -282,9 +282,11 @@ static uint32_t udf_try_read_meta(struct inode *inode, uint32_t block,
 	sector_t ext_offset;
 	struct extent_position epos = {};
 	uint32_t phyblock;
+	int8_t etype;
+	int err = 0;
 
-	if (inode_bmap(inode, block, &epos, &eloc, &elen, &ext_offset) !=
-						(EXT_RECORDED_ALLOCATED >> 30))
+	err = inode_bmap(inode, block, &epos, &eloc, &elen, &ext_offset, &etype);
+	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30))
 		phyblock = 0xFFFFFFFF;
 	else {
 		map = &UDF_SB(sb)->s_partmaps[partition];
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index b7361222f988..a70b6ae4ab8a 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -188,6 +188,7 @@ int udf_truncate_extents(struct inode *inode)
 	loff_t byte_offset;
 	int adsize;
 	struct udf_inode_info *iinfo = UDF_I(inode);
+	int err = 0;
 
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
 		adsize = sizeof(struct short_ad);
@@ -196,10 +197,10 @@ int udf_truncate_extents(struct inode *inode)
 	else
 		BUG();
 
-	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
+	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
 	byte_offset = (offset << sb->s_blocksize_bits) +
 		(inode->i_size & (sb->s_blocksize - 1));
-	if (etype == -1) {
+	if (err < 0) {
 		/* We should extend the file? */
 		WARN_ON(byte_offset);
 		return 0;
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 5067ed68a8b4..d159f20d61e8 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -157,8 +157,9 @@ extern struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 extern int udf_setsize(struct inode *, loff_t);
 extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
-extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
-			 struct kernel_lb_addr *, uint32_t *, sector_t *);
+extern int inode_bmap(struct inode *inode, sector_t block,
+		      struct extent_position *pos, struct kernel_lb_addr *eloc,
+		      uint32_t *elen, sector_t *offset, int8_t *etype);
 int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
 extern int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 				   struct extent_position *epos);
-- 
2.43.0

Re: [PATCH 1/3] udf: refactor udf_current_aext() to handle error

[Jan Kara]

On Wed 18-09-24 17:36:32, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> As Jan suggested in links below, refactor udf_current_aext() to
> differentiate between error and "hit EOF", it now takes pointer to etype
> to store the extent type, return 0 when get etype success; return -ENODATA
> when hit EOF; return -EINVAL when i_alloc_type invalid.
> 
> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
> 
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

Two comments below.

> @@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>  	if (epos->offset + adsize > sb->s_blocksize) {
>  		struct kernel_lb_addr cp_loc;
>  		uint32_t cp_len;
> -		int cp_type;
> +		int8_t cp_type;
>  
>  		epos->offset -= adsize;
> -		cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
> +		err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
> +		if (err < 0)
> +			goto err_out;
>  		cp_len |= ((uint32_t)cp_type) << 30;
>  
>  		__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
> @@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>  	*epos = nepos;
>  
>  	return 0;
> +err_out:
> +	brelse(epos->bh);
> +	return err;
>  }

So here I don't think we want to release epos->bh. Rather we need to
release 'bh' itself which we have got because we did't replace epos->bh yet
with it.

> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  {
>  	int8_t etype;
>  	unsigned int indirections = 0;
> +	int err = 0;
> +
> +	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
> +		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
> +			break;
>  
> -	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
> -	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
>  		udf_pblk_t block;
>  
>  		if (++indirections > UDF_MAX_INDIR_EXTS) {
> @@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  		}
>  	}
>  
> -	return etype;
> +	return err;
>  }

This doesn't look right. Probably it gets fixed up in the following patches
but here should be something like: !err ? etype : -1
to keep udf_next_aext() compatible with its users.

Otherwise the patch looks good.

								Honza

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

Re: [PATCH 2/3] udf: refactor udf_next_aext() to handle error

[Jan Kara]

On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> Same as udf_current_aext(), take pointer to etype to store the extent
> type, while return 0 for success and <0 on error.
> 
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

...

> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
> index 93153665eb37..f865538c985d 100644
> --- a/fs/udf/directory.c
> +++ b/fs/udf/directory.c
> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>   */
>  static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>  {
> +	int8_t etype;
> +	int err = 0;

Nit: please add empty line between declaration and the code.

>  	iter->loffset++;
>  	if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>  		return 0;
>  
>  	iter->loffset = 0;
> -	if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
> -			!= (EXT_RECORDED_ALLOCATED >> 30)) {
> +	err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
> +			    &etype, 1);
> +	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>  		if (iter->pos == iter->dir->i_size) {
>  			iter->elen = 0;
>  			return 0;

...

> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>  		 * empty indirect extent.
>  		 */
>  		if (new_block_bytes)
> -			udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
> +			udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>  	}
>  	iinfo->i_lenExtents += add;
>  

Hum, this will need error checking but we can leave that for future
patches.

> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>  		extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>  	} else {
>  		epos.offset -= adsize;
> -		etype = udf_next_aext(inode, &epos, &extent.extLocation,
> -				      &extent.extLength, 0);
> +		udf_next_aext(inode, &epos, &extent.extLocation,
> +				&extent.extLength, &etype, 0);
>  		extent.extLength |= etype << 30;
>  	}
>  
> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>  	loff_t lbcount = 0, b_off = 0;
>  	udf_pblk_t newblocknum;
>  	sector_t offset = 0;
> -	int8_t etype;
> +	int8_t etype, tmpetype;
>  	struct udf_inode_info *iinfo = UDF_I(inode);
>  	udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>  	int lastblock = 0;
> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>  		prev_epos.offset = cur_epos.offset;
>  		cur_epos.offset = next_epos.offset;
>  
> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
> -		if (etype == -1)
> +		ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
> +		if (ret)
>  			break;

I think here we need to add error handling as well and we should probably
do it in this patch / patch series. If ret is ENODATA, we just break out
from the cycle but if ret is some other error, we need to return that error
from inode_getblk().

> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>  	 * Move prev_epos and cur_epos into indirect extent if we are at
>  	 * the pointer to it
>  	 */
> -	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
> -	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
> +	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> +	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);

Again, this should have error handling now.

>  
>  	/* if the extent is allocated and recorded, return the block
>  	   if the extent is not a multiple of the blocksize, round up */
> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>  	}
>  
>  	/* Are we beyond EOF and preallocated extent? */
> -	if (etype == -1) {
> +	if (ret < 0) {

I'd prefer ret == -ENODATA to make this explicit.

>  		loff_t hole_len;
>  
>  		isBeyondEOF = true;
> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>  
>  		/* if the current block is located in an extent,
>  		   read the next extent */
> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
> -		if (etype != -1) {
> +		if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>  			laarr[c + 1].extLength = (etype << 30) | elen;
>  			laarr[c + 1].extLocation = eloc;
>  			count++;

And this should be distinguisting between EOF and other errors so that we
don't set lastblock wrongly. Instead we should bail with error.

> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>  			if (err < 0)
>  				return err;
>  			udf_next_aext(inode, epos, &laarr[i].extLocation,
> -				      &laarr[i].extLength, 1);
> +				      &laarr[i].extLength, &tmptype, 1);
>  			start++;
>  		}
>  	}
>  
>  	for (i = start; i < endnum; i++) {
> -		udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
> +		udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>  		udf_write_aext(inode, epos, &laarr[i].extLocation,
>  			       laarr[i].extLength, 1);
>  	}

Again these two calls should have error handling now. udf_update_extents()
is already able to return errors.

> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>  	if (epos.bh)
>  		get_bh(epos.bh);
>  
> -	while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
> +	while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>  		udf_write_aext(inode, &epos, &neloc, nelen, 1);
>  		neloc = oeloc;
>  		nelen = (etype << 30) | oelen;

Here, we should check if udf_next_aext() returned error (other than
ENODATA) and bail in that case instead of trying to insert new extent.

> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>  		adsize = 0;
>  
>  	oepos = epos;
> -	if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
> +	if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>  		return -1;
>  
> -	while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>  		udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>  		if (oepos.bh != epos.bh) {
>  			oepos.block = epos.block;
> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>  	}
>  	*elen = 0;
>  	do {
> -		etype = udf_next_aext(inode, pos, eloc, elen, 1);
> -		if (etype == -1) {
> +		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>  			*offset = (bcount - lbcount) >> blocksize_bits;
>  			iinfo->i_lenExtents = lbcount;
>  			return -1;

Again, here we need to distinguish ENODATA from other errors so that we
don't wrongly consider failure to read extent like EOF.

> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> index 91b6e2698e7e..b7361222f988 100644
> --- a/fs/udf/truncate.c
> +++ b/fs/udf/truncate.c
> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>  		BUG();
>  
>  	/* Find the last extent in the file */
> -	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>  		etype = netype;
>  		lbcount += elen;
>  		if (lbcount > inode->i_size) {

This should be checking for error (after the loop) so that we don't
accidentally try to truncate extents early in case of error.

> @@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>  			epos.offset -= adsize;
>  			extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
>  			epos.offset += adsize;
> -			if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
> +			if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
>  				udf_err(inode->i_sb,
>  					"Extent after EOF in inode %u\n",
>  					(unsigned)inode->i_ino);
> @@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
>  	epos.block = iinfo->i_location;
>  
>  	/* Find the last extent in the file */
> -	while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
>  		brelse(prev_epos.bh);
>  		prev_epos = epos;
>  		if (prev_epos.bh)
>  			get_bh(prev_epos.bh);
>  
> -		etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
> +		udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
>  		lbcount += elen;
>  	}
>  	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {

Again error checking for above two calls plus here we should not depend on
'etype' value after udf_next_aext() returned error. So we'll need another
temporary variable for etype to pass to the first udf_next_aext() call.

Thanks!

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

Re: [PATCH 3/3] udf: refactor inode_bmap() to handle error

[Jan Kara]

On Wed 18-09-24 17:36:34, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> Same as udf_current_aext(), take pointer to etype to store the extent
> type, while return 0 for success and <0 on error. On situations like
> ftruncate, udf_extend_file() can detect errors and bail out early
> without resorting to checking for particular offsets and
> assuming internal behavior of these functions.
> 
> Reported-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
> Tested-by: syzbot+7a4842f0b1801230a989@syzkaller.appspotmail.com
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

Overall looks good. I have a few comments below.

> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index ba980ce5e13a..f1b8f0a0d202 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -418,10 +418,11 @@ static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
>  		uint32_t elen;
>  		sector_t offset;
>  		struct extent_position epos = {};
> +		int8_t etype;
>  
>  		down_read(&iinfo->i_data_sem);
> -		if (inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset)
> -				== (EXT_RECORDED_ALLOCATED >> 30)) {
> +		inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset, &etype);

Here we should be checking for error and returning it...

> +		if (etype == (EXT_RECORDED_ALLOCATED >> 30)) {
>  			map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc,
>  							offset);
>  			map->oflags |= UDF_BLK_MAPPED;
> @@ -660,8 +661,10 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>  	 */
>  	udf_discard_prealloc(inode);
>  
> -	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> -	within_last_ext = (etype != -1);
> +	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
> +	if (err < 0 && err != -ENODATA)
> +		goto out;
> +	within_last_ext = (!err);
>  	/* We don't expect extents past EOF... */
>  	WARN_ON_ONCE(within_last_ext &&
>  		     elen > ((loff_t)offset + 1) << inode->i_blkbits);
> @@ -2363,14 +2366,19 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>  	return (elen >> 30);
>  }
>  
> -int8_t inode_bmap(struct inode *inode, sector_t block,
> -		  struct extent_position *pos, struct kernel_lb_addr *eloc,
> -		  uint32_t *elen, sector_t *offset)
> +/*
> + * return 0 when iudf_next_aext() loop success.
> + * return err < 0 and err != -ENODATA indicates error.
> + * return err == -ENODATA indicates hit EOF.
> + */
> +int inode_bmap(struct inode *inode, sector_t block, struct extent_position *pos,
> +	       struct kernel_lb_addr *eloc, uint32_t *elen, sector_t *offset,
> +	       int8_t *etype)
>  {
>  	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
>  	loff_t lbcount = 0, bcount = (loff_t) block << blocksize_bits;
> -	int8_t etype;
>  	struct udf_inode_info *iinfo;
> +	int err = 0;
>  
>  	iinfo = UDF_I(inode);
>  	if (!udf_read_extent_cache(inode, bcount, &lbcount, pos)) {
> @@ -2380,10 +2388,11 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>  	}
>  	*elen = 0;
>  	do {
> -		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
> +		err = udf_next_aext(inode, pos, eloc, elen, etype, 1);
> +		if (err < 0) {

OK, you've added the error handling in this patch. That is good. But still
we should be setting offset and i_lenExtents only in -ENODATA case.
Otherwise we just want to return the error.

>  			*offset = (bcount - lbcount) >> blocksize_bits;
>  			iinfo->i_lenExtents = lbcount;
> -			return -1;
> +			return err;
>  		}

...

> @@ -196,10 +197,10 @@ int udf_truncate_extents(struct inode *inode)
>  	else
>  		BUG();
>  
> -	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> +	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
>  	byte_offset = (offset << sb->s_blocksize_bits) +
>  		(inode->i_size & (sb->s_blocksize - 1));
> -	if (etype == -1) {
> +	if (err < 0) {

Here we should be distinguisting -ENODATA (WARN and return 0) and other
error (which we need to return).

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

Re: [PATCH 1/3] udf: refactor udf_current_aext() to handle error

[Zhao Mengmeng]

On 2024/9/20 23:09, Jan Kara wrote:
> On Wed 18-09-24 17:36:32, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> As Jan suggested in links below, refactor udf_current_aext() to
>> differentiate between error and "hit EOF", it now takes pointer to etype
>> to store the extent type, return 0 when get etype success; return -ENODATA
>> when hit EOF; return -EINVAL when i_alloc_type invalid.
>>
>> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> Two comments below.
> 
>> @@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>>  	if (epos->offset + adsize > sb->s_blocksize) {
>>  		struct kernel_lb_addr cp_loc;
>>  		uint32_t cp_len;
>> -		int cp_type;
>> +		int8_t cp_type;
>>  
>>  		epos->offset -= adsize;
>> -		cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
>> +		err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
>> +		if (err < 0)
>> +			goto err_out;
>>  		cp_len |= ((uint32_t)cp_type) << 30;
>>  
>>  		__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
>> @@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>>  	*epos = nepos;
>>  
>>  	return 0;
>> +err_out:
>> +	brelse(epos->bh);
>> +	return err;
>>  }
> 
> So here I don't think we want to release epos->bh. Rather we need to
> release 'bh' itself which we have got because we did't replace epos->bh yet
> with it.
Yes, should be bh as we haven't done `*epos = nepos`. Will fix.
 
>> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>>  {
>>  	int8_t etype;
>>  	unsigned int indirections = 0;
>> +	int err = 0;
>> +
>> +	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
>> +		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
>> +			break;
>>  
>> -	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
>> -	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
>>  		udf_pblk_t block;
>>  
>>  		if (++indirections > UDF_MAX_INDIR_EXTS) {
>> @@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>>  		}
>>  	}
>>  
>> -	return etype;
>> +	return err;
>>  }
> 
> This doesn't look right. Probably it gets fixed up in the following patches
> but here should be something like: !err ? etype : -1
> to keep udf_next_aext() compatible with its users.
> 
> Otherwise the patch looks good.
> 
> 								Honza
> 

Will do

Re: [PATCH 2/3] udf: refactor udf_next_aext() to handle error

[Zhao Mengmeng]

On 2024/9/20 23:47, Jan Kara wrote:
> On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> Same as udf_current_aext(), take pointer to etype to store the extent
>> type, while return 0 for success and <0 on error.
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> ...
> 
>> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
>> index 93153665eb37..f865538c985d 100644
>> --- a/fs/udf/directory.c
>> +++ b/fs/udf/directory.c
>> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>>   */
>>  static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>>  {
>> +	int8_t etype;
>> +	int err = 0;
> 
> Nit: please add empty line between declaration and the code.

Got it.
>>  	iter->loffset++;
>>  	if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>>  		return 0;
>>  
>>  	iter->loffset = 0;
>> -	if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
>> -			!= (EXT_RECORDED_ALLOCATED >> 30)) {
>> +	err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
>> +			    &etype, 1);
>> +	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>>  		if (iter->pos == iter->dir->i_size) {
>>  			iter->elen = 0;
>>  			return 0;
> 
> ...
> 
>> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>>  		 * empty indirect extent.
>>  		 */
>>  		if (new_block_bytes)
>> -			udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
>> +			udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>>  	}
>>  	iinfo->i_lenExtents += add;
>>  
> 
> Hum, this will need error checking but we can leave that for future
> patches.

Yes, will add in this series.
>> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>>  		extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>>  	} else {
>>  		epos.offset -= adsize;
>> -		etype = udf_next_aext(inode, &epos, &extent.extLocation,
>> -				      &extent.extLength, 0);
>> +		udf_next_aext(inode, &epos, &extent.extLocation,
>> +				&extent.extLength, &etype, 0);
>>  		extent.extLength |= etype << 30;
>>  	}
>>  
>> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	loff_t lbcount = 0, b_off = 0;
>>  	udf_pblk_t newblocknum;
>>  	sector_t offset = 0;
>> -	int8_t etype;
>> +	int8_t etype, tmpetype;
>>  	struct udf_inode_info *iinfo = UDF_I(inode);
>>  	udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>>  	int lastblock = 0;
>> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  		prev_epos.offset = cur_epos.offset;
>>  		cur_epos.offset = next_epos.offset;
>>  
>> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
>> -		if (etype == -1)
>> +		ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
>> +		if (ret)
>>  			break;
> 
> I think here we need to add error handling as well and we should probably
> do it in this patch / patch series. If ret is ENODATA, we just break out
> from the cycle but if ret is some other error, we need to return that error
> from inode_getblk().
> 
>> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	 * Move prev_epos and cur_epos into indirect extent if we are at
>>  	 * the pointer to it
>>  	 */
>> -	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
>> -	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
>> +	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>> +	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> 
> Again, this should have error handling now.
> 
>>  
>>  	/* if the extent is allocated and recorded, return the block
>>  	   if the extent is not a multiple of the blocksize, round up */
>> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	}
>>  
>>  	/* Are we beyond EOF and preallocated extent? */
>> -	if (etype == -1) {
>> +	if (ret < 0) {
> 
> I'd prefer ret == -ENODATA to make this explicit.
> 
>>  		loff_t hole_len;
>>  
>>  		isBeyondEOF = true;
>> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  
>>  		/* if the current block is located in an extent,
>>  		   read the next extent */
>> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
>> -		if (etype != -1) {
>> +		if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>>  			laarr[c + 1].extLength = (etype << 30) | elen;
>>  			laarr[c + 1].extLocation = eloc;
>>  			count++;
> 
> And this should be distinguisting between EOF and other errors so that we
> don't set lastblock wrongly. Instead we should bail with error.
> 
>> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>>  			if (err < 0)
>>  				return err;
>>  			udf_next_aext(inode, epos, &laarr[i].extLocation,
>> -				      &laarr[i].extLength, 1);
>> +				      &laarr[i].extLength, &tmptype, 1);
>>  			start++;
>>  		}
>>  	}
>>  
>>  	for (i = start; i < endnum; i++) {
>> -		udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
>> +		udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>>  		udf_write_aext(inode, epos, &laarr[i].extLocation,
>>  			       laarr[i].extLength, 1);
>>  	}
> 
> Again these two calls should have error handling now. udf_update_extents()
> is already able to return errors.
> 
>> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>>  	if (epos.bh)
>>  		get_bh(epos.bh);
>>  
>> -	while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>>  		udf_write_aext(inode, &epos, &neloc, nelen, 1);
>>  		neloc = oeloc;
>>  		nelen = (etype << 30) | oelen;
> 
> Here, we should check if udf_next_aext() returned error (other than
> ENODATA) and bail in that case instead of trying to insert new extent.
> 
>> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>>  		adsize = 0;
>>  
>>  	oepos = epos;
>> -	if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
>> +	if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>>  		return -1;
>>  
>> -	while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>>  		udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>>  		if (oepos.bh != epos.bh) {
>>  			oepos.block = epos.block;
>> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>>  	}
>>  	*elen = 0;
>>  	do {
>> -		etype = udf_next_aext(inode, pos, eloc, elen, 1);
>> -		if (etype == -1) {
>> +		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>>  			*offset = (bcount - lbcount) >> blocksize_bits;
>>  			iinfo->i_lenExtents = lbcount;
>>  			return -1;
> 
> Again, here we need to distinguish ENODATA from other errors so that we
> don't wrongly consider failure to read extent like EOF.
> 
>> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
>> index 91b6e2698e7e..b7361222f988 100644
>> --- a/fs/udf/truncate.c
>> +++ b/fs/udf/truncate.c
>> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>>  		BUG();
>>  
>>  	/* Find the last extent in the file */
>> -	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>>  		etype = netype;
>>  		lbcount += elen;
>>  		if (lbcount > inode->i_size) {
> 
> This should be checking for error (after the loop) so that we don't
> accidentally try to truncate extents early in case of error.
> 
>> @@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>>  			epos.offset -= adsize;
>>  			extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
>>  			epos.offset += adsize;
>> -			if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
>> +			if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
>>  				udf_err(inode->i_sb,
>>  					"Extent after EOF in inode %u\n",
>>  					(unsigned)inode->i_ino);
>> @@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
>>  	epos.block = iinfo->i_location;
>>  
>>  	/* Find the last extent in the file */
>> -	while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
>>  		brelse(prev_epos.bh);
>>  		prev_epos = epos;
>>  		if (prev_epos.bh)
>>  			get_bh(prev_epos.bh);
>>  
>> -		etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
>> +		udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
>>  		lbcount += elen;
>>  	}
>>  	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
> 
> Again error checking for above two calls plus here we should not depend on
> 'etype' value after udf_next_aext() returned error. So we'll need another
> temporary variable for etype to pass to the first udf_next_aext() call.
> 
> Thanks!
> 
> 								Honza

Will fix them in V2. Thanks.

Re: [PATCH 2/3] udf: refactor udf_next_aext() to handle error

[Zhao Mengmeng]

On 2024/9/20 23:47, Jan Kara wrote:
> On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> Same as udf_current_aext(), take pointer to etype to store the extent
>> type, while return 0 for success and <0 on error.
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> ...
> 
>> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
>> index 93153665eb37..f865538c985d 100644
>> --- a/fs/udf/directory.c
>> +++ b/fs/udf/directory.c
>> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>>   */
>>  static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>>  {
>> +	int8_t etype;
>> +	int err = 0;
> 
> Nit: please add empty line between declaration and the code.
> 
>>  	iter->loffset++;
>>  	if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>>  		return 0;
>>  
>>  	iter->loffset = 0;
>> -	if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
>> -			!= (EXT_RECORDED_ALLOCATED >> 30)) {
>> +	err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
>> +			    &etype, 1);
>> +	if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>>  		if (iter->pos == iter->dir->i_size) {
>>  			iter->elen = 0;
>>  			return 0;
> 
> ...
> 
>> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>>  		 * empty indirect extent.
>>  		 */
>>  		if (new_block_bytes)
>> -			udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
>> +			udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>>  	}
>>  	iinfo->i_lenExtents += add;
>>  
> 
> Hum, this will need error checking but we can leave that for future
> patches.
> 
>> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>>  		extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>>  	} else {
>>  		epos.offset -= adsize;
>> -		etype = udf_next_aext(inode, &epos, &extent.extLocation,
>> -				      &extent.extLength, 0);
>> +		udf_next_aext(inode, &epos, &extent.extLocation,
>> +				&extent.extLength, &etype, 0);
>>  		extent.extLength |= etype << 30;
>>  	}
>>  
>> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	loff_t lbcount = 0, b_off = 0;
>>  	udf_pblk_t newblocknum;
>>  	sector_t offset = 0;
>> -	int8_t etype;
>> +	int8_t etype, tmpetype;
>>  	struct udf_inode_info *iinfo = UDF_I(inode);
>>  	udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>>  	int lastblock = 0;
>> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  		prev_epos.offset = cur_epos.offset;
>>  		cur_epos.offset = next_epos.offset;
>>  
>> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
>> -		if (etype == -1)
>> +		ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
>> +		if (ret)
>>  			break;
> 
> I think here we need to add error handling as well and we should probably
> do it in this patch / patch series. If ret is ENODATA, we just break out
> from the cycle but if ret is some other error, we need to return that error
> from inode_getblk().
> 
>> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	 * Move prev_epos and cur_epos into indirect extent if we are at
>>  	 * the pointer to it
>>  	 */
>> -	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
>> -	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
>> +	udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>> +	udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> 
> Again, this should have error handling now.
> 
>>  
>>  	/* if the extent is allocated and recorded, return the block
>>  	   if the extent is not a multiple of the blocksize, round up */
>> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  	}
>>  
>>  	/* Are we beyond EOF and preallocated extent? */
>> -	if (etype == -1) {
>> +	if (ret < 0) {
> 
> I'd prefer ret == -ENODATA to make this explicit.
> 
>>  		loff_t hole_len;
>>  
>>  		isBeyondEOF = true;
>> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>  
>>  		/* if the current block is located in an extent,
>>  		   read the next extent */
>> -		etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
>> -		if (etype != -1) {
>> +		if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>>  			laarr[c + 1].extLength = (etype << 30) | elen;
>>  			laarr[c + 1].extLocation = eloc;
>>  			count++;
> 
> And this should be distinguisting between EOF and other errors so that we
> don't set lastblock wrongly. Instead we should bail with error.
> 
>> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>>  			if (err < 0)
>>  				return err;
>>  			udf_next_aext(inode, epos, &laarr[i].extLocation,
>> -				      &laarr[i].extLength, 1);
>> +				      &laarr[i].extLength, &tmptype, 1);
>>  			start++;
>>  		}
>>  	}
>>  
>>  	for (i = start; i < endnum; i++) {
>> -		udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
>> +		udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>>  		udf_write_aext(inode, epos, &laarr[i].extLocation,
>>  			       laarr[i].extLength, 1);
>>  	}
> 
> Again these two calls should have error handling now. udf_update_extents()
> is already able to return errors.
> 
>> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>>  	if (epos.bh)
>>  		get_bh(epos.bh);
>>  
>> -	while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>>  		udf_write_aext(inode, &epos, &neloc, nelen, 1);
>>  		neloc = oeloc;
>>  		nelen = (etype << 30) | oelen;
> 
> Here, we should check if udf_next_aext() returned error (other than
> ENODATA) and bail in that case instead of trying to insert new extent.
> 
>> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>>  		adsize = 0;
>>  
>>  	oepos = epos;
>> -	if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
>> +	if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>>  		return -1;
>>  
>> -	while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>>  		udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>>  		if (oepos.bh != epos.bh) {
>>  			oepos.block = epos.block;
>> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>>  	}
>>  	*elen = 0;
>>  	do {
>> -		etype = udf_next_aext(inode, pos, eloc, elen, 1);
>> -		if (etype == -1) {
>> +		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>>  			*offset = (bcount - lbcount) >> blocksize_bits;
>>  			iinfo->i_lenExtents = lbcount;
>>  			return -1;
> 
> Again, here we need to distinguish ENODATA from other errors so that we
> don't wrongly consider failure to read extent like EOF.
> 
>> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
>> index 91b6e2698e7e..b7361222f988 100644
>> --- a/fs/udf/truncate.c
>> +++ b/fs/udf/truncate.c
>> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>>  		BUG();
>>  
>>  	/* Find the last extent in the file */
>> -	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>>  		etype = netype;
>>  		lbcount += elen;
>>  		if (lbcount > inode->i_size) {
> 
> This should be checking for error (after the loop) so that we don't
> accidentally try to truncate extents early in case of error.
> 
Sorry to bother, in case of error(including EOF), it won't go into the loop and
has chance to call extent_trunc(). After the loop, only some update and clean op,

	iinfo->i_lenExtents = inode->i_size;
	brelse(epos.bh);

So I'm a little confused which part of this piece of code needs to change?
 

Re: [PATCH 2/3] udf: refactor udf_next_aext() to handle error

[Jan Kara]

On Tue 24-09-24 20:08:56, Zhao Mengmeng wrote:
> On 2024/9/20 23:47, Jan Kara wrote:
> > On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
> >> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> >> index 91b6e2698e7e..b7361222f988 100644
> >> --- a/fs/udf/truncate.c
> >> +++ b/fs/udf/truncate.c
> >> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
> >>  		BUG();
> >>  
> >>  	/* Find the last extent in the file */
> >> -	while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> >> +	while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
> >>  		etype = netype;
> >>  		lbcount += elen;
> >>  		if (lbcount > inode->i_size) {
> > 
> > This should be checking for error (after the loop) so that we don't
> > accidentally try to truncate extents early in case of error.
> > 
> Sorry to bother, in case of error(including EOF), it won't go into the loop and
> has chance to call extent_trunc(). After the loop, only some update and clean op,
> 
> 	iinfo->i_lenExtents = inode->i_size;
> 	brelse(epos.bh);
> 
> So I'm a little confused which part of this piece of code needs to change?

So if we are not able to scan until EOF due to error, we should set
i_lenExtents to i_size but you're right this is mostly a cosmetic thing.

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