linux-kernel - Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next

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]

Message-ID: <20181026192206.GC187415@joelaf.mtv.corp.google.com>
Date:   Fri, 26 Oct 2018 12:22:06 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     linux-kernel@...r.kernel.org
Cc:     kernel-team@...roid.com, Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Kees Cook <keescook@...omium.org>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz

On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> ---
>  fs/pstore/ram.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>  }
>  
>  static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>  		     u64 *id, enum pstore_type_id *typep, bool update)
>  {
>  	struct persistent_ram_zone *prz;
>  	int i = (*c)++;
>  
>  	/* Give up if we never existed or have hit the end. */
> -	if (!przs || i >= max)
> +	if (!przs)
>  		return NULL;
>  
>  	prz = przs[i];

Ah, looks like I may have introduced an issue here since 'i' isn't checked by
the caller for the single prz case, its only checked for the multiple prz
cases, so something like below could be folded in. I still feel its better
than passing the max argument.

Another thought is, even better we could have a different function when
there's only one prz and not have to pass an array, just pass the first
element? Something like...

ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
	  		    enum pstore_type_id *typep, bool update)
And for the _single  case, we also wouldn't need to pass id so that's another
argument less.

Let me know what you think, otherwise something like the below will need to
be folded in to fix this patch... thanks.

----8<---

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 5702b692bdb9..061d2af2485b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 		}
 	}
 
-	if (!prz_ok(prz))
+	if (!prz_ok(prz) && !cxt->console_read_cnt) {
 		prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
 					   record, 0);
+	}
 
-	if (!prz_ok(prz))
+	if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
 		prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
 					   record, 0);
 
 	/* ftrace is last since it may want to dynamically allocate memory. */
 	if (!prz_ok(prz)) {
-		if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
+		if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
+		    !cxt->ftrace_read_cnt) {
 			prz = ramoops_get_next_prz(cxt->fprzs,
 					&cxt->ftrace_read_cnt, record, 0);
 		} else {