[GIT PULL] pstore updates for v6.6-rc1

[GIT PULL] pstore updates for v6.6-rc1

[Kees Cook]

Hi Linus,

Please pull these pstore updates for v6.6-rc1. This contains a fair bit
of code _removal_ which is always nice. Changes have been in -next for
most of the development cycle.

Thanks!

-Kees

The following changes since commit fdf0eaf11452d72945af31804e2a1048ee1b574c:

  Linux 6.5-rc2 (2023-07-16 15:10:37 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/pstore-v6.6-rc1

for you to fetch changes up to af58740d8b06a6a97b7594235a1be11bd6aa37fa:

  pstore: Fix kernel-doc warning (2023-08-18 13:27:28 -0700)

----------------------------------------------------------------
pstore updates for v6.6-rc1

- Greatly simplify compression support (Ard Biesheuvel).

- Avoid crashes for corrupted offsets when prz size is 0 (Enlin Mu).

- Expand range of usable record sizes (Yuxiao Zhang).

- Fix kernel-doc warning (Matthew Wilcox).

----------------------------------------------------------------
Ard Biesheuvel (2):
      pstore: Remove worst-case compression size logic
      pstore: Replace crypto API compression with zlib_deflate library calls

Enlin Mu (1):
      pstore/ram: Check start of empty przs during init

Matthew Wilcox (Oracle) (1):
      pstore: Fix kernel-doc warning

Yuxiao Zhang (1):
      pstore: Support record sizes larger than kmalloc() limit

 fs/pstore/Kconfig    | 100 ++-------------
 fs/pstore/inode.c    |   2 +-
 fs/pstore/platform.c | 353 +++++++++++++++++----------------------------------
 fs/pstore/ram.c      |  11 +-
 fs/pstore/ram_core.c |  17 ++-
 5 files changed, 137 insertions(+), 346 deletions(-)

-- 
Kees Cook

Re: [GIT PULL] pstore updates for v6.6-rc1

[pr-tracker-bot]

The pull request you sent on Mon, 28 Aug 2023 11:21:23 -0700:

> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/pstore-v6.6-rc1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5b07aaca1809f459d74589c38b20f87da554027f

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] pstore updates for v6.6-rc1

[Linus Torvalds]

On Mon, 28 Aug 2023 at 11:21, Kees Cook <keescook@chromium.org> wrote:
>
> Please pull these pstore updates for v6.6-rc1. This contains a fair bit
> of code _removal_ which is always nice.

Hmm. The diffstat certainly looks good, but the end result isn't great..

I now get 124 lines of

   pstore: zlib_inflate() failed, ret = -5!

in my bootup dmesg.

Considering that there's no reason for pstore to even be active on
this machine, I think it's because pstore now goes and tries to
uncompress something entirely invalid.

The message itself does not seem to be new, but with the switch from
the crypto code, it apparently used to be

    crypto_comp_decompress failed, ret = %d!

but the key word here is *apparently*. I never got that message
before. So something else has changed, and I'm thinking that the old
code probably didn't even try to decompress the bogus data it found?

I dunno. But 124 lines of insane garbage in the kernel messages is not
a good thing.

                  Linus

Re: [GIT PULL] pstore updates for v6.6-rc1

[Kees Cook]

On August 28, 2023 4:56:00 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Mon, 28 Aug 2023 at 11:21, Kees Cook <keescook@chromium.org> wrote:
>>
>> Please pull these pstore updates for v6.6-rc1. This contains a fair bit
>> of code _removal_ which is always nice.
>
>Hmm. The diffstat certainly looks good, but the end result isn't great..
>
>I now get 124 lines of
>
>   pstore: zlib_inflate() failed, ret = -5!
>
>in my bootup dmesg.
>
>Considering that there's no reason for pstore to even be active on
>this machine, I think it's because pstore now goes and tries to
>uncompress something entirely invalid.
>
>The message itself does not seem to be new, but with the switch from
>the crypto code, it apparently used to be
>
>    crypto_comp_decompress failed, ret = %d!
>
>but the key word here is *apparently*. I never got that message
>before. So something else has changed, and I'm thinking that the old
>code probably didn't even try to decompress the bogus data it found?
>
>I dunno. But 124 lines of insane garbage in the kernel messages is not
>a good thing.

Oh dear! That's obviously unexpected. I have so many questions. :P

- does this happen at every boot? (I assume yes.)
- what CONFIG are you built with?
- what was the prior CONFIG?
- what backend is in use? (Or better yet, what does "dmesg | grep pstore" report?)
- are you using systemd?

Decompression is only attempted if it's a valid record. If the records aren't being removed after boot (i.e. unlinked from /sys/fs/pstore) they won't get cleared. Normally systemd-pstore moves everything to /var/lib/systemd/pstore. But that must not be happening since you keep seeing the warnings.

That you have 124 of these makes me think you've got the EFI backend (CONFIG_EFI_VARS_PSTORE) built and it's default enabled (CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE=n). The latter config was created to keep the EFI backend from filling the EFI variable space. I think distros started setting it to "n" once systemd-pstore was added, which keeps the EFI variables from piling up...

So, I assume either systemd-pstore isn't running for you or something has gone sideways with it. And since I did testing of "changed compression type" without systemd-pstore, I bet systemd-pstore ignores the failed records...
https://github.com/systemd/systemd/blob/599a3124849819ba5af0a71b7572e87256814881/src/pstore/pstore.c#L225
Yup. Ugh. (Though I still find it odd that you have 124 records...)

Let me think about the best way to deal with this. I expect I'll have pstore wipe the failed records as it is expressly not expected to work across differing configs/kernel versions. And permanently spewing errors is not ok.

In the meantime, you can make the warnings go away with:

rm /sys/fs/pstore/*enc.z


-- 
Kees Cook

Re: [GIT PULL] pstore updates for v6.6-rc1

[Linus Torvalds]

On Mon, 28 Aug 2023 at 18:28, Kees Cook <kees@kernel.org> wrote:
>
> - does this happen at every boot? (I assume yes.)

Yes so far. I don't boot between every pull, so I think I've only had
two boots since the pstore pull, but both of them have this.

I also see it on my laptop, so it's not hw-specific.

> - what CONFIG are you built with?

I'll send my config separately in private, I don't think we want to
have that kind of noisy thing on the list. But it's basically a
standard Fedora config, cut down with "make localmodconfig" and with
some of the more annoying options disabled.

> - what was the prior CONFIG?

No changes, apart from "make oldconfig".

> - what backend is in use? (Or better yet, what does "dmesg | grep pstore" report?)

  [torvalds@ryzen linux]$ dmesg -t | grep pstore
  pstore: Using crash dump compression: deflate
  pstore: Registered efi_pstore as persistent store backend
  pstore: zlib_inflate() failed, ret = -5!
  pstore: zlib_inflate() failed, ret = -5!
  .. repeats a lot ..

> - are you using systemd?

Yup. Standard F37 install - except for the kernel, of course.

It does happen right after

  Write protecting the kernel read-only data: 22528k
  Freeing unused kernel image (rodata/data gap) memory: 224K
  Run /init as init process
    with arguments:
      /init
      rhgb
    with environment:
      HOME=/
      TERM=linux
      BOOT_IMAGE=(hd0,gpt2)/vmlinuz-6.5.0-00771-g5af3e822077e
      resume=/dev/mapper/fedora_localhost--live-swap
  [ lots of "pstore: zlib_inflate() failed, ret = -5!" ]

and just before systemd adds

  systemd[1]: systemd 251.14-2.fc37 running in system mode (+PAM
+AUDIT +SELINUX -APPARMOR +IMA +SMAC>
  systemd[1]: Detected architecture x86-64.

to the logs.

Which could easily mean that it's triggered by something systemd does
very early.

But none of those other messages are new, and that systemd version
that it reports is the same it has reported before (without any storm
of zlib_inflate() error messages).

> Let me think about the best way to deal with this. I expect I'll have pstore wipe the failed records as it is expressly not expected to work across differing configs/kernel versions. And permanently spewing errors is not ok.

So none of that sounds new.

The only thing that is new is the kernel pstore implementation. Why
was this not a problem before? The warning existed back then too, but
I never actually got it.

I get the feeling that you are overlooking that basic fact.

                 Linus

Re: [GIT PULL] pstore updates for v6.6-rc1

[Kees Cook]

On Mon, Aug 28, 2023 at 06:44:02PM -0700, Linus Torvalds wrote:
> The only thing that is new is the kernel pstore implementation. Why
> was this not a problem before? The warning existed back then too, but
> I never actually got it.

Right -- if the compression method from before was different, it'll fail
now. (i.e. we removed everything but zlib.)

> I get the feeling that you are overlooking that basic fact.

That's why I was wondering about the prior config; it could confirm the
default compression algo. But digging around it seems like zlib is the
default in the F37 kernel config. I'll keep looking; there is clearly
some combination I don't know.

I remain concerned about why there are 124. That's a LOT, and without
prior warnings, I don't know why systemd-pstore wasn't removing them.
Can you send me "ls -la /sys/fs/pstore" ? Maybe they aren't a dump type
that systemd knows about.

I will try to reproduce this with an F37 image...

-- 
Kees Cook

Re: [GIT PULL] pstore updates for v6.6-rc1

[Linus Torvalds]

On Mon, 28 Aug 2023 at 20:44, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Aug 28, 2023 at 06:44:02PM -0700, Linus Torvalds wrote:
> > The only thing that is new is the kernel pstore implementation. Why
> > was this not a problem before? The warning existed back then too, but
> > I never actually got it.
>
> Right -- if the compression method from before was different, it'll fail
> now. (i.e. we removed everything but zlib.)

I don't think that was the case.

Looking back in my logs, I see lines like this:

  Aug 07 16:59:29 ryzen kernel: pstore: Using crash dump compression: deflate

and while it appears F37 used to support other formats, it does have

  CONFIG_PSTORE_DEFLATE_COMPRESS_DEFAULT=y

so it should all be zlib-compatible from what I can tell.

                  Linus

Re: [GIT PULL] pstore updates for v6.6-rc1

[Ard Biesheuvel]

On Tue, 29 Aug 2023 at 19:13, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 28 Aug 2023 at 20:44, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Aug 28, 2023 at 06:44:02PM -0700, Linus Torvalds wrote:
> > > The only thing that is new is the kernel pstore implementation. Why
> > > was this not a problem before? The warning existed back then too, but
> > > I never actually got it.
> >
> > Right -- if the compression method from before was different, it'll fail
> > now. (i.e. we removed everything but zlib.)
>
> I don't think that was the case.
>
> Looking back in my logs, I see lines like this:
>
>   Aug 07 16:59:29 ryzen kernel: pstore: Using crash dump compression: deflate
>
> and while it appears F37 used to support other formats, it does have
>
>   CONFIG_PSTORE_DEFLATE_COMPRESS_DEFAULT=y
>
> so it should all be zlib-compatible from what I can tell.
>

-5 == Z_BUF_ERROR which is only returned by zlib_inflate() in one
particular case (according the the kerneldoc):

    In this implementation, the flush parameter of inflate() only affects the
   return code (per zlib.h).  inflate() always writes as much as possible to
   strm->next_out, given the space available and the provided input--the effect
   documented in zlib.h of Z_SYNC_FLUSH.  Furthermore, inflate() always defers
   the allocation of and copying into a sliding window until necessary, which
   provides the effect documented in zlib.h for Z_FINISH when the entire input
   stream available.  So the only thing the flush parameter actually does is:
   when flush is set to Z_FINISH, inflate() cannot return Z_OK.  Instead it
   will return Z_BUF_ERROR if it has not reached the end of the stream.

and the crypto compress wrapper for inflate does

        ret = zlib_inflate(stream, Z_SYNC_FLUSH);
        /*
         * Work around a bug in zlib, which sometimes wants to taste an extra
         * byte when being used in the (undocumented) raw deflate mode.
         * (From USAGI).
         */
        if (ret == Z_OK && !stream->avail_in && stream->avail_out) {
                u8 zerostuff = 0;
                stream->next_in = &zerostuff;
                stream->avail_in = 1;
                ret = zlib_inflate(stream, Z_FINISH);
        }

IOW, it does not use Z_FINISH but Z_SYNC_FLUSH for the primary
invocation, and only stuffs in one additional NUL byte if it returns
Z_OK instead of Z_STREAM_END.

This is an oversight on my part. The diff below plugs this into the pstore code

--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -593,7 +593,13 @@ static void decompress_record(struct pstore_record *record,
        zstream->next_out       = workspace;
        zstream->avail_out      = psinfo->bufsize;

-       ret = zlib_inflate(zstream, Z_FINISH);
+       ret = zlib_inflate(zstream, Z_SYNC_FLUSH);
+       if (ret == Z_OK && !zstream->avail_in && zstream->avail_out) {
+               u8 zerostuff = 0;
+               zstream->next_in = &zerostuff;
+               zstream->avail_in = 1;
+               ret = zlib_inflate(zstream, Z_FINISH);
+       }
        if (ret != Z_STREAM_END) {
                pr_err("zlib_inflate() failed, ret = %d!\n", ret);
                kvfree(workspace);

Re: [GIT PULL] pstore updates for v6.6-rc1

[Linus Torvalds]

On Tue, 29 Aug 2023 at 10:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> This is an oversight on my part. The diff below plugs this into the pstore code

Hmm. My reaction is that you should also add the comment about the
"Work around a bug in zlib" issue, because this code makes no sense
otherwise.

Of course, potentially even better would be to actually move this fix
into our copy of zlib. Does the bug / misfeature still exist in
upstream zlib?

Also, grepping around a bit, I note that btrfs, for example, just does
that Z_SYNC_FLUSH, and then checks for Z_OK. None of this Z_STREAM_END
stuff.

Similarly, going off to the debian code search, I find other code that
just accepts either Z_OK or Z_STREAM_END.

So what's so magical about how pstore uses zlib? This is just very odd.

             Linus

Re: [GIT PULL] pstore updates for v6.6-rc1

[Ard Biesheuvel]

On Tue, 29 Aug 2023 at 20:04, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 29 Aug 2023 at 10:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > This is an oversight on my part. The diff below plugs this into the pstore code
>
> Hmm. My reaction is that you should also add the comment about the
> "Work around a bug in zlib" issue, because this code makes no sense
> otherwise.
>

Naturally. But pasting the comment into the email body seemed unnecessary.

> Of course, potentially even better would be to actually move this fix
> into our copy of zlib. Does the bug / misfeature still exist in
> upstream zlib?
>

I have no idea. You are the one sitting on the only [potential]
reproducer I am aware of, and there is nothing in the git (or prior)
history that sheds any light on this. Could you copy one of those EFI
variables to a file and share it so I can try and reproduce this?

> Also, grepping around a bit, I note that btrfs, for example, just does
> that Z_SYNC_FLUSH, and then checks for Z_OK. None of this Z_STREAM_END
> stuff.
>
> Similarly, going off to the debian code search, I find other code that
> just accepts either Z_OK or Z_STREAM_END.
>
> So what's so magical about how pstore uses zlib? This is just very odd.
>

AIUI, zlib can be used in raw mode and with a header/footer. Passing
the wbits parameter as a negative number (!) will switch into raw
mode.

The btrfs and jffs2 occurrences both default to positive wbits, and
switch to negative in a very specific case that involves zlib
internals that I don't understand. crypto/deflate.c implements both
modes, and pstore always used the raw one in all cases.

The workaround in crypto/deflate.c is documented as being specific to
the raw mode, which is why it makes sense to at least verify whether
the same workaround that pstore-deflate was using when doing raw zlib
through the crypto API is still needed now that it calls zlib
directly.

Re: [GIT PULL] pstore updates for v6.6-rc1

[Eric Biggers]

Hi,

On Tue, Aug 29, 2023 at 11:43:37PM +0200, Ard Biesheuvel wrote:
> On Tue, 29 Aug 2023 at 20:04, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, 29 Aug 2023 at 10:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > This is an oversight on my part. The diff below plugs this into the pstore code
> >
> > Hmm. My reaction is that you should also add the comment about the
> > "Work around a bug in zlib" issue, because this code makes no sense
> > otherwise.
> >
> 
> Naturally. But pasting the comment into the email body seemed unnecessary.
> 
> > Of course, potentially even better would be to actually move this fix
> > into our copy of zlib. Does the bug / misfeature still exist in
> > upstream zlib?
> >
> 
> I have no idea. You are the one sitting on the only [potential]
> reproducer I am aware of, and there is nothing in the git (or prior)
> history that sheds any light on this. Could you copy one of those EFI
> variables to a file and share it so I can try and reproduce this?
> 
> > Also, grepping around a bit, I note that btrfs, for example, just does
> > that Z_SYNC_FLUSH, and then checks for Z_OK. None of this Z_STREAM_END
> > stuff.
> >
> > Similarly, going off to the debian code search, I find other code that
> > just accepts either Z_OK or Z_STREAM_END.
> >
> > So what's so magical about how pstore uses zlib? This is just very odd.
> >
> 
> AIUI, zlib can be used in raw mode and with a header/footer. Passing
> the wbits parameter as a negative number (!) will switch into raw
> mode.
> 
> The btrfs and jffs2 occurrences both default to positive wbits, and
> switch to negative in a very specific case that involves zlib
> internals that I don't understand. crypto/deflate.c implements both
> modes, and pstore always used the raw one in all cases.
> 
> The workaround in crypto/deflate.c is documented as being specific to
> the raw mode, which is why it makes sense to at least verify whether
> the same workaround that pstore-deflate was using when doing raw zlib
> through the crypto API is still needed now that it calls zlib
> directly.

I get the "pstore: zlib_inflate() failed, ret = -5!" messages on my system too,
so I looked into it.  What's happening is that the pstore records are coming
from the efi_pstore backend, which has pstore_info::bufsize of 1024 bytes, but
they decompress to more than 1024 bytes.  Since pstore now sizes the buffer for
decompressed data to only pstore_info::bufsize, lib/zlib_inflate correctly
returns Z_BUF_ERROR.  (BTW, I write "lib/zlib_inflate", not "zlib", since it was
forked from the real zlib 25 years ago.  Regardless, the problem isn't there.)

I think we partially misinterpreted the functions like zbufsize_deflate() that
Ard's patches removed.  They seemed to be used only for getting the worst case
compressed size for an uncompressed size of pstore_info::bufsize.  Actually,
they were used both for that, *and* for getting the uncompressed size to try to
compress into pstore_info::bufsize.  (Which is very weird, as these are two very
different concepts.)

So I think first we need to decide whether pstore should try to use compression
to fit more than pstore_info::bufsize of data in each pstore record, as opposed
to just shrinking the size of the record actually written.  If yes, then this
really needs some more thought than the previous code which seemed to do this by
accident.  If no, then the new approach is fine.

BTW, what happens if someone was using a non-DEFLATE algorithm and then upgrades
their kernel?  Decompression can fail for that too.  So maybe pstore just needs
to consider that decompression of pstore records written by an older kernel can
fail -- either due to the algorithm changing or due to the uncompressed size
being too large for the new code -- and silence the error messages accordingly?
How "persistent" do these persistent store records really have to be?

- Eric

Re: [GIT PULL] pstore updates for v6.6-rc1

[Ard Biesheuvel]

On Wed, 30 Aug 2023 at 08:05, Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi,
>
> On Tue, Aug 29, 2023 at 11:43:37PM +0200, Ard Biesheuvel wrote:
> > On Tue, 29 Aug 2023 at 20:04, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > On Tue, 29 Aug 2023 at 10:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > This is an oversight on my part. The diff below plugs this into the pstore code
> > >
> > > Hmm. My reaction is that you should also add the comment about the
> > > "Work around a bug in zlib" issue, because this code makes no sense
> > > otherwise.
> > >
> >
> > Naturally. But pasting the comment into the email body seemed unnecessary.
> >
> > > Of course, potentially even better would be to actually move this fix
> > > into our copy of zlib. Does the bug / misfeature still exist in
> > > upstream zlib?
> > >
> >
> > I have no idea. You are the one sitting on the only [potential]
> > reproducer I am aware of, and there is nothing in the git (or prior)
> > history that sheds any light on this. Could you copy one of those EFI
> > variables to a file and share it so I can try and reproduce this?
> >
> > > Also, grepping around a bit, I note that btrfs, for example, just does
> > > that Z_SYNC_FLUSH, and then checks for Z_OK. None of this Z_STREAM_END
> > > stuff.
> > >
> > > Similarly, going off to the debian code search, I find other code that
> > > just accepts either Z_OK or Z_STREAM_END.
> > >
> > > So what's so magical about how pstore uses zlib? This is just very odd.
> > >
> >
> > AIUI, zlib can be used in raw mode and with a header/footer. Passing
> > the wbits parameter as a negative number (!) will switch into raw
> > mode.
> >
> > The btrfs and jffs2 occurrences both default to positive wbits, and
> > switch to negative in a very specific case that involves zlib
> > internals that I don't understand. crypto/deflate.c implements both
> > modes, and pstore always used the raw one in all cases.
> >
> > The workaround in crypto/deflate.c is documented as being specific to
> > the raw mode, which is why it makes sense to at least verify whether
> > the same workaround that pstore-deflate was using when doing raw zlib
> > through the crypto API is still needed now that it calls zlib
> > directly.
>
> I get the "pstore: zlib_inflate() failed, ret = -5!" messages on my system too,
> so I looked into it.  What's happening is that the pstore records are coming
> from the efi_pstore backend, which has pstore_info::bufsize of 1024 bytes, but
> they decompress to more than 1024 bytes.  Since pstore now sizes the buffer for
> decompressed data to only pstore_info::bufsize, lib/zlib_inflate correctly
> returns Z_BUF_ERROR.  (BTW, I write "lib/zlib_inflate", not "zlib", since it was
> forked from the real zlib 25 years ago.  Regardless, the problem isn't there.)
>
> I think we partially misinterpreted the functions like zbufsize_deflate() that
> Ard's patches removed.  They seemed to be used only for getting the worst case
> compressed size for an uncompressed size of pstore_info::bufsize.  Actually,
> they were used both for that, *and* for getting the uncompressed size to try to
> compress into pstore_info::bufsize.  (Which is very weird, as these are two very
> different concepts.)
>

Agreed. The whole point of that worst-case logic is that a given
buffer may expand due to compression overhead, so the input should be
consumed in chunks of a fixed size N, with the buffer space sized
according to worst-case-comp-size(N) (although storing the buffer
compressed in that case is also pointless, as we have already
established). The existing code seems to do the opposite: it consumes
the input in chunks of worst-case-comp-size(N) in order to store it
into a buffer of size N. This happens to work because this code only
ever operates on ASCII text but it makes no sense whatsoever.

Another reason to get rid of this stuff - it is seriously broken.

> So I think first we need to decide whether pstore should try to use compression
> to fit more than pstore_info::bufsize of data in each pstore record, as opposed
> to just shrinking the size of the record actually written.  If yes, then this
> really needs some more thought than the previous code which seemed to do this by
> accident.  If no, then the new approach is fine.
>

Given that only deflate is left now, we can easily bring that back,
although I question the utility. However, deflate being the default,
this is going to affect many people and so I think bringing it back
makes sense on that basis alone, but only on the decompress side.

> BTW, what happens if someone was using a non-DEFLATE algorithm and then upgrades
> their kernel?  Decompression can fail for that too.  So maybe pstore just needs
> to consider that decompression of pstore records written by an older kernel can
> fail -- either due to the algorithm changing or due to the uncompressed size
> being too large for the new code -- and silence the error messages accordingly?
> How "persistent" do these persistent store records really have to be?

This was mentioned in the cover letter: pstore is a last-resort
diagnostic facility, and given how pointless all this configurability
was, I seriously doubt that the removal of all those algorithms is
going to have an impact.

In any case, I'll rate limit the error so it doesn't clutter up the logs.

Re: [GIT PULL] pstore updates for v6.6-rc1

[Herbert Xu]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> Of course, potentially even better would be to actually move this fix
> into our copy of zlib. Does the bug / misfeature still exist in
> upstream zlib?
> 
> Also, grepping around a bit, I note that btrfs, for example, just does
> that Z_SYNC_FLUSH, and then checks for Z_OK. None of this Z_STREAM_END
> stuff.
> 
> Similarly, going off to the debian code search, I find other code that
> just accepts either Z_OK or Z_STREAM_END.
> 
> So what's so magical about how pstore uses zlib? This is just very odd.

The Crypto API DEFLATE algorithm was added for IPComp (RFC 2394).
So there is a specific format required and to achieve that through
zlib, you must use raw mode.

Later on someone added "zlib-deflate" to the Crypto API which does
emit the zlib header/trailer.  It appears to be completely unused
and it was only added because certain hardware happened be able to
emit the same header/trailer.  We should remove zlib-defalte
and all the driver implementations of it from the Crypto API.

Normally, zlib will emit a header and a checksum trailer.  That's
why its implementation assumes that there will be at least one (or
in fact two) byte(s) after the end of the compressed data.  I haven't
checked the latest upstream but I would presume that it would still
make the same assumption as raw mode (or DEFLATE) is an undocumented
feature of zlib.

Coming back to pstore, for better or worse the original implementation
of pstore chose the Crypto API deflate algorithm so it produces
output that is equivalent to that of RFC 2394.  Therefore it relies
on zlib raw mode and it must supply an extra byte at the end of
the compressed data to ensure that decompression works properly.

The other in-kernel users of zlib appears to use it with a header and
trailer so they are unaffected by this.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [GIT PULL] pstore updates for v6.6-rc1

[Kees Cook]

On Wed, Aug 30, 2023 at 09:48:48AM +0200, Ard Biesheuvel wrote:
> In any case, I'll rate limit the error so it doesn't clutter up the logs.

Great; thanks for looking at it!

A related issue I'm going to tackle is dealing with the risk of
ever-growing record counts for backends that don't treat their storage
as a circular buffer. (e.g. ramoops will overwrite the latest record
when it runs out of empty areas, but EFI will just keep on writing new
records.) It's clear we can't depend on userspace to do this clean-up.
I think pstore tossing the oldest records above a (configurable) limit
(say, 32) per dump type makes sense...

-- 
Kees Cook