Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221013210648.137452-1-gpiccoli@igalia.com> <20221013210648.137452-2-gpiccoli@igalia.com>
In-Reply-To: <20221013210648.137452-2-gpiccoli@igalia.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 14 Oct 2022 16:46:58 +0200
Message-ID: <CAMj1kXGe++psjOmmztkNY2KeGAGD3DD8F_spG0F37+4vu6dwBw@mail.gmail.com>
Subject: Re: [PATCH V2 1/3] pstore: Alert on backend write error
To:     "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc:     linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-efi@vger.kernel.org,
        kernel-dev@igalia.com, kernel@gpiccoli.net, keescook@chromium.org,
        anton@enomsg.org, ccross@android.com, tony.luck@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 13 Oct 2022 at 23:11, Guilherme G. Piccoli <gpiccoli@igalia.com> 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 <gpiccoli@igalia.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>
>
> V2:
> - Show error message late, outside of the critical region
> (thanks Kees for the idea!).
>
>
>  fs/pstore/platform.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 06c2c66af332..cbc0b468c1ab 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -393,6 +393,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>         const char      *why;
>         unsigned int    part = 1;
>         unsigned long   flags = 0;
> +       int             saved_ret = 0;
>         int             ret;
>
>         why = kmsg_dump_reason_str(reason);
> @@ -463,12 +464,21 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>                 if (ret == 0 && reason == KMSG_DUMP_OOPS) {
>                         pstore_new_entry = 1;
>                         pstore_timer_kick();
> +               } else {
> +                       /* Preserve only the first non-zero returned value. */
> +                       if (!saved_ret)
> +                               saved_ret = ret;
>                 }
>
>                 total += record.size;
>                 part++;
>         }
>         spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> +
> +       if (saved_ret) {
> +               pr_err_once("backend (%s) writing error (%d)\n", psinfo->name,
> +                           saved_ret);
> +       }
>  }
>
>  static struct kmsg_dumper pstore_dumper = {
> --
> 2.38.0
>