Hi pstore maintainers, this is a small series with some improvements
overall. Most of them are minors, but the implicit conversion thing
is a bit more "relevant" in a sense it's more invasive and would fit
more as a "fix".
The code is based on v6.0, and it was tested with multiple compression
algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
My plan is to also work some ramoops improvements (for example, related
to [0]); they're more complex so it's deferred for a second series,
specific for that.
Reviews and comments are greatly appreciated!
Thanks in advance,
Guilherme
[0] https://lore.kernel.org/lkml/[email protected]/
Guilherme G. Piccoli (8):
pstore: Improve error reporting in case of backend overlap
pstore: Expose kmsg_bytes as a module parameter
pstore: Inform unregistered backend names as well
pstore: Alert on backend write error
pstore: Fix long-term implicit conversions in the compression routines
MAINTAINERS: Add a mailing-list for the pstore infrastructure
efi: pstore: Follow convention for the efi-pstore backend name
efi: pstore: Add module parameter for setting the record size
MAINTAINERS | 1 +
drivers/firmware/efi/efi-pstore.c | 17 +++++---
fs/pstore/platform.c | 64 ++++++++++++++++---------------
3 files changed, 46 insertions(+), 36 deletions(-)
--
2.38.0
The pstore infrastructure supports one single backend at a time;
trying to load a another backend causes an error and displays a
message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
register_pstore() error reporting").
Happens that this message is not really clear about the situation,
also the current error returned (-EPERM) isn't accurate, whereas
-EBUSY makes more sense. We have another place in the code that
relies in the -EBUSY return for a similar check.
So, make it consistent here by returning -EBUSY and using a
similar message in both scenarios.
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
fs/pstore/platform.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 0c034ea39954..c32957e4b256 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
int pstore_register(struct pstore_info *psi)
{
if (backend && strcmp(backend, psi->name)) {
- pr_warn("ignoring unexpected backend '%s'\n", psi->name);
- return -EPERM;
+ pr_warn("backend '%s' already in use: ignoring '%s'\n",
+ backend, psi->name);
+ return -EBUSY;
}
/* Sanity check flags. */
--
2.38.0
Currently, this entry contains only the maintainers name. Add hereby
a mailing-list as well, for archiving purposes.
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
Hi Kees / all, not sure if up to me doing that (apologies if not) and
maybe fsdevel is not the proper list, but I think worth having at least
one list explicitely mentioned in MAINTAINERS in order people use that
as a pstore archive of patches. If you prefer other list, lemme know.
Cheers,
Guilherme
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 72b9654f764c..16a18125bf0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16465,6 +16465,7 @@ M: Kees Cook <[email protected]>
M: Anton Vorontsov <[email protected]>
M: Colin Cross <[email protected]>
M: Tony Luck <[email protected]>
+L: [email protected]
S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
F: Documentation/admin-guide/ramoops.rst
--
2.38.0
The pstore dump function doesn't alert at all on errors - despite
pstore is usually a last resource and if it fails users won't be
able to read the kernel log, this is not the case for server users
with serial access, for example.
So, let's at least attempt to inform such advanced users on the first
backend writing error detected during the kmsg dump - this is also
very useful for pstore debugging purposes.
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
fs/pstore/platform.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 06c2c66af332..ee50812fdd2e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
if (ret == 0 && reason == KMSG_DUMP_OOPS) {
pstore_new_entry = 1;
pstore_timer_kick();
+ } else {
+ pr_err_once("backend (%s) writing error (%d)\n",
+ psinfo->name, ret);
}
total += record.size;
--
2.38.0
On Thu, Oct 06, 2022 at 07:42:08PM -0300, Guilherme G. Piccoli wrote:
> The pstore dump function doesn't alert at all on errors - despite
> pstore is usually a last resource and if it fails users won't be
> able to read the kernel log, this is not the case for server users
> with serial access, for example.
>
> So, let's at least attempt to inform such advanced users on the first
> backend writing error detected during the kmsg dump - this is also
> very useful for pstore debugging purposes.
>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
> fs/pstore/platform.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 06c2c66af332..ee50812fdd2e 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> if (ret == 0 && reason == KMSG_DUMP_OOPS) {
> pstore_new_entry = 1;
> pstore_timer_kick();
> + } else {
> + pr_err_once("backend (%s) writing error (%d)\n",
> + psinfo->name, ret);
We're holding a spinlock here, so doing a pr_*() call isn't a great
idea. It's kind of not a great idea to try to write to the log in the
middle of a dump either, but we do attempt it at the start.
Perhaps keep a saved_ret or something and send it after the spin lock is
released?
--
Kees Cook
On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
> overall. Most of them are minors, but the implicit conversion thing
> is a bit more "relevant" in a sense it's more invasive and would fit
> more as a "fix".
>
> The code is based on v6.0, and it was tested with multiple compression
> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
>
> [...]
Applied to for-next/pstore, thanks!
[1/8] pstore: Improve error reporting in case of backend overlap
https://git.kernel.org/kees/c/55dbe25ee4c8
[2/8] pstore: Expose kmsg_bytes as a module parameter
https://git.kernel.org/kees/c/1af13c2b6324
[3/8] pstore: Inform unregistered backend names as well
https://git.kernel.org/kees/c/a4f92789f799
--
Kees Cook
For some reason, the efi-pstore backend name (exposed through the
pstore infrastructure) is hardcoded as "efi", whereas all the other
backends follow a kind of convention in using the module name.
Let's do it here as well, to make user's life easier (they might
use this info for unloading the module backend, for example).
Cc: Ard Biesheuvel <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
drivers/firmware/efi/efi-pstore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 3bddc152fcd4..97a9e84840a0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -207,7 +207,7 @@ static int efi_pstore_erase(struct pstore_record *record)
static struct pstore_info efi_pstore_info = {
.owner = THIS_MODULE,
- .name = "efi",
+ .name = KBUILD_MODNAME,
.flags = PSTORE_FLAGS_DMESG,
.open = efi_pstore_open,
.close = efi_pstore_close,
--
2.38.0
On Thu, Oct 06, 2022 at 07:42:05PM -0300, Guilherme G. Piccoli wrote:
> The pstore infrastructure supports one single backend at a time;
> trying to load a another backend causes an error and displays a
> message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
> register_pstore() error reporting").
>
> Happens that this message is not really clear about the situation,
> also the current error returned (-EPERM) isn't accurate, whereas
> -EBUSY makes more sense. We have another place in the code that
> relies in the -EBUSY return for a similar check.
>
> So, make it consistent here by returning -EBUSY and using a
> similar message in both scenarios.
>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
> fs/pstore/platform.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 0c034ea39954..c32957e4b256 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
> int pstore_register(struct pstore_info *psi)
> {
> if (backend && strcmp(backend, psi->name)) {
> - pr_warn("ignoring unexpected backend '%s'\n", psi->name);
> - return -EPERM;
> + pr_warn("backend '%s' already in use: ignoring '%s'\n",
> + backend, psi->name);
> + return -EBUSY;
Thank you! Yes, this has bothered me for a while. :)
--
Kees Cook
On Thu, Oct 06, 2022 at 11:29:02PM +0000, Luck, Tony wrote:
> > Tony, I see your recent responses, but if you'd rather not be bothered
> > by pstore stuff any more, please let me know. :)
>
> Kees,
>
> Occasionally something catches my eye ... but in general I'm not looking at
> pstore patches. You can drop me from the MAINTAINERS file.
Do you mind if I leave you? It's nice to have extra eyes on it when it
does happen! :)
--
Kees Cook
> Tony, I see your recent responses, but if you'd rather not be bothered
> by pstore stuff any more, please let me know. :)
Kees,
Occasionally something catches my eye ... but in general I'm not looking at
pstore patches. You can drop me from the MAINTAINERS file.
-Tony
On 06/10/2022 20:27, Kees Cook wrote:
> [...]
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>> if (ret == 0 && reason == KMSG_DUMP_OOPS) {
>> pstore_new_entry = 1;
>> pstore_timer_kick();
>> + } else {
>> + pr_err_once("backend (%s) writing error (%d)\n",
>> + psinfo->name, ret);
>
> We're holding a spinlock here, so doing a pr_*() call isn't a great
> idea. It's kind of not a great idea to try to write to the log in the
> middle of a dump either, but we do attempt it at the start.
>
> Perhaps keep a saved_ret or something and send it after the spin lock is
> released?
>
Hi Kees, thanks a lot for the very quick review!!
Agree with you, I'll rework this one.
Do you agree with showing only a single error? For me makes sense since
we just wanna hint advanced users (+ people-debugging-pstore heh) that
something went wrong.
Cheers,
Guilherme
On Thu, Oct 06, 2022 at 08:34:44PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:27, Kees Cook wrote:
> > [...]
> >> --- a/fs/pstore/platform.c
> >> +++ b/fs/pstore/platform.c
> >> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> >> if (ret == 0 && reason == KMSG_DUMP_OOPS) {
> >> pstore_new_entry = 1;
> >> pstore_timer_kick();
> >> + } else {
> >> + pr_err_once("backend (%s) writing error (%d)\n",
> >> + psinfo->name, ret);
> >
> > We're holding a spinlock here, so doing a pr_*() call isn't a great
> > idea. It's kind of not a great idea to try to write to the log in the
> > middle of a dump either, but we do attempt it at the start.
> >
> > Perhaps keep a saved_ret or something and send it after the spin lock is
> > released?
> >
>
> Hi Kees, thanks a lot for the very quick review!!
>
> Agree with you, I'll rework this one.
> Do you agree with showing only a single error? For me makes sense since
> we just wanna hint advanced users (+ people-debugging-pstore heh) that
> something went wrong.
Yeah, I agree -- it's going to be for folks working on pstore code. :)
--
Kees Cook
On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> Currently, this entry contains only the maintainers name. Add hereby
This likely need a general refresh, too.
Colin, you haven't sent anything pstore related since 2016. Please let
me know if you'd like to stay listed here.
Anton, same question for you (last I see is 2015).
Tony, I see your recent responses, but if you'd rather not be bothered
by pstore stuff any more, please let me know. :)
> a mailing-list as well, for archiving purposes.
>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
>
>
> Hi Kees / all, not sure if up to me doing that (apologies if not) and
> maybe fsdevel is not the proper list, but I think worth having at least
> one list explicitely mentioned in MAINTAINERS in order people use that
> as a pstore archive of patches. If you prefer other list, lemme know.
I think that's a reasonable guess! :) Thanks for reminding me about
this. I think I'd rather use [email protected], since
we've got a patchwork configured. It's not a _totally_ unreasonable
topic to have there. ;)
--
Kees Cook
On 06/10/2022 20:27, Kees Cook wrote:
> On Thu, Oct 06, 2022 at 07:42:05PM -0300, Guilherme G. Piccoli wrote:
>> The pstore infrastructure supports one single backend at a time;
>> trying to load a another backend causes an error and displays a
>> message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
>> register_pstore() error reporting").
>>
>> Happens that this message is not really clear about the situation,
>> also the current error returned (-EPERM) isn't accurate, whereas
>> -EBUSY makes more sense. We have another place in the code that
>> relies in the -EBUSY return for a similar check.
>>
>> So, make it consistent here by returning -EBUSY and using a
>> similar message in both scenarios.
>>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>> ---
>> fs/pstore/platform.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 0c034ea39954..c32957e4b256 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
>> int pstore_register(struct pstore_info *psi)
>> {
>> if (backend && strcmp(backend, psi->name)) {
>> - pr_warn("ignoring unexpected backend '%s'\n", psi->name);
>> - return -EPERM;
>> + pr_warn("backend '%s' already in use: ignoring '%s'\n",
>> + backend, psi->name);
>> + return -EBUSY;
>
> Thank you! Yes, this has bothered me for a while. :)
Heheh ditto! Thank you for the great and fast review =)
On 06/10/2022 20:22, Kees Cook wrote:
> On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
>> Currently, this entry contains only the maintainers name. Add hereby
>
> This likely need a general refresh, too.
>
> Colin, you haven't sent anything pstore related since 2016. Please let
> me know if you'd like to stay listed here.
>
> Anton, same question for you (last I see is 2015).
>
> Tony, I see your recent responses, but if you'd rather not be bothered
> by pstore stuff any more, please let me know. :)
>
Hi Kees, in case you want an extra pair of eyes to review/test pstore,
you can add me as reviewer - since we're using pstore in the Steam Deck
now and I have some improvements/fixes planned, I could help testing and
reviewing the stuff as well.
Feel free to ignore that as well if you think it's not pertinent!
Thanks,
Guilherme
On Thu, Oct 6, 2022 at 4:22 PM Kees Cook <[email protected]> wrote:
>
> On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> > Currently, this entry contains only the maintainers name. Add hereby
>
> This likely need a general refresh, too.
>
> Colin, you haven't sent anything pstore related since 2016. Please let
> me know if you'd like to stay listed here.
You can remove me.
>> Occasionally something catches my eye ... but in general I'm not looking at
>> pstore patches. You can drop me from the MAINTAINERS file.
>
> Do you mind if I leave you? It's nice to have extra eyes on it when it
> does happen! :)
That's ok too. pstore traffic isn't a significant fraction of my e-mail flow.
-Tony
On Fri, Oct 07, 2022 at 01:32:57PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:22, Kees Cook wrote:
> > On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> >> Currently, this entry contains only the maintainers name. Add hereby
> >
> > This likely need a general refresh, too.
> >
> > Colin, you haven't sent anything pstore related since 2016. Please let
> > me know if you'd like to stay listed here.
> >
> > Anton, same question for you (last I see is 2015).
> >
> > Tony, I see your recent responses, but if you'd rather not be bothered
> > by pstore stuff any more, please let me know. :)
> >
>
> Hi Kees, in case you want an extra pair of eyes to review/test pstore,
> you can add me as reviewer - since we're using pstore in the Steam Deck
> now and I have some improvements/fixes planned, I could help testing and
> reviewing the stuff as well.
Sure! That'd be lovely. :)
--
Kees Cook
On 06/10/2022 20:24, Kees Cook wrote:
> On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
>> overall. Most of them are minors, but the implicit conversion thing
>> is a bit more "relevant" in a sense it's more invasive and would fit
>> more as a "fix".
>>
>> The code is based on v6.0, and it was tested with multiple compression
>> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
>> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
>>
>> [...]
>
> Applied to for-next/pstore, thanks!
>
> [1/8] pstore: Improve error reporting in case of backend overlap
> https://git.kernel.org/kees/c/55dbe25ee4c8
> [2/8] pstore: Expose kmsg_bytes as a module parameter
> https://git.kernel.org/kees/c/1af13c2b6324
> [3/8] pstore: Inform unregistered backend names as well
> https://git.kernel.org/kees/c/a4f92789f799
>
Thanks Kees! just a heads-up on how I'll proceed.
(a) Patches 1-3 were added already.
(b) MAINTAINERS patch was reworked by yourself in the other series, so
I'll discard my version.
(c) I'll rework patches 4 and 8 and re-submit them plus patch 7
(including the ACK from Ard).
(d) Gonna discard for now patch 5, planning to test a new version on top
of the crypto acomp interface V2 from Ard/you.
Cheers,
Guilherme
On Wed, Oct 12, 2022 at 12:50:50PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:24, Kees Cook wrote:
> > On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
> >> overall. Most of them are minors, but the implicit conversion thing
> >> is a bit more "relevant" in a sense it's more invasive and would fit
> >> more as a "fix".
> >>
> >> The code is based on v6.0, and it was tested with multiple compression
> >> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
> >> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
> >>
> >> [...]
> >
> > Applied to for-next/pstore, thanks!
> >
> > [1/8] pstore: Improve error reporting in case of backend overlap
> > https://git.kernel.org/kees/c/55dbe25ee4c8
> > [2/8] pstore: Expose kmsg_bytes as a module parameter
> > https://git.kernel.org/kees/c/1af13c2b6324
> > [3/8] pstore: Inform unregistered backend names as well
> > https://git.kernel.org/kees/c/a4f92789f799
> >
>
> Thanks Kees! just a heads-up on how I'll proceed.
>
> (a) Patches 1-3 were added already.
>
> (b) MAINTAINERS patch was reworked by yourself in the other series, so
> I'll discard my version.
>
> (c) I'll rework patches 4 and 8 and re-submit them plus patch 7
> (including the ACK from Ard).
>
> (d) Gonna discard for now patch 5, planning to test a new version on top
> of the crypto acomp interface V2 from Ard/you.
Sounds good; thanks!
--
Kees Cook
On Thu, 6 Oct 2022 19:42:11 -0300, Guilherme G. Piccoli wrote:
> For some reason, the efi-pstore backend name (exposed through the
> pstore infrastructure) is hardcoded as "efi", whereas all the other
> backends follow a kind of convention in using the module name.
>
> Let's do it here as well, to make user's life easier (they might
> use this info for unloading the module backend, for example).
>
> [...]
Applied to for-next/pstore, thanks!
[7/8] efi: pstore: Follow convention for the efi-pstore backend name
https://git.kernel.org/kees/c/39bae0ee0656
--
Kees Cook