> compressed size > source buffer size has nothing to do with it. the
> hw compressor must not exceed the *destination* buffer size, no matter
> how small or large it is.
> if your hw compressor can't guarantee that it won't overrun the dest
> buffer, you need to use a bounce buffer that's sized large enough to
> guarantee your hw won't corrupt memory.
> again..."greater than source data size" has nothing to do with this.
> if lzo or lz4 fail to check the output buffer size before writing into
> it, that's a serious bug in their code that needs to be fixed.
I see a deviation between the consumer of compression driver and owner of compression driver here.
See how zram is dealing with this by always passing PAGE_SIZE * 2 for destination length when it calls compression driver.
On 9/26/18, 2:01 AM, "Dan Streetman" <ddstreet@xxxxxxxx> wrote:
On Thu, Sep 20, 2018 at 9:01 PM Um, Taeil <taeilum@xxxxxxxxxx> wrote:
>
> > do you have a specific example of how this causes any actual problem?
> Yes, I have a H/W accelerator that tries to finish compression even if compressed data size is greater than source data size.
compressed size > source buffer size has nothing to do with it. the
hw compressor must not exceed the *destination* buffer size, no matter
how small or large it is.
if your hw compressor can't guarantee that it won't overrun the dest
buffer, you need to use a bounce buffer that's sized large enough to
guarantee your hw won't corrupt memory.
see the ppc PowerNV hw compressor driver for reference.
>
> > personally, i'd prefer reducing zswap_dstmem down to 1 page to save
> > memory, since there is no case where zswap would ever want to use a
> > compressed page larger than that.
> I don't think "reducing zswap_dstmem down to 1 page" works in today's lzo and lz4 kernel implementation.
> If I read kernel's lzo, lz4 code correctly, it does not stop compression when compressed data size is greater than source data size.
again..."greater than source data size" has nothing to do with this.
if lzo or lz4 fail to check the output buffer size before writing into
it, that's a serious bug in their code that needs to be fixed.
>
> On 9/19/18, 8:47 AM, "Dan Streetman" <ddstreet@xxxxxxxx> wrote:
>
> On Tue, Sep 18, 2018 at 7:48 PM Um, Taeil <taeilum@xxxxxxxxxx> wrote:
> >
> > We can tell whether compressed size is greater than PAGE_SIZE by looking at the returned *dlen value from crypto_comp_compress. This should be fairly easy.
> > This is actually what zram is doing today. zram looks for *dlen and not take the compressed result if *dlen is greater than certain size.
> > I think errors from crypto_comp_compress should be real errors.
> >
> > Today in kernel compression drivers such as lzo and lz4, they do not stop just because compression result size is greater than source size.
> > Also, some H/W accelerators would not have the option of stopping compression just because of the result size is greater than source size.
>
> do you have a specific example of how this causes any actual problem?
>
> personally, i'd prefer reducing zswap_dstmem down to 1 page to save
> memory, since there is no case where zswap would ever want to use a
> compressed page larger than that.
>
> >
> > Thank you,
> > Taeil
> >
> > On 9/18/18, 2:44 PM, "Dan Streetman" <ddstreet@xxxxxxxx> wrote:
> >
> > On Tue, Sep 18, 2018 at 2:52 PM Um, Taeil <taeilum@xxxxxxxxxx> wrote:
> > >
> > > Problem statement:
> > > "compressed data are not fully copied to destination buffer when compressed data size is greater than source data"
> > >
> > > Why:
> > > 5th argument of crypto_comp_compress function is *dlen, which tell the compression driver how many bytes the destination buffer space is allocated (allowed to write data).
> > > This *dlen is important especially for H/W accelerator based compression driver because it is dangerous if we allow the H/W accelerator to access memory beyond *dst + *dlen.
> > > Note that buffer location would be passed as physical address.
> > > Due to the above reason, H/W accelerator based compression driver need to honor *dlen value when it serves crypto_comp_compress API.
> >
> > and that's exactly what zswap wants to happen - any compressor (hw or
> > sw) should fail with an error code (ENOSPC makes the most sense, but
> > zswap doesn't actually care) if the compressed data size is larger
> > than the provided data buffer.
> >
> > > Today, we pass slen = PAGE_SIZE and *dlen=PAGE_SIZE to crypto_comp_compress in zswap.c.
> > > If compressed data size is greater than source (uncompressed) data size, H/W accelerator cannot copy (deliver) the entire compressed data.
> >
> > If the "compressed" data is larger than 1 page, then there is no point
> > in storing the page in zswap.
> >
> > remember that zswap is different than zram; in zram, there's no other
> > place to store the data. However, with zswap, if compression fails or
> > isn't good, we can just pass the uncompressed page down to the swap
> > device.
> >
> > >
> > > Thank you,
> > > Taeil
> > >
> > > On 9/18/18, 7:15 AM, "Dan Streetman" <ddstreet@xxxxxxxx> wrote:
> > >
> > > On Mon, Sep 17, 2018 at 7:10 PM Um, Taeil <taeilum@xxxxxxxxxx> wrote:
> > > >
> > > > Currently, we allocate PAGE_SIZE * 2 for zswap_dstmem which is used as compression destination buffer.
> > > >
> > > > However, we pass only half of the size (PAGE_SIZE) to crypto_comp_compress.
> > > >
> > > > This might not be a problem for CPU based existing lzo, lz4 crypto compression driver implantation.
> > > >
> > > > However, this could be a problem for some H/W acceleration compression drivers, which honor destination buffer size when it prepares H/W resources.
> > >
> > > How exactly could it be a problem?
> > >
> > > >
> > > > Actually, this patch is aligned with what zram is passing when it calls crypto_comp_compress.
> > > >
> > > > The following simple patch will solve this problem. I tested it with existing crypto/lzo.c and crypto/lz4.c compression driver and it works fine.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > --- mm/zswap.c.orig 2018-09-14 14:36:37.984199232 -0700
> > > >
> > > > +++ mm/zswap.c 2018-09-14 14:36:53.340189681 -0700
> > > >
> > > > @@ -1001,7 +1001,7 @@ static int zswap_frontswap_store(unsigne
> > > >
> > > > struct zswap_entry *entry, *dupentry;
> > > >
> > > > struct crypto_comp *tfm;
> > > >
> > > > int ret;
> > > >
> > > > - unsigned int hlen, dlen = PAGE_SIZE;
> > > >
> > > > + unsigned int hlen, dlen = PAGE_SIZE * 2;
> > > >
> > > > unsigned long handle, value;
> > > >
> > > > char *buf;
> > > >
> > > > u8 *src, *dst;
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Thank you,
> > > >
> > > > Taeil
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
