[0/3] crypto: Add SG support to deflate

Message

Herbert Xu March 19, 2025, 6:04 a.m. UTC

This patch-series adds SG support to deflate so that IPsec can
avoid linearising the data.

Herbert Xu (3):
  crypto: acomp - Move scomp stream allocation code into acomp
  crypto: acomp - Add acomp_walk
  crypto: deflate - Convert to acomp

 crypto/acompress.c                  | 228 ++++++++++++++++
 crypto/deflate.c                    | 405 ++++++++++++++--------------
 crypto/scompress.c                  | 133 +--------
 include/crypto/internal/acompress.h |  77 ++++++
 include/crypto/internal/scompress.h |  28 +-
 5 files changed, 534 insertions(+), 337 deletions(-)

Comments

Ard Biesheuvel March 20, 2025, 7:51 a.m. UTC | #1

On Wed, 19 Mar 2025 at 07:05, Herbert Xu <[e-mail]> wrote:
>
> This patch-series adds SG support to deflate so that IPsec can
> avoid linearising the data.
>
> Herbert Xu (3):
>   crypto: acomp - Move scomp stream allocation code into acomp
>   crypto: acomp - Add acomp_walk
>   crypto: deflate - Convert to acomp
>

IIRC Eric had some feedback at the time regarding the exact behavior
of the zlib API, and I notice that the code no longer deals with
Z_SYNC_FLUSH at all, which I did handle in my version of patch #3.

Do your tests have coverage for all the conditional cases there?

Herbert Xu March 20, 2025, 8:14 a.m. UTC | #2

On Thu, Mar 20, 2025 at 08:51:40AM +0100, Ard Biesheuvel wrote:
>
> IIRC Eric had some feedback at the time regarding the exact behavior
> of the zlib API, and I notice that the code no longer deals with
> Z_SYNC_FLUSH at all, which I did handle in my version of patch #3.

I didn't see any feedback regarding this when looking at your patch:

https://lore.kernel.org/linux-crypto/[e-mail]/

Do you have a link to that discussion?

I was going to add the original USAGI workaround but then I
thought perhaps it is no longer necessary as our zlib has
been updated since the workaround was added back in 2003.

My understanding is that the workaround is not about Z_SYNC_FLUSH
but feeding an extra byte to the decompressor.  The only difference
between Z_SYNC_FLUSH and Z_FLUSH on inflate is that one would return
Z_OK while the other returns Z_BUF_ERROR,  both are treated as an
error by crypto/deflate.c.

> Do your tests have coverage for all the conditional cases there?

If you mean the scatterlists then yes I have coverage for that.

If you mean the USAGI workaround then no because I don't know what
triggered the original problem.

I do note however that zcomp which also contains deflate does not
have this workaround either.  If it was really necessary then zram
would have run into it and screamed loudly about not being able to
decompress a page.  Or perhaps nobody ever uses zram with deflate.

Cheers,

Ard Biesheuvel March 20, 2025, 8:55 a.m. UTC | #3

On Thu, 20 Mar 2025 at 09:14, Herbert Xu <[e-mail]> wrote:
>
> On Thu, Mar 20, 2025 at 08:51:40AM +0100, Ard Biesheuvel wrote:
> >
> > IIRC Eric had some feedback at the time regarding the exact behavior
> > of the zlib API, and I notice that the code no longer deals with
> > Z_SYNC_FLUSH at all, which I did handle in my version of patch #3.
>
> I didn't see any feedback regarding this when looking at your patch:
>
> https://lore.kernel.org/linux-crypto/[e-mail]/
>
> Do you have a link to that discussion?
>

No. I did some digging but I could find anything. Eric might remember.

> I was going to add the original USAGI workaround but then I
> thought perhaps it is no longer necessary as our zlib has
> been updated since the workaround was added back in 2003.
>
> My understanding is that the workaround is not about Z_SYNC_FLUSH
> but feeding an extra byte to the decompressor.  The only difference
> between Z_SYNC_FLUSH and Z_FLUSH on inflate is that one would return
> Z_OK while the other returns Z_BUF_ERROR,  both are treated as an
> error by crypto/deflate.c.
>

I'm fine with this, I just wanted to raise it because it jogged my
memory but I can't quite remember the details. So if things are
working as expected, it's all fine with me.

> > Do your tests have coverage for all the conditional cases there?
>
> If you mean the scatterlists then yes I have coverage for that.
>
> If you mean the USAGI workaround then no because I don't know what
> triggered the original problem.
>
> I do note however that zcomp which also contains deflate does not
> have this workaround either.  If it was really necessary then zram
> would have run into it and screamed loudly about not being able to
> decompress a page.  Or perhaps nobody ever uses zram with deflate.
>

Yeah I meant in general, not the workaround for the mythical USAGI issue :-)

Eric Biggers March 20, 2025, 5:34 p.m. UTC | #4

On Thu, Mar 20, 2025 at 09:55:42AM +0100, Ard Biesheuvel wrote:
> On Thu, 20 Mar 2025 at 09:14, Herbert Xu <[e-mail]> wrote:
> >
> > On Thu, Mar 20, 2025 at 08:51:40AM +0100, Ard Biesheuvel wrote:
> > >
> > > IIRC Eric had some feedback at the time regarding the exact behavior
> > > of the zlib API, and I notice that the code no longer deals with
> > > Z_SYNC_FLUSH at all, which I did handle in my version of patch #3.
> >
> > I didn't see any feedback regarding this when looking at your patch:
> >
> > https://lore.kernel.org/linux-crypto/[e-mail]/
> >
> > Do you have a link to that discussion?
> >
> 
> No. I did some digging but I could find anything. Eric might remember.

I'm not sure what this is referring to.

Then again this patchset doesn't apply, so it's unreviewable anyway.

Just a note, for compression and decompression it's often more efficient to
linearize in the caller.  Otherwise the algorithm ends up having to copy the
uncompressed data to an internal buffer anyway.  That's needed for the match
finding (compression) and match copying (decompression) to work.

As I mentioned before, the "stream" terminology that Herbert is choosing for
some reason also seems less than ideal.  "workspace" would be better.  Many of
the compression algorithms don't even support streaming.

> > I was going to add the original USAGI workaround but then I
> > thought perhaps it is no longer necessary as our zlib has
> > been updated since the workaround was added back in 2003.

The kernel's zlib was forked from upstream zlib in the 90s and hasn't been
re-synced since then.

- Eric

Herbert Xu March 21, 2025, 2:36 a.m. UTC | #5

On Thu, Mar 20, 2025 at 05:34:50PM +0000, Eric Biggers wrote:
>
> Then again this patchset doesn't apply, so it's unreviewable anyway.

You can test it at

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git acomp2
 
> The kernel's zlib was forked from upstream zlib in the 90s and hasn't been
> re-synced since then.

It was updated in 2006, three years after the USAGI workaround:

commit 4f3865fb57a04db7cca068fed1c15badc064a302
Author: Richard Purdie <[e-mail]>
Date:   Thu Jun 22 14:47:34 2006 -0700

    [PATCH] zlib_inflate: Upgrade library code to a recent version

Cheers,