This reverts commit e4f0a7ec586b7644107839f5394fb685cf1aadcc.
When using this new interface, both efi_pstore and ramoops
backends are unable to properly decompress dmesg if using
zstd, lz4 and lzo algorithms (and maybe more). It does succeed
with deflate though.
The message observed in the kernel log is:
[2.328828] pstore: crypto_acomp_decompress failed, ret = -22!
The pstore infrastructure is able to collect the dmesg with
both backends tested, but since decompression fails it's
unreadable. With this revert everything is back to normal.
Fixes: e4f0a7ec586b ("pstore: migrate to crypto acomp interface")
Cc: Ard Biesheuvel <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
Hi Ard, Thorsten and pstore maintainers. I've found this yday
during pstore development - it was "hidden" since I was using
deflate. Tried some fixes (I plan to submit a cast fix for a
long-term issue later), but nothing I tried fixed this.
So, I thought in sending this revert - feel free to ignore it if
anybody comes with a proper fix for the async compress interface
proposed by Ard. The idea of the revert is because the 6.0-rc
cycle is nearly over, and would be nice to avoid introducing
this regression.
Also, I searched some mailing list discussions / submission of
the patch ("pstore: migrate to crypto acomp interface"), but
couldn't find it - can any of you point it to me in case it's
in some archive?
Thanks in advance, and sorry for reporting this so late in the
cycle, I wish I'd found it before.
Cheers,
Guilherme
fs/pstore/platform.c | 63 +++++++++-----------------------------------
1 file changed, 12 insertions(+), 51 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index b2fd3c20e7c2..0c034ea39954 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -28,14 +28,11 @@
#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"
/*
@@ -93,8 +90,7 @@ module_param(compress, charp, 0444);
MODULE_PARM_DESC(compress, "compression to use");
/* Compression parameters */
-static struct crypto_acomp *tfm;
-static struct acomp_req *creq;
+static struct crypto_comp *tfm;
struct pstore_zbackend {
int (*zbufsize)(size_t size);
@@ -272,21 +268,12 @@ 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;
- 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);
+ ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
if (ret) {
pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
return ret;
@@ -297,7 +284,7 @@ static int pstore_compress(const void *in, void *out,
static void allocate_buf_for_compression(void)
{
- struct crypto_acomp *acomp;
+ struct crypto_comp *ctx;
int size;
char *buf;
@@ -309,7 +296,7 @@ static void allocate_buf_for_compression(void)
if (!psinfo || tfm)
return;
- if (!crypto_has_acomp(zbackend->name, 0, CRYPTO_ALG_ASYNC)) {
+ if (!crypto_has_comp(zbackend->name, 0, 0)) {
pr_err("Unknown compression: %s\n", zbackend->name);
return;
}
@@ -328,24 +315,16 @@ static void allocate_buf_for_compression(void)
return;
}
- acomp = crypto_alloc_acomp(zbackend->name, 0, CRYPTO_ALG_ASYNC);
- if (IS_ERR_OR_NULL(acomp)) {
+ ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+ if (IS_ERR_OR_NULL(ctx)) {
kfree(buf);
pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
- 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);
+ PTR_ERR(ctx));
return;
}
/* A non-NULL big_oops_buf indicates compression is available. */
- tfm = acomp;
+ tfm = ctx;
big_oops_buf_sz = size;
big_oops_buf = buf;
@@ -355,8 +334,7 @@ static void allocate_buf_for_compression(void)
static void free_buf_for_compression(void)
{
if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
- acomp_request_free(creq);
- crypto_free_acomp(tfm);
+ crypto_free_comp(tfm);
tfm = NULL;
}
kfree(big_oops_buf);
@@ -693,8 +671,6 @@ 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;
@@ -718,30 +694,16 @@ static void decompress_record(struct pstore_record *record)
if (!workspace)
return;
- dreq = acomp_request_alloc(tfm);
- if (!dreq) {
- kfree(workspace);
- return;
- }
-
- 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_acomp_decompress(dreq);
+ ret = crypto_comp_decompress(tfm, record->buf, record->size,
+ workspace, &unzipped_len);
if (ret) {
- pr_err("crypto_acomp_decompress failed, ret = %d!\n", ret);
+ pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
kfree(workspace);
return;
}
/* Append ECC notice to decompressed buffer. */
- unzipped_len = dreq->dlen;
memcpy(workspace + unzipped_len, record->buf + record->size,
record->ecc_notice_size);
@@ -749,7 +711,6 @@ static void decompress_record(struct pstore_record *record)
unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
GFP_KERNEL);
kfree(workspace);
- acomp_request_free(dreq);
if (!unzipped)
return;
--
2.37.3
On Thu, Sep 29, 2022 at 06:55:15PM -0300, Guilherme G. Piccoli wrote:
> This reverts commit e4f0a7ec586b7644107839f5394fb685cf1aadcc.
>
> When using this new interface, both efi_pstore and ramoops
> backends are unable to properly decompress dmesg if using
> zstd, lz4 and lzo algorithms (and maybe more). It does succeed
> with deflate though.
Hi!
Thanks for looking at this. I wasn't able to reproduce the problem,
initially. Booting with pstore.backend=ramoops pstore.compress=zstd and
writing to /dev/pmsg0, after a reboot I'm able to read it back.
> The message observed in the kernel log is:
>
> [2.328828] pstore: crypto_acomp_decompress failed, ret = -22!
Hmm, that's EINVAL.
> The pstore infrastructure is able to collect the dmesg with
> both backends tested, but since decompression fails it's
> unreadable. With this revert everything is back to normal.
>
> Fixes: e4f0a7ec586b ("pstore: migrate to crypto acomp interface")
> Cc: Ard Biesheuvel <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
A reminder to myself, I keep getting surprised that systemd is
stealing the pstore filesystem contents and moving them into
/var/lib/systemd/pstore/
> Hi Ard, Thorsten and pstore maintainers. I've found this yday
> during pstore development - it was "hidden" since I was using
> deflate. Tried some fixes (I plan to submit a cast fix for a
> long-term issue later), but nothing I tried fixed this.
What's your setup for this? I'm using emulated NVDIMM through qemu for
a ramoops backend. But trying this with the EFI backend (booting
undef EFI with pstore.backend=efi), I _do_ see the problem. That's
weird... I suspect there's some back interaction with buffer size
differences between ramoops and EFI & deflate and zstd.
And I can confirm EFI+zstd with the acomp change reverted fixes it.
Ard, anything jump to mind for you?
> So, I thought in sending this revert - feel free to ignore it if
> anybody comes with a proper fix for the async compress interface
> proposed by Ard. The idea of the revert is because the 6.0-rc
> cycle is nearly over, and would be nice to avoid introducing
> this regression.
>
> Also, I searched some mailing list discussions / submission of
> the patch ("pstore: migrate to crypto acomp interface"), but
> couldn't find it - can any of you point it to me in case it's
> in some archive?
Hm, it's possible this was just sent directly to me? If that's true, I
apologize for not re-posting it to lkml. I suspect I didn't notice at
the time that it wasn't CCed to a list.
> Thanks in advance, and sorry for reporting this so late in the
> cycle, I wish I'd found it before.
No worries! Whatever the case, there's always -stable updates. :)
-Kees
--
Kees Cook
[Note: this mail is primarily send for documentation purposes and/or for
regzbot, my Linux kernel regression tracking bot. That's why I removed
most or all folks from the list of recipients, but left any that looked
like a mailing lists. These mails usually contain '#forregzbot' in the
subject, to make them easy to spot and filter out.]
[TLDR: I'm adding this regression report to the list of tracked
regressions; all text from me you find below is based on a few templates
paragraphs you might have encountered already already in similar form.]
Hi, this is your Linux kernel regression tracker. Thx for the report.
On 29.09.22 23:55, Guilherme G. Piccoli wrote:
> This reverts commit e4f0a7ec586b7644107839f5394fb685cf1aadcc.
>
> When using this new interface, both efi_pstore and ramoops
> backends are unable to properly decompress dmesg if using
> zstd, lz4 and lzo algorithms (and maybe more). It does succeed
> with deflate though.
>
> The message observed in the kernel log is:
>
> [2.328828] pstore: crypto_acomp_decompress failed, ret = -22!
>
> The pstore infrastructure is able to collect the dmesg with
> both backends tested, but since decompression fails it's
> unreadable. With this revert everything is back to normal.
>
> Fixes: e4f0a7ec586b ("pstore: migrate to crypto acomp interface")
> Cc: Ard Biesheuvel <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> Hi Ard, Thorsten and pstore maintainers. I've found this yday
> during pstore development - it was "hidden" since I was using
> deflate. Tried some fixes (I plan to submit a cast fix for a
> long-term issue later), but nothing I tried fixed this.
>
> So, I thought in sending this revert - feel free to ignore it if
> anybody comes with a proper fix for the async compress interface
> proposed by Ard. The idea of the revert is because the 6.0-rc
> cycle is nearly over, and would be nice to avoid introducing
> this regression.
>
> Also, I searched some mailing list discussions / submission of
> the patch ("pstore: migrate to crypto acomp interface"), but
> couldn't find it - can any of you point it to me in case it's
> in some archive?
>
> Thanks in advance, and sorry for reporting this so late in the
> cycle, I wish I'd found it before.
Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:
#regzbot ^introduced e4f0a7ec586b
#regzbot title pstore: efi_pstore and ramoops backends are sometimes
unable to properly decompress dmesg
#regzbot ignore-activity
This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply -- ideally with also
telling regzbot about it, as explained here:
https://linux-regtracking.leemhuis.info/tracked-regression/
Reminder for developers: When fixing the issue, add 'Link:' tags
pointing to the report (the mail this one replies to), as explained for
in the Linux kernel's documentation; above webpage explains why this is
important for tracked regressions.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.
On 30/09/2022 00:29, Kees Cook wrote:
> [...]
>
> Hi!
>
> Thanks for looking at this. I wasn't able to reproduce the problem,
> initially. Booting with pstore.backend=ramoops pstore.compress=zstd and
> writing to /dev/pmsg0, after a reboot I'm able to read it back.
>
Hi Kees, thanks a lot for your attention!
IIUC, compression applies to dmesg only, correct?
> [...]
> What's your setup for this? I'm using emulated NVDIMM through qemu for
> a ramoops backend. But trying this with the EFI backend (booting
> undef EFI with pstore.backend=efi), I _do_ see the problem. That's
> weird... I suspect there's some back interaction with buffer size
> differences between ramoops and EFI & deflate and zstd.
>
> And I can confirm EFI+zstd with the acomp change reverted fixes it.
>
I'm using qemu but was able to use real HW (Steam Deck). In both cases,
kernel is not using the entire RAM ("mem=" parameter, for example) so we
can use a bit for ramoops. Also, both setups are UEFI, hence I can also
use efi_pstore.
> [...]
> Hm, it's possible this was just sent directly to me? If that's true, I
> apologize for not re-posting it to lkml. I suspect I didn't notice at
> the time that it wasn't CCed to a list.
No need for apologies, thanks for the clarification! How about if we add
a mailing list in the pstore entry on MAINTAINERS file, since it's just
composed for you and 3 other people now? I mean, "officially" speaking,
it should be enough to send a patch for the 4 maintainers with no list
in CC, and that's bad for achieving purposes. What list should be the
best, fsdevel? Lkml?
>
> No worries! Whatever the case, there's always -stable updates. :)
Heheh you're right! But for something like this (pstore/dmesg
compression broke for the most backends), I'd be glad if we could fix it
before the release.
Cheers,
Guilherme
On Fri, 30 Sept 2022 at 14:39, Guilherme G. Piccoli <[email protected]> wrote:
>
> On 30/09/2022 00:29, Kees Cook wrote:
> > [...]
> >
> > Hi!
> >
> > Thanks for looking at this. I wasn't able to reproduce the problem,
> > initially. Booting with pstore.backend=ramoops pstore.compress=zstd and
> > writing to /dev/pmsg0, after a reboot I'm able to read it back.
> >
>
> Hi Kees, thanks a lot for your attention!
> IIUC, compression applies to dmesg only, correct?
>
>
> > [...]
> > What's your setup for this? I'm using emulated NVDIMM through qemu for
> > a ramoops backend. But trying this with the EFI backend (booting
> > undef EFI with pstore.backend=efi), I _do_ see the problem. That's
> > weird... I suspect there's some back interaction with buffer size
> > differences between ramoops and EFI & deflate and zstd.
> >
> > And I can confirm EFI+zstd with the acomp change reverted fixes it.
> >
>
> I'm using qemu but was able to use real HW (Steam Deck). In both cases,
> kernel is not using the entire RAM ("mem=" parameter, for example) so we
> can use a bit for ramoops. Also, both setups are UEFI, hence I can also
> use efi_pstore.
>
Does this help?
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index b2fd3c20e7c2..c0b609d7d04e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -292,7 +292,7 @@ static int pstore_compress(const void *in, void *out,
return ret;
}
- return outlen;
+ return creq->dlen;
}
static void allocate_buf_for_compression(void)
>
> > [...]
> > Hm, it's possible this was just sent directly to me? If that's true, I
> > apologize for not re-posting it to lkml. I suspect I didn't notice at
> > the time that it wasn't CCed to a list.
>
> No need for apologies, thanks for the clarification! How about if we add
> a mailing list in the pstore entry on MAINTAINERS file, since it's just
> composed for you and 3 other people now? I mean, "officially" speaking,
> it should be enough to send a patch for the 4 maintainers with no list
> in CC, and that's bad for achieving purposes. What list should be the
> best, fsdevel? Lkml?
>
Makes sense
>
> >
> > No worries! Whatever the case, there's always -stable updates. :)
>
> Heheh you're right! But for something like this (pstore/dmesg
> compression broke for the most backends), I'd be glad if we could fix it
> before the release.
Yeah better to revert - this was not a critical change anyway. But I
think the tweak above should fix things (it works for me here)
On Fri, Sep 30, 2022 at 03:31:17PM -0300, Guilherme G. Piccoli wrote:
> On 30/09/2022 12:51, Ard Biesheuvel wrote:
> > [...]
> >
> > Does this help?
> >
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index b2fd3c20e7c2..c0b609d7d04e 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -292,7 +292,7 @@ static int pstore_compress(const void *in, void *out,
> > return ret;
> > }
> >
> > - return outlen;
> > + return creq->dlen;
> > }
> >
> > static void allocate_buf_for_compression(void)
> >
>
> Thanks a lot Ard, this seems to be the fix! Tested with lz4/zstd/deflate
> in both ramoops/efi backends, and all worked fine. It makes sense,
> outlen was modified in the previous API and not in the acomp thing, so
> it was a good catch =)
>
>
> >> Heheh you're right! But for something like this (pstore/dmesg
> >> compression broke for the most backends), I'd be glad if we could fix it
> >> before the release.
> >
> > Yeah better to revert - this was not a critical change anyway. But I
> > think the tweak above should fix things (it works for me here)
>
> Agreed - in fact seems it was reverted already. More than that, I found
> yet another small issue in the acomp refactor, a memory leak - attached
> is a patch with the fix, feel free to integrate in your acomp refactor
> when re-submitting (I mean, feel free to just integrate the code, don't
> need to send it as a separate patch/fix).
>
> I'm also working some fixes in implicit conversions in pstore that
> aren't great (unsigned -> int in many places), I'll send some stuff next
> week.
Awesome! Thank you both! I'll probably be busy for the next week with
the merge window, but after that I'll pull new stuff into -next for
pstore. I appreciate more people poking around in the code. :)
--
Kees Cook
On 30/09/2022 12:51, Ard Biesheuvel wrote:
> [...]
>
> Does this help?
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index b2fd3c20e7c2..c0b609d7d04e 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -292,7 +292,7 @@ static int pstore_compress(const void *in, void *out,
> return ret;
> }
>
> - return outlen;
> + return creq->dlen;
> }
>
> static void allocate_buf_for_compression(void)
>
Thanks a lot Ard, this seems to be the fix! Tested with lz4/zstd/deflate
in both ramoops/efi backends, and all worked fine. It makes sense,
outlen was modified in the previous API and not in the acomp thing, so
it was a good catch =)
>> Heheh you're right! But for something like this (pstore/dmesg
>> compression broke for the most backends), I'd be glad if we could fix it
>> before the release.
>
> Yeah better to revert - this was not a critical change anyway. But I
> think the tweak above should fix things (it works for me here)
Agreed - in fact seems it was reverted already. More than that, I found
yet another small issue in the acomp refactor, a memory leak - attached
is a patch with the fix, feel free to integrate in your acomp refactor
when re-submitting (I mean, feel free to just integrate the code, don't
need to send it as a separate patch/fix).
I'm also working some fixes in implicit conversions in pstore that
aren't great (unsigned -> int in many places), I'll send some stuff next
week.
Cheers,
Guilherme