linux-kernel - Re: [PATCH 7/8] pstore: Remove needless lock during console writes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Nov 2018 11:32:04 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH 7/8] pstore: Remove needless lock during console writes

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@...lfernandes.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@...omium.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
>