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

[Gao Xiang]

On Mon, Feb 07, 2022 at 07:59:50AM +0800, Gao Xiang wrote:
> 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.

Sorry this part shouldn't have Z_EROFS_VLE_DI_D0_CBLKCNT flag.

Btw, I think a proper change for this might be just:
	if (d0 > Z_EROFS_VLE_DI_D0_CBLKCNT - 1)
		di.di_u.delta[0] = le16_to_cpu(Z_EROFS_VLE_DI_D0_CBLKCNT - 1);
	else
		di.di_u.delta[0] = cpu_to_le16(d0);
Or using max() to simplify above even more a bit.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang