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: <20181127025332.23wlp4zai6e7mrip@gmail.com>
Date:   Mon, 26 Nov 2018 16:53:32 -1000
From:   Joey Pabalinas <joeypabalinas@...il.com>
To:     Minchan Kim <minchan@...nel.org>
Cc:     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 Tue, Nov 27, 2018 at 11:13:27AM +0900, Minchan Kim wrote:
> 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?

-E2BIG return from strscpy() just means the string was longer than would
fit in the length passed as the third argument. This means only
`sizeof(mode_buf) - 1` bytes were copied into the dest, with the last
byte being the \0 terminator.

You can find the function source in lib/string.c.

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

Well, with the write system call you can have partial writes, in which
case it would return the (short) number of bytes written. But in this
function, even if there was some sort of partial write possible, this
function still only every returns len or error.

Neither of these are that important though, so Ack from me with or
without these two suggestions.

Reviewed-by: Joey Pabalinas <joeypabalinas@...il.com>

-- 
Cheers,
Joey Pabalinas

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