Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20181102183204.GC14942@google.com>
References: <20181101235200.28584-1-keescook@chromium.org> <20181101235200.28584-8-keescook@chromium.org>
 <20181102183204.GC14942@google.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Fri, 2 Nov 2018 13:40:06 -0700
Message-ID: <CAGXu5jKNjipB2yWf_7pE+m29Ma8KUOXotz2L=Pc7=wBZC3AGdA@mail.gmail.com>
Subject: Re: [PATCH 7/8] pstore: Remove needless lock during console writes
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 2, 2018 at 11:32 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Thu, Nov 01, 2018 at 04:51:59PM -0700, Kees Cook wrote:
>> Since commit 70ad35db3321 ("pstore: Convert console write to use
>> ->write_buf"), the console writer does not use the preallocated crash
>> dump buffer any more, so there is no reason to perform locking around it.
>
> Out of curiosity, what was the reason for having this preallocated crash
> buffer in the first place? I thought the 'console' type only did regular
> kernel console logging, not crash dumps.

The primary reason is that the dumper needs to write to somewhere and
we don't know the state of the system (memory allocation may not work
for example).

The other frontends tend to run at "sane" locations in the kernel. The
dumper, however, is quite fragile.

> I looked at all the patches and had some minor nits, with the nits addressed
> (if you agree with them), feel free to add my Reviewed-by on future respins:
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

> Also I wonder if Namhyung is still poking around that virtio pstore driver he
> mentioned in the commit mentioned above. :)

Did that never land? I thought it mostly had to happen at the qemu end?

With nvdimm emulation, we can just use ramoops. :)

-Kees

-- 
Kees Cook