1. Patchew
  2. linux
  3. View series

[PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()

Chao Yu posted 1 patch 3 months ago
There is a newer version of this series
fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
1 file changed, 22 insertions(+), 38 deletions(-)
[PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Chao Yu 3 months ago 
All below functions will do sanity check on m->type, let's move sanity
check to z_erofs_load_compact_lcluster() for cleanup.
- z_erofs_map_blocks_fo
- z_erofs_get_extent_compressedlen
- z_erofs_get_extent_decompressedlen
- z_erofs_extent_lookback

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 38 deletions(-)

diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 0bebc6e3a4d7..e530b152e14e 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -240,6 +240,13 @@ static int z_erofs_load_compact_lcluster(struct z_erofs_maprecorder *m,
 static int z_erofs_load_lcluster_from_disk(struct z_erofs_maprecorder *m,
 					   unsigned int lcn, bool lookahead)
 {
+	if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
+		erofs_err(m->inode->i_sb, "unknown type %u @ lcn %u of nid %llu",
+				m->type, lcn, EROFS_I(m->inode)->nid);
+		DBG_BUGON(1);
+		return -EOPNOTSUPP;
+	}
+
 	switch (EROFS_I(m->inode)->datalayout) {
 	case EROFS_INODE_COMPRESSED_FULL:
 		return z_erofs_load_full_lcluster(m, lcn);
@@ -265,12 +272,7 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
 		if (err)
 			return err;
 
-		if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
-			erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
-				  m->type, lcn, vi->nid);
-			DBG_BUGON(1);
-			return -EOPNOTSUPP;
-		} else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
+		if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
 			lookback_distance = m->delta[0];
 			if (!lookback_distance)
 				break;
@@ -333,17 +335,13 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
 		}
 		if (m->compressedblks)
 			goto out;
-	} else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
-		/*
-		 * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type
-		 * rather than CBLKCNT, it's a 1 block-sized pcluster.
-		 */
-		m->compressedblks = 1;
-		goto out;
 	}
-	erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
-	DBG_BUGON(1);
-	return -EFSCORRUPTED;
+
+	/*
+	 * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type rather
+	 * than CBLKCNT, it's a 1 block-sized pcluster.
+	 */
+	m->compressedblks = 1;
 out:
 	m->map->m_plen = erofs_pos(sb, m->compressedblks);
 	return 0;
@@ -379,11 +377,6 @@ static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
 			if (lcn != headlcn)
 				break;	/* ends at the next HEAD lcluster */
 			m->delta[1] = 1;
-		} else {
-			erofs_err(inode->i_sb, "unknown type %u @ lcn %llu of nid %llu",
-				  m->type, lcn, vi->nid);
-			DBG_BUGON(1);
-			return -EOPNOTSUPP;
 		}
 		lcn += m->delta[1];
 	}
@@ -429,10 +422,7 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
 	map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED;
 	end = (m.lcn + 1ULL) << lclusterbits;
 
-	switch (m.type) {
-	case Z_EROFS_LCLUSTER_TYPE_PLAIN:
-	case Z_EROFS_LCLUSTER_TYPE_HEAD1:
-	case Z_EROFS_LCLUSTER_TYPE_HEAD2:
+	if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
 		if (endoff >= m.clusterofs) {
 			m.headtype = m.type;
 			map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
@@ -443,7 +433,7 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
 			 */
 			if (ztailpacking && end > inode->i_size)
 				end = inode->i_size;
-			break;
+			goto map_block;
 		}
 		/* m.lcn should be >= 1 if endoff < m.clusterofs */
 		if (!m.lcn) {
@@ -455,19 +445,13 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
 		end = (m.lcn << lclusterbits) | m.clusterofs;
 		map->m_flags |= EROFS_MAP_FULL_MAPPED;
 		m.delta[0] = 1;
-		fallthrough;
-	case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
-		/* get the corresponding first chunk */
-		err = z_erofs_extent_lookback(&m, m.delta[0]);
-		if (err)
-			goto unmap_out;
-		break;
-	default:
-		erofs_err(sb, "unknown type %u @ offset %llu of nid %llu",
-			  m.type, ofs, vi->nid);
-		err = -EOPNOTSUPP;
-		goto unmap_out;
 	}
+
+	/* get the corresponding first chunk */
+	err = z_erofs_extent_lookback(&m, m.delta[0]);
+	if (err)
+		goto unmap_out;
+map_block:
 	if (m.partialref)
 		map->m_flags |= EROFS_MAP_PARTIAL_REF;
 	map->m_llen = end - map->m_la;
-- 
2.49.0
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Gao Xiang 3 months ago 
Hi Chao,

On 2025/7/7 16:47, Chao Yu wrote:
> All below functions will do sanity check on m->type, let's move sanity
> check to z_erofs_load_compact_lcluster() for cleanup.
> - z_erofs_map_blocks_fo
> - z_erofs_get_extent_compressedlen
> - z_erofs_get_extent_decompressedlen
> - z_erofs_extent_lookback
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
How about appending the following diff to tidy up the code
a bit further (avoid `goto map_block` and `goto out`)?


  fs/erofs/zmap.c | 67 +++++++++++++++++++++++--------------------------
  1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index e530b152e14e..431199452542 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -327,21 +327,18 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
  	DBG_BUGON(lcn == initial_lcn &&
  		  m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD);
  
-	if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
-		if (m->delta[0] != 1) {
-			erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
-			DBG_BUGON(1);
-			return -EFSCORRUPTED;
-		}
-		if (m->compressedblks)
-			goto out;
+	if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD && m->delta[0] != 1) {
+		erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
+		DBG_BUGON(1);
+		return -EFSCORRUPTED;
  	}
  
  	/*
  	 * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type rather
  	 * than CBLKCNT, it's a 1 block-sized pcluster.
  	 */
-	m->compressedblks = 1;
+	if (m->type != Z_EROFS_LCLUSTER_TYPE_NONHEAD || !m->compressedblks)
+		m->compressedblks = 1;
  out:
  	m->map->m_plen = erofs_pos(sb, m->compressedblks);
  	return 0;
@@ -422,36 +419,34 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
  	map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED;
  	end = (m.lcn + 1ULL) << lclusterbits;
  
-	if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
-		if (endoff >= m.clusterofs) {
-			m.headtype = m.type;
-			map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
-			/*
-			 * For ztailpacking files, in order to inline data more
-			 * effectively, special EOF lclusters are now supported
-			 * which can have three parts at most.
-			 */
-			if (ztailpacking && end > inode->i_size)
-				end = inode->i_size;
-			goto map_block;
+	if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD && endoff >= m.clusterofs) {
+		m.headtype = m.type;
+		map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
+		/*
+		 * For ztailpacking files, in order to inline data more
+		 * effectively, special EOF lclusters are now supported
+		 * which can have three parts at most.
+		 */
+		if (ztailpacking && end > inode->i_size)
+			end = inode->i_size;
+	} else {
+		if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
+			/* m.lcn should be >= 1 if endoff < m.clusterofs */
+			if (!m.lcn) {
+				erofs_err(sb, "invalid logical cluster 0 at nid %llu",
+					  vi->nid);
+				err = -EFSCORRUPTED;
+				goto unmap_out;
+			}
+			end = (m.lcn << lclusterbits) | m.clusterofs;
+			map->m_flags |= EROFS_MAP_FULL_MAPPED;
+			m.delta[0] = 1;
  		}
-		/* m.lcn should be >= 1 if endoff < m.clusterofs */
-		if (!m.lcn) {
-			erofs_err(sb, "invalid logical cluster 0 at nid %llu",
-				  vi->nid);
-			err = -EFSCORRUPTED;
+		/* get the corresponding first chunk */
+		err = z_erofs_extent_lookback(&m, m.delta[0]);
+		if (err)
  			goto unmap_out;
-		}
-		end = (m.lcn << lclusterbits) | m.clusterofs;
-		map->m_flags |= EROFS_MAP_FULL_MAPPED;
-		m.delta[0] = 1;
  	}
-
-	/* get the corresponding first chunk */
-	err = z_erofs_extent_lookback(&m, m.delta[0]);
-	if (err)
-		goto unmap_out;
-map_block:
  	if (m.partialref)
  		map->m_flags |= EROFS_MAP_PARTIAL_REF;
  	map->m_llen = end - map->m_la;
-- 
2.43.5
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Chao Yu 3 months ago 
On 7/8/25 10:58, Gao Xiang wrote:
> Hi Chao,
> 
> On 2025/7/7 16:47, Chao Yu wrote:
>> All below functions will do sanity check on m->type, let's move sanity
>> check to z_erofs_load_compact_lcluster() for cleanup.
>> - z_erofs_map_blocks_fo
>> - z_erofs_get_extent_compressedlen
>> - z_erofs_get_extent_decompressedlen
>> - z_erofs_extent_lookback
>>
>> Signed-off-by: Chao Yu <chao@kernel.org>
> How about appending the following diff to tidy up the code
> a bit further (avoid `goto map_block` and `goto out`)?

Xiang,

Looks good to me, will append the diff in the patch.

Thanks,

> 
> 
>  fs/erofs/zmap.c | 67 +++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index e530b152e14e..431199452542 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -327,21 +327,18 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
>      DBG_BUGON(lcn == initial_lcn &&
>            m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD);
>  
> -    if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> -        if (m->delta[0] != 1) {
> -            erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> -            DBG_BUGON(1);
> -            return -EFSCORRUPTED;
> -        }
> -        if (m->compressedblks)
> -            goto out;
> +    if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD && m->delta[0] != 1) {
> +        erofs_err(sb, "bogus CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> +        DBG_BUGON(1);
> +        return -EFSCORRUPTED;
>      }
>  
>      /*
>       * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type rather
>       * than CBLKCNT, it's a 1 block-sized pcluster.
>       */
> -    m->compressedblks = 1;
> +    if (m->type != Z_EROFS_LCLUSTER_TYPE_NONHEAD || !m->compressedblks)
> +        m->compressedblks = 1;
>  out:
>      m->map->m_plen = erofs_pos(sb, m->compressedblks);
>      return 0;
> @@ -422,36 +419,34 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
>      map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED;
>      end = (m.lcn + 1ULL) << lclusterbits;
>  
> -    if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> -        if (endoff >= m.clusterofs) {
> -            m.headtype = m.type;
> -            map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
> -            /*
> -             * For ztailpacking files, in order to inline data more
> -             * effectively, special EOF lclusters are now supported
> -             * which can have three parts at most.
> -             */
> -            if (ztailpacking && end > inode->i_size)
> -                end = inode->i_size;
> -            goto map_block;
> +    if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD && endoff >= m.clusterofs) {
> +        m.headtype = m.type;
> +        map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
> +        /*
> +         * For ztailpacking files, in order to inline data more
> +         * effectively, special EOF lclusters are now supported
> +         * which can have three parts at most.
> +         */
> +        if (ztailpacking && end > inode->i_size)
> +            end = inode->i_size;
> +    } else {
> +        if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> +            /* m.lcn should be >= 1 if endoff < m.clusterofs */
> +            if (!m.lcn) {
> +                erofs_err(sb, "invalid logical cluster 0 at nid %llu",
> +                      vi->nid);
> +                err = -EFSCORRUPTED;
> +                goto unmap_out;
> +            }
> +            end = (m.lcn << lclusterbits) | m.clusterofs;
> +            map->m_flags |= EROFS_MAP_FULL_MAPPED;
> +            m.delta[0] = 1;
>          }
> -        /* m.lcn should be >= 1 if endoff < m.clusterofs */
> -        if (!m.lcn) {
> -            erofs_err(sb, "invalid logical cluster 0 at nid %llu",
> -                  vi->nid);
> -            err = -EFSCORRUPTED;
> +        /* get the corresponding first chunk */
> +        err = z_erofs_extent_lookback(&m, m.delta[0]);
> +        if (err)
>              goto unmap_out;
> -        }
> -        end = (m.lcn << lclusterbits) | m.clusterofs;
> -        map->m_flags |= EROFS_MAP_FULL_MAPPED;
> -        m.delta[0] = 1;
>      }
> -
> -    /* get the corresponding first chunk */
> -    err = z_erofs_extent_lookback(&m, m.delta[0]);
> -    if (err)
> -        goto unmap_out;
> -map_block:
>      if (m.partialref)
>          map->m_flags |= EROFS_MAP_PARTIAL_REF;
>      map->m_llen = end - map->m_la;
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Hongbo Li 3 months ago 

On 2025/7/7 16:47, Chao Yu wrote:
> All below functions will do sanity check on m->type, let's move sanity
> check to z_erofs_load_compact_lcluster() for cleanup.
> - z_erofs_map_blocks_fo
> - z_erofs_get_extent_compressedlen
> - z_erofs_get_extent_decompressedlen
> - z_erofs_extent_lookback
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>   fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
>   1 file changed, 22 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index 0bebc6e3a4d7..e530b152e14e 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -240,6 +240,13 @@ static int z_erofs_load_compact_lcluster(struct z_erofs_maprecorder *m,
>   static int z_erofs_load_lcluster_from_disk(struct z_erofs_maprecorder *m,
>   					   unsigned int lcn, bool lookahead)
>   {
> +	if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
> +		erofs_err(m->inode->i_sb, "unknown type %u @ lcn %u of nid %llu",
> +				m->type, lcn, EROFS_I(m->inode)->nid);
> +		DBG_BUGON(1);
> +		return -EOPNOTSUPP;
> +	}
> +

Hi, Chao,

After moving the condition in here, there is no need to check in 
z_erofs_extent_lookback, z_erofs_get_extent_compressedlen and 
z_erofs_get_extent_decompressedlen. Because in z_erofs_map_blocks_fo, 
the condition has been checked in before. Right?

Thanks,
Hongbo

>   	switch (EROFS_I(m->inode)->datalayout) {
>   	case EROFS_INODE_COMPRESSED_FULL:
>   		return z_erofs_load_full_lcluster(m, lcn);
> @@ -265,12 +272,7 @@ static int z_erofs_extent_lookback(struct z_erofs_maprecorder *m,
>   		if (err)
>   			return err;
>   
> -		if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
> -			erofs_err(sb, "unknown type %u @ lcn %lu of nid %llu",
> -				  m->type, lcn, vi->nid);
> -			DBG_BUGON(1);
> -			return -EOPNOTSUPP;
> -		} else if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
> +		if (m->type == Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
>   			lookback_distance = m->delta[0];
>   			if (!lookback_distance)
>   				break;
> @@ -333,17 +335,13 @@ static int z_erofs_get_extent_compressedlen(struct z_erofs_maprecorder *m,
>   		}
>   		if (m->compressedblks)
>   			goto out;
> -	} else if (m->type < Z_EROFS_LCLUSTER_TYPE_MAX) {
> -		/*
> -		 * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type
> -		 * rather than CBLKCNT, it's a 1 block-sized pcluster.
> -		 */
> -		m->compressedblks = 1;
> -		goto out;
>   	}
> -	erofs_err(sb, "cannot found CBLKCNT @ lcn %lu of nid %llu", lcn, vi->nid);
> -	DBG_BUGON(1);
> -	return -EFSCORRUPTED;
> +
> +	/*
> +	 * if the 1st NONHEAD lcluster is actually PLAIN or HEAD type rather
> +	 * than CBLKCNT, it's a 1 block-sized pcluster.
> +	 */
> +	m->compressedblks = 1;
>   out:
>   	m->map->m_plen = erofs_pos(sb, m->compressedblks);
>   	return 0;
> @@ -379,11 +377,6 @@ static int z_erofs_get_extent_decompressedlen(struct z_erofs_maprecorder *m)
>   			if (lcn != headlcn)
>   				break;	/* ends at the next HEAD lcluster */
>   			m->delta[1] = 1;
> -		} else {
> -			erofs_err(inode->i_sb, "unknown type %u @ lcn %llu of nid %llu",
> -				  m->type, lcn, vi->nid);
> -			DBG_BUGON(1);
> -			return -EOPNOTSUPP;
>   		}
>   		lcn += m->delta[1];
>   	}
> @@ -429,10 +422,7 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
>   	map->m_flags = EROFS_MAP_MAPPED | EROFS_MAP_ENCODED;
>   	end = (m.lcn + 1ULL) << lclusterbits;
>   
> -	switch (m.type) {
> -	case Z_EROFS_LCLUSTER_TYPE_PLAIN:
> -	case Z_EROFS_LCLUSTER_TYPE_HEAD1:
> -	case Z_EROFS_LCLUSTER_TYPE_HEAD2:
> +	if (m.type != Z_EROFS_LCLUSTER_TYPE_NONHEAD) {
>   		if (endoff >= m.clusterofs) {
>   			m.headtype = m.type;
>   			map->m_la = (m.lcn << lclusterbits) | m.clusterofs;
> @@ -443,7 +433,7 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
>   			 */
>   			if (ztailpacking && end > inode->i_size)
>   				end = inode->i_size;
> -			break;
> +			goto map_block;
>   		}
>   		/* m.lcn should be >= 1 if endoff < m.clusterofs */
>   		if (!m.lcn) {
> @@ -455,19 +445,13 @@ static int z_erofs_map_blocks_fo(struct inode *inode,
>   		end = (m.lcn << lclusterbits) | m.clusterofs;
>   		map->m_flags |= EROFS_MAP_FULL_MAPPED;
>   		m.delta[0] = 1;
> -		fallthrough;
> -	case Z_EROFS_LCLUSTER_TYPE_NONHEAD:
> -		/* get the corresponding first chunk */
> -		err = z_erofs_extent_lookback(&m, m.delta[0]);
> -		if (err)
> -			goto unmap_out;
> -		break;
> -	default:
> -		erofs_err(sb, "unknown type %u @ offset %llu of nid %llu",
> -			  m.type, ofs, vi->nid);
> -		err = -EOPNOTSUPP;
> -		goto unmap_out;
>   	}
> +
> +	/* get the corresponding first chunk */
> +	err = z_erofs_extent_lookback(&m, m.delta[0]);
> +	if (err)
> +		goto unmap_out;
> +map_block:
>   	if (m.partialref)
>   		map->m_flags |= EROFS_MAP_PARTIAL_REF;
>   	map->m_llen = end - map->m_la;
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Gao Xiang 3 months ago 

On 2025/7/8 10:30, Hongbo Li wrote:
> 
> 
> On 2025/7/7 16:47, Chao Yu wrote:
>> All below functions will do sanity check on m->type, let's move sanity
>> check to z_erofs_load_compact_lcluster() for cleanup.
>> - z_erofs_map_blocks_fo
>> - z_erofs_get_extent_compressedlen
>> - z_erofs_get_extent_decompressedlen
>> - z_erofs_extent_lookback
>>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>   fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
>>   1 file changed, 22 insertions(+), 38 deletions(-)
>>
>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>> index 0bebc6e3a4d7..e530b152e14e 100644
>> --- a/fs/erofs/zmap.c
>> +++ b/fs/erofs/zmap.c
>> @@ -240,6 +240,13 @@ static int z_erofs_load_compact_lcluster(struct z_erofs_maprecorder *m,
>>   static int z_erofs_load_lcluster_from_disk(struct z_erofs_maprecorder *m,
>>                          unsigned int lcn, bool lookahead)
>>   {
>> +    if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>> +        erofs_err(m->inode->i_sb, "unknown type %u @ lcn %u of nid %llu",
>> +                m->type, lcn, EROFS_I(m->inode)->nid);
>> +        DBG_BUGON(1);
>> +        return -EOPNOTSUPP;
>> +    }
>> +
> 
> Hi, Chao,
> 
> After moving the condition in here, there is no need to check in z_erofs_extent_lookback, z_erofs_get_extent_compressedlen and z_erofs_get_extent_decompressedlen. Because in z_erofs_map_blocks_fo, the condition has been checked in before. Right?

I've replied some similar question.

Because z_erofs_get_extent_compressedlen and z_erofs_get_extent_decompressedlen()
use the different lcn (lcluster) against z_erofs_map_blocks_fo().

So if a new lcn(lcluster number) is loaded, we'd check if the type is valid.

Thanks,
Gao Xiang
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Chao Yu 3 months ago 
On 7/8/25 10:35, Gao Xiang wrote:
> 
> 
> On 2025/7/8 10:30, Hongbo Li wrote:
>>
>>
>> On 2025/7/7 16:47, Chao Yu wrote:
>>> All below functions will do sanity check on m->type, let's move sanity
>>> check to z_erofs_load_compact_lcluster() for cleanup.
>>> - z_erofs_map_blocks_fo
>>> - z_erofs_get_extent_compressedlen
>>> - z_erofs_get_extent_decompressedlen
>>> - z_erofs_extent_lookback
>>>
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>> ---
>>>   fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
>>>   1 file changed, 22 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>>> index 0bebc6e3a4d7..e530b152e14e 100644
>>> --- a/fs/erofs/zmap.c
>>> +++ b/fs/erofs/zmap.c
>>> @@ -240,6 +240,13 @@ static int z_erofs_load_compact_lcluster(struct z_erofs_maprecorder *m,
>>>   static int z_erofs_load_lcluster_from_disk(struct z_erofs_maprecorder *m,
>>>                          unsigned int lcn, bool lookahead)
>>>   {
>>> +    if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>> +        erofs_err(m->inode->i_sb, "unknown type %u @ lcn %u of nid %llu",
>>> +                m->type, lcn, EROFS_I(m->inode)->nid);
>>> +        DBG_BUGON(1);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +
>>
>> Hi, Chao,
>>
>> After moving the condition in here, there is no need to check in z_erofs_extent_lookback, z_erofs_get_extent_compressedlen and z_erofs_get_extent_decompressedlen. Because in z_erofs_map_blocks_fo,
>> the condition has been checked in before. Right?
> 
> I've replied some similar question.
> 
> Because z_erofs_get_extent_compressedlen and z_erofs_get_extent_decompressedlen()
> use the different lcn (lcluster) against z_erofs_map_blocks_fo().
> 
> So if a new lcn(lcluster number) is loaded, we'd check if the type is valid.

Yeah, Xiang has noticed that previously [1], the case is as below, so we'd better check the
condition in z_erofs_load_compact_lcluster() rather than z_erofs_map_blocks_fo():

- z_erofs_extent_lookback
 - z_erofs_load_lcluster_from_disk
  - z_erofs_load_full_lcluster
  : m->type = advise & Z_EROFS_LI_LCLUSTER_TYPE_MASK;
 - z_erofs_load_compact_lcluster
 : m->type = type;

[1] https://lore.kernel.org/linux-erofs/04050888-7abf-40fa-98d6-6215b8ba989e@kernel.org/

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>
Re: [PATCH] erofs: do sanity check on m->type in z_erofs_load_compact_lcluster()
Posted by Hongbo Li 3 months ago 

On 2025/7/8 10:35, Gao Xiang wrote:
> 
> 
> On 2025/7/8 10:30, Hongbo Li wrote:
>>
>>
>> On 2025/7/7 16:47, Chao Yu wrote:
>>> All below functions will do sanity check on m->type, let's move sanity
>>> check to z_erofs_load_compact_lcluster() for cleanup.
>>> - z_erofs_map_blocks_fo
>>> - z_erofs_get_extent_compressedlen
>>> - z_erofs_get_extent_decompressedlen
>>> - z_erofs_extent_lookback
>>>
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>> ---
>>>   fs/erofs/zmap.c | 60 ++++++++++++++++++-------------------------------
>>>   1 file changed, 22 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
>>> index 0bebc6e3a4d7..e530b152e14e 100644
>>> --- a/fs/erofs/zmap.c
>>> +++ b/fs/erofs/zmap.c
>>> @@ -240,6 +240,13 @@ static int z_erofs_load_compact_lcluster(struct 
>>> z_erofs_maprecorder *m,
>>>   static int z_erofs_load_lcluster_from_disk(struct 
>>> z_erofs_maprecorder *m,
>>>                          unsigned int lcn, bool lookahead)
>>>   {
>>> +    if (m->type >= Z_EROFS_LCLUSTER_TYPE_MAX) {
>>> +        erofs_err(m->inode->i_sb, "unknown type %u @ lcn %u of nid 
>>> %llu",
>>> +                m->type, lcn, EROFS_I(m->inode)->nid);
>>> +        DBG_BUGON(1);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +
>>
>> Hi, Chao,
>>
>> After moving the condition in here, there is no need to check in 
>> z_erofs_extent_lookback, z_erofs_get_extent_compressedlen and 
>> z_erofs_get_extent_decompressedlen. Because in z_erofs_map_blocks_fo, 
>> the condition has been checked in before. Right?
> 
> I've replied some similar question.
> 
> Because z_erofs_get_extent_compressedlen and 
> z_erofs_get_extent_decompressedlen()
> use the different lcn (lcluster) against z_erofs_map_blocks_fo().
> 
> So if a new lcn(lcluster number) is loaded, we'd check if the type is 
> valid.
> 
Ok, the type will change after loading.

Thanks,
Hongbo

> Thanks,
> Gao Xiang
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.