Re: [RFC PATCH 05/21] ubifs: Pass worst-case buffer size to compression routines

[Zhihao Cheng]

在 2023/7/19 22:38, Ard Biesheuvel 写道:
> On Wed, 19 Jul 2023 at 16:23, Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote:
> > 在 2023/7/19 16:33, Ard Biesheuvel 写道:
> > > On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> > > > On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote:
> > > > > Currently, the ubifs code allocates a worst case buffer size to
> > > > > recompress a data node, but does not pass the size of that buffer to the
> > > > > compression code. This means that the compression code will never use
> >
> > I think you mean the 'out_len' which describes the lengh of 'buf' is
> > passed into ubifs_decompress, which effects the result of
> > decompressor(eg. lz4 uses length to calculate the buffer end pos).
> > So, we should pass the real lenghth of 'buf'.
>
> Yes, that is what I meant.
>
> But Eric makes a good point, and looking a bit more closely, there is
> really no need for the multiplication here: we know the size of the
> decompressed data, so we don't need the additional space.

Right, we get 'out_len' from 'dn->size' which is the length of 
uncompressed data. ubifs_compress makes sure the compressed length is 
smaller than original length.

> I intend to drop this patch, and replace it with the following:
>
> ----------------8<--------------
>
> Currently, when truncating a data node, a decompression buffer is
> allocated that is twice the size of the data node's uncompressed size.
> However, the fact that this space is available is not communicated to
> the compression routines, as out_len itself is not updated.
>
> The additional space is not needed even in the theoretical worst case
> where compression might lead to inadvertent expansion: first of all,
> increasing the size of the input buffer does not help mitigate that
> issue. And given the truncation of the data node and the fact that the
> original data compressed well enough to pass the UBIFS_MIN_COMPRESS_DIFF
> test, there is no way on this particular code path that compression
> could result in expansion beyond the original decompressed size, and so
> no mitigation is necessary to begin with.
>
> So let's just drop WORST_COMPR_FACTOR here.
>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index dc52ac0f4a345f30..0b55cbfe0c30505e 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -1489,7 +1489,7 @@ static int truncate_data_node(const struct
> ubifs_info *c, const struct inode *in
>          int err, dlen, compr_type, out_len, data_size;
>
>          out_len = le32_to_cpu(dn->size);
> -       buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
> +       buf = kmalloc(out_len, GFP_NOFS);
>          if (!buf)
>                  return -ENOMEM;
> .

This version looks better.

Reviewed-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>