[PATCH] erofs: fix lz4 inplace decompression

[Gao Xiang]

On 2023/12/6 12:39, Gao Xiang wrote:
> 
> 
> On 2023/12/6 12:28, Juhyung Park wrote:
>> 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. :))
> 
> I will handle these LTSes manually, so don't worry about that :)
> 
>>
>> 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?
> 
> Yeah, I could keep this line when applying.  Thanks for pointing out.
> 
>>
>>>          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.
> 
> Thanks, but the kernel coding style doesn't allow that.

Oh, I could move this initialization below, I will send out
a quick v2 for this.

Thanks,
Gao Xiang