linux-kernel - Re: [PATCH v2 5/7] zram: support idle/huge page writeback

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: <20181126094737.sfudpeoigra2vir7@gmail.com>
Date:   Sun, 25 Nov 2018 23:47:37 -1000
From:   Joey Pabalinas <joeypabalinas@...il.com>
To:     Minchan Kim <minchan@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Joey Pabalinas <joeypabalinas@...il.com>
Subject: Re: [PATCH v2 5/7] zram: support idle/huge page writeback

On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> +	strlcpy(mode_buf, buf, sizeof(mode_buf));
> +	/* ignore trailing newline */
> +	sz = strlen(mode_buf);

One possible idea would be to use strscpy() instead and directly assign
the return value to sz, avoiding an extra strlen() call (though you would
have to check if `sz == -E2BIG` and do `sz = sizeof(mode_buf) - 1` in that
case).

> +	if (!strcmp(mode_buf, "idle"))
> +		mode = IDLE_WRITEBACK;
> +	if (!strcmp(mode_buf, "huge"))
> +		mode = HUGE_WRITEBACK;

Maybe using `else if (!strcmp(mode_buf, "huge"))` would be slightly
better here, avoiding a second strcmp() if mode_buf has already
matched "idle".

> +		if ((mode & IDLE_WRITEBACK &&
> +			  !zram_test_flag(zram, index, ZRAM_IDLE)) &&
> +		    (mode & HUGE_WRITEBACK &&
> +			  !zram_test_flag(zram, index, ZRAM_HUGE)))
> +			goto next;

Wouldn't writing this as `mode & (IDLE_WRITEBACK | HUGE_WRITEBACK)`
be a bit easier to read as well as slightly more compact?

> +	ret = len;
> +	 __free_page(page);
> +release_init_lock:
> +	up_read(&zram->init_lock);
> +	return ret;

Hm, I noticed that this function either returns an error or just the passed
in len on success, and I'm left wondering if there might be other useful
information which could be passed back to the caller instead. I can't
immediately think of any such information, though, so it's possible I'm
just daydreaming :)

-- 
Cheers,
Joey Pabalinas

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)