[PATCH v1] erofs-utils: lib: Fix 8MB bug on uncompressed extent size

[Gao Xiang]

Hi Kelvin,

On Tue, Dec 21, 2021 at 06:03:07PM -0800, Kelvin Zhang wrote:
> Previously, uncompressed extent can be at most 8MB before mkfs.erofs
> crashes on some error condition. This is due to a minor bug in how
> compressed indices are encoded. This patch fixes the issue.
> 
> Signed-off-by: Kelvin Zhang <zhangkelvin at google.com>
> ---
>  include/erofs_fs.h |  2 +-
>  lib/compress.c     | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> index 9a91877..13eaf24 100644
> --- a/include/erofs_fs.h
> +++ b/include/erofs_fs.h
> @@ -353,7 +353,7 @@ enum {
>   * compressed block count of a compressed extent (in logical clusters, aka.
>   * block count of a pcluster).
>   */
> -#define Z_EROFS_VLE_DI_D0_CBLKCNT		(1 << 11)
> +#define Z_EROFS_VLE_DI_D0_CBLKCNT		(1U << 11)

If erofs_fs.h update is necessary, I prefer to update in-kernel
header first. Would you mind making a kernel patch for this if needed?

>  
>  struct z_erofs_vle_decompressed_index {
>  	__le16 di_advise;
> diff --git a/lib/compress.c b/lib/compress.c
> index 98be7a2..23e571c 100644
> --- a/lib/compress.c
> +++ b/lib/compress.c
> @@ -97,7 +97,26 @@ static void vle_write_indexes(struct z_erofs_vle_compress_ctx *ctx,
>  		} else if (d0) {
>  			type = Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD;
>  
> -			di.di_u.delta[0] = cpu_to_le16(d0);
> +			/* If the |Z_EROFS_VLE_DI_D0_CBLKCNT| bit is set, parser
> +			 * will interpret |delta[0]| as size of pcluster, rather
> +			 * than distance to last head cluster. Normally this
> +			 * isn't a problem, because uncompressed extent size are
> +			 * below Z_EROFS_VLE_DI_D0_CBLKCNT * BLOCK_SIZE = 8MB.
> +			 * But with large pcluster it's possible to go over this
> +			 * number, resulting in corrupted compressed indices.
> +			 * To solve this, we use Z_EROFS_VLE_DI_D0_CBLKCNT-1 if
> +			 * the uncompressed extent size goes above 8MB. This is
> +			 * OK because if kernel sees another non-head cluster
> +			 * after going back by |delta[0]| blocks, kernel will
> +			 * just keep looking back.
> +			 */

Would you mind updating this into the kernel comment style, I mean
/*
 * ...
 */
Instead?

> +			if (d0 & Z_EROFS_VLE_DI_D0_CBLKCNT) {
> +				di.di_u.delta[0] = max(
> +					d0 & (~Z_EROFS_VLE_DI_D0_CBLKCNT),
> +					Z_EROFS_VLE_DI_D0_CBLKCNT-1);

May I ask if it's actually tested with big pcluster feature? It's
lack of cpu_to_le16() convert and even the original
Z_EROFS_VLE_DI_D0_CBLKCNT flag.

Thanks,
Gao Xiang

> +			} else {
> +				di.di_u.delta[0] = cpu_to_le16(d0);
> +			}
>  			di.di_u.delta[1] = cpu_to_le16(d1);
>  		} else {
>  			type = raw ? Z_EROFS_VLE_CLUSTER_TYPE_PLAIN :
> -- 
> 2.34.1.448.ga2b2bfdf31-goog