[PATCH] erofs: fix lz4 inplace decompression
Juhyung Park
qkrwngud825 at gmail.com
Wed Dec 6 15:28:52 AEDT 2023
Hi Gao,
Thanks for the timely fix.
This also fixes the issue.
Reported-and-tested-by: Juhyung Park <qkrwngud825 at gmail.com>
Might wanna add
Cc: stable at vger.kernel.org # 5.4+
but it seems like this doesn't apply cleanly for 5.4, 5.10 and 5.15
also requires some manual conflict resolution. :))
Wrote some nits below:
On Wed, Dec 6, 2023 at 12:08 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
>
> Currently EROFS can map another compressed buffer for inplace
> decompression, which was used to handle the cases that some pages of
> compressed data are actually not in-place I/O.
>
> However, like most simple LZ77 algorithms, LZ4 expects the compressed
> data is arranged at the end of the decompressed buffer and it
> explicitly uses memmove() to handle overlapping:
> __________________________________________________________
> |_ direction of decompression --> ____ |_ compressed data _|
>
> Although EROFS arranges compressed data like this, it typically map two
> individual virtual buffers so the relative order is uncertain.
> Previously, it was hardly observed since LZ4 only uses memmove() for
> short overlapped literals and x86/arm64 memmove implementations seem to
> completely cover it up so they don't have this issue. Juhyung reported
> that EROFS data corruption can be found on a new Intel x86 processor.
> After some analysis, it seems that recent x86 processors with the new
> FSRM feature expose this issue with "rep movsb".
>
> Let's strictly use the decompressed buffer for lz4 inplace
> decompression for now. Later, as an useful improvement, we could try
> to tie up these two buffers together in the correct order.
>
> Reported-by: Juhyung Park <qkrwngud825 at gmail.com>
> Closes: https://lore.kernel.org/r/CAD14+f2AVKf8Fa2OO1aAUdDNTDsVzzR6ctU_oJSmTyd6zSYR2Q@mail.gmail.com
> Fixes: 0ffd71bcc3a0 ("staging: erofs: introduce LZ4 decompression inplace")
> Fixes: 598162d05080 ("erofs: support decompress big pcluster for lz4 backend")
> Signed-off-by: Gao Xiang <hsiangkao at linux.alibaba.com>
> ---
> Hi Juhyung,
> Please help check if this patch resolves your issue.
> Also, I will test it in various hardware configurations first as well.
>
> fs/erofs/decompressor.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 5ec11f5024b7..5905d7c4d1db 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -121,11 +121,11 @@ static int z_erofs_lz4_prepare_dstpages(struct z_erofs_lz4_decompress_ctx *ctx,
> }
>
> static void *z_erofs_lz4_handle_overlap(struct z_erofs_lz4_decompress_ctx *ctx,
> - void *inpage, unsigned int *inputmargin, int *maptype,
> - bool may_inplace)
> + void *inpage, void *out, unsigned int *inputmargin,
> + int *maptype, bool may_inplace)
> {
> struct z_erofs_decompress_req *rq = ctx->rq;
> - unsigned int omargin, total, i, j;
> + unsigned int omargin, total, i;
> struct page **in;
> void *src, *tmp;
>
> @@ -135,20 +135,19 @@ static void *z_erofs_lz4_handle_overlap(struct z_erofs_lz4_decompress_ctx *ctx,
> omargin < LZ4_DECOMPRESS_INPLACE_MARGIN(rq->inputsize))
> goto docopy;
>
> - for (i = 0; i < ctx->inpages; ++i) {
> - DBG_BUGON(rq->in[i] == NULL);
> - for (j = 0; j < ctx->outpages - ctx->inpages + i; ++j)
> - if (rq->out[j] == rq->in[i])
> - goto docopy;
> - }
> + for (i = 0; i < ctx->inpages; ++i)
> + if (rq->out[ctx->outpages - ctx->inpages + i] !=
> + rq->in[i])
> + goto docopy;
Linux kernel coding guideline recommends placing braces on multi-line
nested condition statements.
See https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/process/coding-style.rst?h=v6.6#n225
> + kunmap_local(inpage);
> + *maptype = 3;
> + return out + ((ctx->outpages - ctx->inpages) << PAGE_SHIFT);
> }
> -
Unnecessary diff?
> if (ctx->inpages <= 1) {
> *maptype = 0;
> return inpage;
> }
> kunmap_local(inpage);
> - might_sleep();
> src = erofs_vm_map_ram(rq->in, ctx->inpages);
> if (!src)
> return ERR_PTR(-ENOMEM);
> @@ -204,12 +203,12 @@ int z_erofs_fixup_insize(struct z_erofs_decompress_req *rq, const char *padbuf,
> }
>
> static int z_erofs_lz4_decompress_mem(struct z_erofs_lz4_decompress_ctx *ctx,
> - u8 *out)
> + u8 *dst)
> {
> struct z_erofs_decompress_req *rq = ctx->rq;
> bool support_0padding = false, may_inplace = false;
> unsigned int inputmargin;
> - u8 *headpage, *src;
> + u8 *out = dst + rq->pageofs_out, *headpage, *src;
Move this initialization below "/* legacy format could compress extra
data in a pcluster. */" looks better imho.
> int ret, maptype;
>
> DBG_BUGON(*rq->in == NULL);
> @@ -230,7 +229,7 @@ static int z_erofs_lz4_decompress_mem(struct z_erofs_lz4_decompress_ctx *ctx,
> }
>
> inputmargin = rq->pageofs_in;
> - src = z_erofs_lz4_handle_overlap(ctx, headpage, &inputmargin,
> + src = z_erofs_lz4_handle_overlap(ctx, headpage, dst, &inputmargin,
> &maptype, may_inplace);
> if (IS_ERR(src))
> return PTR_ERR(src);
> @@ -265,7 +264,7 @@ static int z_erofs_lz4_decompress_mem(struct z_erofs_lz4_decompress_ctx *ctx,
> vm_unmap_ram(src, ctx->inpages);
> } else if (maptype == 2) {
> erofs_put_pcpubuf(src);
> - } else {
> + } else if (maptype != 3) {
> DBG_BUGON(1);
> return -EFAULT;
> }
> @@ -308,7 +307,7 @@ static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq,
> }
>
> dstmap_out:
> - ret = z_erofs_lz4_decompress_mem(&ctx, dst + rq->pageofs_out);
> + ret = z_erofs_lz4_decompress_mem(&ctx, dst);
> if (!dst_maptype)
> kunmap_local(dst);
> else if (dst_maptype == 2)
> --
> 2.39.3
>
More information about the Linux-erofs
mailing list