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: <20181127021327.GD237537@google.com>
Date:   Tue, 27 Nov 2018 11:13:27 +0900
From:   Minchan Kim <minchan@...nel.org>
To:     Joey Pabalinas <joeypabalinas@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH v2 5/7] zram: support idle/huge page writeback

On Sun, Nov 25, 2018 at 11:47:37PM -1000, Joey Pabalinas wrote:
> 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).

Thanks for the suggstion.
If I limit destination buffer smaller, I couldn't meet -E2BIG?

> 
> > +	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".

I considered "huge|idle" as an option. Anyway, in that case, mode should
"mode |= ". At this moment, yes, lets use "else if" since I don't have
strong opinion to support "idle|huge".

> 
> > +		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 :)

It is write syscall semantic of sysfs so not sure it's doable to pass
other value to user.

> 
> -- 
> Cheers,
> Joey Pabalinas