fs/pstore/platform.c | 74 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 17 deletions(-)
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.1On 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
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
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?
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
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.
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
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.
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
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?
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
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?
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?
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)
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...
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
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.
Thanks for re-sending this one, I'll test it next week =) Cheers, Guilherme
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
© 2016 - 2026 Red Hat, Inc.