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;
Date:   Fri, 2 Nov 2018 11:32:04 -0700
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     linux-kernel@vger.kernel.org, Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH 7/8] pstore: Remove needless lock during console writes
Message-ID: <20181102183204.GC14942@google.com>
References: <20181101235200.28584-1-keescook@chromium.org>
 <20181101235200.28584-8-keescook@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181101235200.28584-8-keescook@chromium.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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>

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

thanks,

- Joel

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/platform.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index a956c7bc3f67..32340e7dd6a5 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -461,31 +461,14 @@ static void pstore_unregister_kmsg(void)
>  #ifdef CONFIG_PSTORE_CONSOLE
>  static void pstore_console_write(struct console *con, const char *s, unsigned c)
>  {
> -	const char *e = s + c;
> +	struct pstore_record record;
>  
> -	while (s < e) {
> -		struct pstore_record record;
> -		unsigned long flags;
> -
> -		pstore_record_init(&record, psinfo);
> -		record.type = PSTORE_TYPE_CONSOLE;
> -
> -		if (c > psinfo->bufsize)
> -			c = psinfo->bufsize;
> +	pstore_record_init(&record, psinfo);
> +	record.type = PSTORE_TYPE_CONSOLE;
>  
> -		if (oops_in_progress) {
> -			if (!spin_trylock_irqsave(&psinfo->buf_lock, flags))
> -				break;
> -		} else {
> -			spin_lock_irqsave(&psinfo->buf_lock, flags);
> -		}
> -		record.buf = (char *)s;
> -		record.size = c;
> -		psinfo->write(&record);
> -		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> -		s += c;
> -		c = e - s;
> -	}
> +	record.buf = (char *)s;
> +	record.size = c;
> +	psinfo->write(&record);
>  }
>  
>  static struct console pstore_console = {
> -- 
> 2.17.1
>