Re: [REGRESSION][PATCH] Revert "pstore: migrate to crypto acomp interface"

From: Kees Cook <keescook@chromium.org>
To: "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: linux-kernel@vger.kernel.org, regressions@lists.linux.dev,
	linux-fsdevel@vger.kernel.org, kernel-dev@igalia.com,
	kernel@gpiccoli.net, Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
	Thorsten Leemhuis <linux@leemhuis.info>
Subject: Re: [REGRESSION][PATCH] Revert "pstore: migrate to crypto acomp interface"
Date: Thu, 29 Sep 2022 20:29:12 -0700	[thread overview]
Message-ID: <202209291951.134BE2409@keescook> (raw)
In-Reply-To: <20220929215515.276486-1-gpiccoli@igalia.com>

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 <ardb@kernel.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

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