[PATCH] pstore: migrate to crypto acomp interface (take 2)

[PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

From: Ard Biesheuvel <ardb@kernel.org>

The crypto 'compress' interface is deprecated, so before adding new
features, migrate to the acomp interface. Note that we are only using
synchronous implementations of acomp, so we don't have to deal with
asynchronous completion.

[ Tweaked error paths to avoid memory leak, as pointed out byGuilherme
  G. Piccoli <gpiccoli@igalia.com>, and fixed pstore_compress()
  return value -kees ]

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Link: https://lore.kernel.org/lkml/CAMj1kXFnoqj+cn-0dT8fg0kgLvVx+Q2Ex-4CUjSnA9yRprmC-w@mail.gmail.com/
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 74 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 17 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 0c034ea39954..f82256612c19 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -28,11 +28,14 @@
 #include <linux/crypto.h>
 #include <linux/string.h>
 #include <linux/timer.h>
+#include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
 
+#include <crypto/acompress.h>
+
 #include "internal.h"
 
 /*
@@ -90,7 +93,8 @@ module_param(compress, charp, 0444);
 MODULE_PARM_DESC(compress, "compression to use");
 
 /* Compression parameters */
-static struct crypto_comp *tfm;
+static struct crypto_acomp *tfm;
+static struct acomp_req *creq;
 
 struct pstore_zbackend {
 	int (*zbufsize)(size_t size);
@@ -268,23 +272,32 @@ static const struct pstore_zbackend zbackends[] = {
 static int pstore_compress(const void *in, void *out,
 			   unsigned int inlen, unsigned int outlen)
 {
+	struct scatterlist src, dst;
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS))
 		return -EINVAL;
 
-	ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
+	sg_init_table(&src, 1);
+	sg_set_buf(&src, in, inlen);
+
+	sg_init_table(&dst, 1);
+	sg_set_buf(&dst, out, outlen);
+
+	acomp_request_set_params(creq, &src, &dst, inlen, outlen);
+
+	ret = crypto_acomp_compress(creq);
 	if (ret) {
 		pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
 		return ret;
 	}
 
-	return outlen;
+	return creq->dlen;
 }
 
 static void allocate_buf_for_compression(void)
 {
-	struct crypto_comp *ctx;
+	struct crypto_acomp *acomp;
 	int size;
 	char *buf;
 
@@ -296,7 +309,7 @@ static void allocate_buf_for_compression(void)
 	if (!psinfo || tfm)
 		return;
 
-	if (!crypto_has_comp(zbackend->name, 0, 0)) {
+	if (!crypto_has_acomp(zbackend->name, 0, CRYPTO_ALG_ASYNC)) {
 		pr_err("Unknown compression: %s\n", zbackend->name);
 		return;
 	}
@@ -315,16 +328,24 @@ static void allocate_buf_for_compression(void)
 		return;
 	}
 
-	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
-	if (IS_ERR_OR_NULL(ctx)) {
+	acomp = crypto_alloc_acomp(zbackend->name, 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR_OR_NULL(acomp)) {
 		kfree(buf);
 		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
-		       PTR_ERR(ctx));
+		       PTR_ERR(acomp));
+		return;
+	}
+
+	creq = acomp_request_alloc(acomp);
+	if (!creq) {
+		crypto_free_acomp(acomp);
+		kfree(buf);
+		pr_err("acomp_request_alloc('%s') failed\n", zbackend->name);
 		return;
 	}
 
 	/* A non-NULL big_oops_buf indicates compression is available. */
-	tfm = ctx;
+	tfm = acomp;
 	big_oops_buf_sz = size;
 	big_oops_buf = buf;
 
@@ -334,7 +355,8 @@ static void allocate_buf_for_compression(void)
 static void free_buf_for_compression(void)
 {
 	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
-		crypto_free_comp(tfm);
+		acomp_request_free(creq);
+		crypto_free_acomp(tfm);
 		tfm = NULL;
 	}
 	kfree(big_oops_buf);
@@ -671,6 +693,8 @@ static void decompress_record(struct pstore_record *record)
 	int ret;
 	int unzipped_len;
 	char *unzipped, *workspace;
+	struct acomp_req *dreq;
+	struct scatterlist src, dst;
 
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !record->compressed)
 		return;
@@ -694,31 +718,47 @@ static void decompress_record(struct pstore_record *record)
 	if (!workspace)
 		return;
 
+	dreq = acomp_request_alloc(tfm);
+	if (!dreq)
+		goto out_free_workspace;
+
+	sg_init_table(&src, 1);
+	sg_set_buf(&src, record->buf, record->size);
+
+	sg_init_table(&dst, 1);
+	sg_set_buf(&dst, workspace, unzipped_len);
+
+	acomp_request_set_params(dreq, &src, &dst, record->size, unzipped_len);
+
 	/* After decompression "unzipped_len" is almost certainly smaller. */
-	ret = crypto_comp_decompress(tfm, record->buf, record->size,
-					  workspace, &unzipped_len);
+	ret = crypto_acomp_decompress(dreq);
 	if (ret) {
-		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
-		kfree(workspace);
-		return;
+		pr_err("crypto_acomp_decompress failed, ret = %d!\n", ret);
+		goto out;
 	}
 
 	/* Append ECC notice to decompressed buffer. */
+	unzipped_len = dreq->dlen;
 	memcpy(workspace + unzipped_len, record->buf + record->size,
 	       record->ecc_notice_size);
 
 	/* Copy decompressed contents into an minimum-sized allocation. */
 	unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
 			   GFP_KERNEL);
-	kfree(workspace);
 	if (!unzipped)
-		return;
+		goto out;
 
 	/* Swap out compressed contents with decompressed contents. */
 	kfree(record->buf);
 	record->buf = unzipped;
 	record->size = unzipped_len;
 	record->compressed = false;
+
+out:
+	acomp_request_free(dreq);
+out_free_workspace:
+	kfree(workspace);
+	return;
 }
 
 /*
-- 
2.34.1

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Thu, Oct 06, 2022 at 04:41:38PM -0700, Kees Cook wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The crypto 'compress' interface is deprecated, so before adding new
> features, migrate to the acomp interface. Note that we are only using
> synchronous implementations of acomp, so we don't have to deal with
> asynchronous completion.
> 
> [ Tweaked error paths to avoid memory leak, as pointed out byGuilherme
                                                              ^^
gah, I'll fix this typo locally. :P

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Guilherme G. Piccoli]

Thanks for re-sending this one, I'll test it next week =)
Cheers,


Guilherme

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Guilherme G. Piccoli]

On 06/10/2022 20:41, Kees Cook wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The crypto 'compress' interface is deprecated, so before adding new
> features, migrate to the acomp interface. Note that we are only using
> synchronous implementations of acomp, so we don't have to deal with
> asynchronous completion.
> 
> [ Tweaked error paths to avoid memory leak, as pointed out byGuilherme
>   G. Piccoli <gpiccoli@igalia.com>, and fixed pstore_compress()
>   return value -kees ]
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Link: https://lore.kernel.org/lkml/CAMj1kXFnoqj+cn-0dT8fg0kgLvVx+Q2Ex-4CUjSnA9yRprmC-w@mail.gmail.com/
> Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/platform.c | 74 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 17 deletions(-)
> 

Hi Kees, thanks for re-sending this one.

Just tested it on top of v6.0, with efi-pstore/ramoops, using zstd, lz4
and deflate - everything working as expected.
So, with the typo fixed, have my:

Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Cheers,


Guilherme

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Mon, Oct 17, 2022 at 01:26:12PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:41, Kees Cook wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 
> > The crypto 'compress' interface is deprecated, so before adding new
> > features, migrate to the acomp interface. Note that we are only using
> > synchronous implementations of acomp, so we don't have to deal with
> > asynchronous completion.

Ard, given your observation about all the per-cpu memory allocation,
should pstore still go ahead with this conversion?

-Kees

> > 
> > [ Tweaked error paths to avoid memory leak, as pointed out byGuilherme
> >   G. Piccoli <gpiccoli@igalia.com>, and fixed pstore_compress()
> >   return value -kees ]
> > 
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > Link: https://lore.kernel.org/lkml/CAMj1kXFnoqj+cn-0dT8fg0kgLvVx+Q2Ex-4CUjSnA9yRprmC-w@mail.gmail.com/
> > Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  fs/pstore/platform.c | 74 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 57 insertions(+), 17 deletions(-)
> > 
> 
> Hi Kees, thanks for re-sending this one.
> 
> Just tested it on top of v6.0, with efi-pstore/ramoops, using zstd, lz4
> and deflate - everything working as expected.
> So, with the typo fixed, have my:
> 
> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Thanks!

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 20:01, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 17, 2022 at 01:26:12PM -0300, Guilherme G. Piccoli wrote:
> > On 06/10/2022 20:41, Kees Cook wrote:
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > The crypto 'compress' interface is deprecated, so before adding new
> > > features, migrate to the acomp interface. Note that we are only using
> > > synchronous implementations of acomp, so we don't have to deal with
> > > asynchronous completion.
>
> Ard, given your observation about all the per-cpu memory allocation,
> should pstore still go ahead with this conversion?
>

Well, the reason for doing this conversion was so that we could move
the 'worst case buffer size' logic into the individual drivers, as
Herbert would't allow that for legacy comp.

But as we found, we don't really care about the theoretical worst case
of an input that is incompressible - we can just pass the uncompressed
size as the upper bound, and if the crypto fails, we just store the
data uncompressed (which never happens in the first place with ASCII
text)

So once we use the same size for input and output, I was curious
whether we could encrypt in place, and get rid of the big_oops_buf.
And the answer is 'yes', precisely because we have this horrid per-CPU
allocation which serves as a bounce buffer. And this is not specific
to acomp, the old comp algorithms get wrapped in scomps which receive
the same treatment.

So at that point, I wondered what the point is of all this complexity.
Do we really need 6 different algorithms to compress a couple of K of
ASCII text on a code path that is ice cold by definition? Wouldn't it
be better to drop the crypto API altogether here, and just use GZIP
via the library interface?

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Guilherme G. Piccoli]

On 17/10/2022 15:14, Ard Biesheuvel wrote:
> [...]
> 
> So at that point, I wondered what the point is of all this complexity.
> Do we really need 6 different algorithms to compress a couple of K of
> ASCII text on a code path that is ice cold by definition? Wouldn't it
> be better to drop the crypto API altogether here, and just use GZIP
> via the library interface?

Skipping all the interesting and more complex parts, I'd just want to
consider zstd maybe? Quite fast and efficient - it's what we're using by
default on Steam Deck.

I'm not sure what is the gzip library interface - you mean skipping the
scomp/legacy comp interface, and make use directly of gzip?

Cheers,


Guilherme

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 20:23, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 17/10/2022 15:14, Ard Biesheuvel wrote:
> > [...]
> >
> > So at that point, I wondered what the point is of all this complexity.
> > Do we really need 6 different algorithms to compress a couple of K of
> > ASCII text on a code path that is ice cold by definition? Wouldn't it
> > be better to drop the crypto API altogether here, and just use GZIP
> > via the library interface?
>
> Skipping all the interesting and more complex parts, I'd just want to
> consider zstd maybe?

I just made the point that it doesn't matter. So on the one hand, I
don't have any objections to ZSTD per se. But I do wonder if it is the
best choice when it comes to code size etc. Perhaps one of the
compression algorithms is guaranteed to be compiled in anyway?

> Quite fast and efficient - it's what we're using by
> default on Steam Deck.
>
> I'm not sure what is the gzip library interface - you mean skipping the
> scomp/legacy comp interface, and make use directly of gzip?
>

zlib_deflate() and friends.

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Mon, Oct 17, 2022 at 08:14:14PM +0200, Ard Biesheuvel wrote:
> So once we use the same size for input and output, I was curious
> whether we could encrypt in place, and get rid of the big_oops_buf.
> And the answer is 'yes', precisely because we have this horrid per-CPU
> allocation which serves as a bounce buffer. And this is not specific
> to acomp, the old comp algorithms get wrapped in scomps which receive
> the same treatment.

Ah, in the sense that "in place" is actually happening in the per-cpu
allocation, and only if it succeeds does the input buffer get
overwritten?

> So at that point, I wondered what the point is of all this complexity.
> Do we really need 6 different algorithms to compress a couple of K of
> ASCII text on a code path that is ice cold by definition? Wouldn't it
> be better to drop the crypto API altogether here, and just use GZIP
> via the library interface?

Well, my goal was to make the algo "pstore doesn't care". If someone
picks deflate, do they still get all the per-cpu allocations?

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 21:29, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 17, 2022 at 08:14:14PM +0200, Ard Biesheuvel wrote:
> > So once we use the same size for input and output, I was curious
> > whether we could encrypt in place, and get rid of the big_oops_buf.
> > And the answer is 'yes', precisely because we have this horrid per-CPU
> > allocation which serves as a bounce buffer. And this is not specific
> > to acomp, the old comp algorithms get wrapped in scomps which receive
> > the same treatment.
>
> Ah, in the sense that "in place" is actually happening in the per-cpu
> allocation, and only if it succeeds does the input buffer get
> overwritten?
>

Something like that IIRC.

> > So at that point, I wondered what the point is of all this complexity.
> > Do we really need 6 different algorithms to compress a couple of K of
> > ASCII text on a code path that is ice cold by definition? Wouldn't it
> > be better to drop the crypto API altogether here, and just use GZIP
> > via the library interface?
>
> Well, my goal was to make the algo "pstore doesn't care". If someone
> picks deflate, do they still get all the per-cpu allocations?
>

Not if you use the library interface directly.

The issue with the percpu buffers is that they are only kept if any
scomp TFMs are active, but this is always the case for pstore, as you
don't want to allocate it on the panic path.

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Mon, Oct 17, 2022 at 09:33:06PM +0200, Ard Biesheuvel wrote:
> On Mon, 17 Oct 2022 at 21:29, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Oct 17, 2022 at 08:14:14PM +0200, Ard Biesheuvel wrote:
> > > So once we use the same size for input and output, I was curious
> > > whether we could encrypt in place, and get rid of the big_oops_buf.
> > > And the answer is 'yes', precisely because we have this horrid per-CPU
> > > allocation which serves as a bounce buffer. And this is not specific
> > > to acomp, the old comp algorithms get wrapped in scomps which receive
> > > the same treatment.
> >
> > Ah, in the sense that "in place" is actually happening in the per-cpu
> > allocation, and only if it succeeds does the input buffer get
> > overwritten?
> 
> Something like that IIRC.
> 
> > > So at that point, I wondered what the point is of all this complexity.
> > > Do we really need 6 different algorithms to compress a couple of K of
> > > ASCII text on a code path that is ice cold by definition? Wouldn't it
> > > be better to drop the crypto API altogether here, and just use GZIP
> > > via the library interface?
> >
> > Well, my goal was to make the algo "pstore doesn't care". If someone
> > picks deflate, do they still get all the per-cpu allocations?
> >
> 
> Not if you use the library interface directly.
> 
> The issue with the percpu buffers is that they are only kept if any
> scomp TFMs are active, but this is always the case for pstore, as you
> don't want to allocate it on the panic path.

Okay, so strictly speaking, eliminating the per-CPU allocation is an
improvement. Keeping scomp and doing in-place compression will let
pstore use "any" compressions method.

Is there a crypto API that does _not_ preallocate the per-CPU stuff?
Because, as you say, it's a huge amount of memory on the bigger
systems...

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 21:40, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 17, 2022 at 09:33:06PM +0200, Ard Biesheuvel wrote:
> > On Mon, 17 Oct 2022 at 21:29, Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Mon, Oct 17, 2022 at 08:14:14PM +0200, Ard Biesheuvel wrote:
> > > > So once we use the same size for input and output, I was curious
> > > > whether we could encrypt in place, and get rid of the big_oops_buf.
> > > > And the answer is 'yes', precisely because we have this horrid per-CPU
> > > > allocation which serves as a bounce buffer. And this is not specific
> > > > to acomp, the old comp algorithms get wrapped in scomps which receive
> > > > the same treatment.
> > >
> > > Ah, in the sense that "in place" is actually happening in the per-cpu
> > > allocation, and only if it succeeds does the input buffer get
> > > overwritten?
> >
> > Something like that IIRC.
> >
> > > > So at that point, I wondered what the point is of all this complexity.
> > > > Do we really need 6 different algorithms to compress a couple of K of
> > > > ASCII text on a code path that is ice cold by definition? Wouldn't it
> > > > be better to drop the crypto API altogether here, and just use GZIP
> > > > via the library interface?
> > >
> > > Well, my goal was to make the algo "pstore doesn't care". If someone
> > > picks deflate, do they still get all the per-cpu allocations?
> > >
> >
> > Not if you use the library interface directly.
> >
> > The issue with the percpu buffers is that they are only kept if any
> > scomp TFMs are active, but this is always the case for pstore, as you
> > don't want to allocate it on the panic path.
>
> Okay, so strictly speaking, eliminating the per-CPU allocation is an
> improvement. Keeping scomp and doing in-place compression will let
> pstore use "any" compressions method.
>

I'm not following the point you are making here.

> Is there a crypto API that does _not_ preallocate the per-CPU stuff?
> Because, as you say, it's a huge amount of memory on the bigger
> systems...
>

The library interface for each of the respective algorithms.

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Mon, Oct 17, 2022 at 09:45:08PM +0200, Ard Biesheuvel wrote:
> On Mon, 17 Oct 2022 at 21:40, Kees Cook <keescook@chromium.org> wrote:
> > Okay, so strictly speaking, eliminating the per-CPU allocation is an
> > improvement. Keeping scomp and doing in-place compression will let
> > pstore use "any" compressions method.
> 
> I'm not following the point you are making here.

Sorry, I mean to say that if I leave scomp in pstore, nothing is "worse"
(i.e. the per-cpu allocation is present in both scomp and acomp). i.e.
no regression either way, but if we switch to a distinct library call,
it's an improvement on the memory utilization front.

> > Is there a crypto API that does _not_ preallocate the per-CPU stuff?
> > Because, as you say, it's a huge amount of memory on the bigger
> > systems...
> 
> The library interface for each of the respective algorithms.

Where is the crypto API for just using the library interfaces, so I
don't have to be tied to a specific algo?

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 22:11, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 17, 2022 at 09:45:08PM +0200, Ard Biesheuvel wrote:
> > On Mon, 17 Oct 2022 at 21:40, Kees Cook <keescook@chromium.org> wrote:
> > > Okay, so strictly speaking, eliminating the per-CPU allocation is an
> > > improvement. Keeping scomp and doing in-place compression will let
> > > pstore use "any" compressions method.
> >
> > I'm not following the point you are making here.
>
> Sorry, I mean to say that if I leave scomp in pstore, nothing is "worse"
> (i.e. the per-cpu allocation is present in both scomp and acomp). i.e.
> no regression either way, but if we switch to a distinct library call,
> it's an improvement on the memory utilization front.
>
> > > Is there a crypto API that does _not_ preallocate the per-CPU stuff?
> > > Because, as you say, it's a huge amount of memory on the bigger
> > > systems...
> >
> > The library interface for each of the respective algorithms.
>
> Where is the crypto API for just using the library interfaces, so I
> don't have to be tied to a specific algo?
>

That doesn't exist, that is the point.

But how does the algo matter when you are dealing with mere kilobytes
of ASCII text?

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Kees Cook]

On Mon, Oct 17, 2022 at 10:13:52PM +0200, Ard Biesheuvel wrote:
> On Mon, 17 Oct 2022 at 22:11, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Oct 17, 2022 at 09:45:08PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 17 Oct 2022 at 21:40, Kees Cook <keescook@chromium.org> wrote:
> > > > Okay, so strictly speaking, eliminating the per-CPU allocation is an
> > > > improvement. Keeping scomp and doing in-place compression will let
> > > > pstore use "any" compressions method.
> > >
> > > I'm not following the point you are making here.
> >
> > Sorry, I mean to say that if I leave scomp in pstore, nothing is "worse"
> > (i.e. the per-cpu allocation is present in both scomp and acomp). i.e.
> > no regression either way, but if we switch to a distinct library call,
> > it's an improvement on the memory utilization front.
> >
> > > > Is there a crypto API that does _not_ preallocate the per-CPU stuff?
> > > > Because, as you say, it's a huge amount of memory on the bigger
> > > > systems...
> > >
> > > The library interface for each of the respective algorithms.
> >
> > Where is the crypto API for just using the library interfaces, so I
> > don't have to be tied to a specific algo?
> >
> 
> That doesn't exist, that is the point.

Shouldn't something like that exist, though?

> But how does the algo matter when you are dealing with mere kilobytes
> of ASCII text?

Sure, though, this is how we got here -- every couple of years, someone
added another library interface to another compression aglo. I tore all
that out so we could avoid having to choose a single one, but was left
with the zbufsize mess (that, yes, doesn't matter). So now pstore can
just not care what compression is chosen.

-- 
Kees Cook

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 22:36, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 17, 2022 at 10:13:52PM +0200, Ard Biesheuvel wrote:
> > On Mon, 17 Oct 2022 at 22:11, Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Mon, Oct 17, 2022 at 09:45:08PM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 17 Oct 2022 at 21:40, Kees Cook <keescook@chromium.org> wrote:
> > > > > Okay, so strictly speaking, eliminating the per-CPU allocation is an
> > > > > improvement. Keeping scomp and doing in-place compression will let
> > > > > pstore use "any" compressions method.
> > > >
> > > > I'm not following the point you are making here.
> > >
> > > Sorry, I mean to say that if I leave scomp in pstore, nothing is "worse"
> > > (i.e. the per-cpu allocation is present in both scomp and acomp). i.e.
> > > no regression either way, but if we switch to a distinct library call,
> > > it's an improvement on the memory utilization front.
> > >
> > > > > Is there a crypto API that does _not_ preallocate the per-CPU stuff?
> > > > > Because, as you say, it's a huge amount of memory on the bigger
> > > > > systems...
> > > >
> > > > The library interface for each of the respective algorithms.
> > >
> > > Where is the crypto API for just using the library interfaces, so I
> > > don't have to be tied to a specific algo?
> > >
> >
> > That doesn't exist, that is the point.
>
> Shouldn't something like that exist, though?
>

Well, if what you have in mind is a pluggable API where abstract
compress() invocations can be backed by different implementations,
you'll soon find that you don't want to compile every alternative into
the kernel statically, and you'll need some kind of module dispatch.
That brings you very close to the crypto API already.

However, the main mismatch between the crypto API and a library
interface is the use of scatterlists, and this is the reason we have
those per-cpu buffers in the first place, as the underlying algos
don't operate on scatterlists, and so you need a scratch buffer to
hold the data. Another complication is that you cannot test for
in-place operation as easily by comparing src and dst pointers, given
that distinct scatterlists for src and may still describe the same
buffer in memory.

All this complexity is there to abstract from the differences between
software algos and h/w accelerators, but there only exists a single
instance of the latter in the tree, for HiSilicon SoCs, so this is
obviously not a resounding success.

In summary, we're better off sticking with the legacy comp interface,
but unfortunately, due to the way that has been plumbed into the
scomp/acomp with scatterlists version, that doesn't really help us get
rid of the memory overhead.


> > But how does the algo matter when you are dealing with mere kilobytes
> > of ASCII text?
>
> Sure, though, this is how we got here -- every couple of years, someone
> added another library interface to another compression aglo.

But why? How does that make a meaningful difference for compressing kernel logs?

> I tore all
> that out so we could avoid having to choose a single one, but was left
> with the zbufsize mess (that, yes, doesn't matter). So now pstore can
> just not care what compression is chosen.
>

What I propose is to simply hard wire pstore to a single existing
library implementation, and forget about the crypto API entirely.

We know the pros, given the above. So what would we lose that is
valuable by doing this?

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Guilherme G. Piccoli]

On 17/10/2022 18:01, Ard Biesheuvel wrote:
> [...]
> 
> In summary, we're better off sticking with the legacy comp interface,
> but unfortunately, due to the way that has been plumbed into the
> scomp/acomp with scatterlists version, that doesn't really help us get
> rid of the memory overhead.
>

Out of curiosity, do you have a number here, like X kilobytes per active
CPU?

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Ard Biesheuvel]

On Mon, 17 Oct 2022 at 23:10, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 17/10/2022 18:01, Ard Biesheuvel wrote:
> > [...]
> >
> > In summary, we're better off sticking with the legacy comp interface,
> > but unfortunately, due to the way that has been plumbed into the
> > scomp/acomp with scatterlists version, that doesn't really help us get
> > rid of the memory overhead.
> >
>
> Out of curiosity, do you have a number here, like X kilobytes per active
> CPU?

2x128 KB per CPU, which makes 128  KB also the maximum input/output
size per request when using this interface. (SCOMP_SCRATCH_SIZE)

On my 32x4 CPU workstation, this amounts to 32 MB permanently locked
up for nothing when the pstore driver is loaded (with compression
enabled)

Re: [PATCH] pstore: migrate to crypto acomp interface (take 2)

[Guilherme G. Piccoli]

On 17/10/2022 18:16, Ard Biesheuvel wrote:
> On Mon, 17 Oct 2022 at 23:10, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>> [...]
>> Out of curiosity, do you have a number here, like X kilobytes per active
>> CPU?
> 
> 2x128 KB per CPU, which makes 128  KB also the maximum input/output
> size per request when using this interface. (SCOMP_SCRATCH_SIZE)
> 
> On my 32x4 CPU workstation, this amounts to 32 MB permanently locked
> up for nothing when the pstore driver is loaded (with compression
> enabled)

Thanks! This is really something...