linux-kernel - Re: [PATCH] iov_iter: optimise iter type checking

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: <20210109170359.GT3579531@ZenIV.linux.org.uk>
Date:   Sat, 9 Jan 2021 17:03:59 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Pavel Begunkov <asml.silence@...il.com>
Cc:     linux-fsdevel@...r.kernel.org, Jens Axboe <axboe@...nel.dk>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iov_iter: optimise iter type checking

On Sat, Jan 09, 2021 at 04:09:08PM +0000, Pavel Begunkov wrote:
> On 06/12/2020 16:01, Pavel Begunkov wrote:
> > On 21/11/2020 14:37, Pavel Begunkov wrote:
> >> The problem here is that iov_iter_is_*() helpers check types for
> >> equality, but all iterate_* helpers do bitwise ands. This confuses
> >> compilers, so even if some cases were handled separately with
> >> iov_iter_is_*(), corresponding ifs in iterate*() right after are not
> >> eliminated.
> >>
> >> E.g. iov_iter_npages() first handles discards, but iterate_all_kinds()
> >> still checks for discard iter type and generates unreachable code down
> >> the line.
> > 
> > Ping. This one should be pretty simple
> 
> Ping please. Any doubts about this patch?

Sorry, had been buried in other crap.  I'm really not fond of the
bitmap use; if anything, I would rather turn iterate_and_advance() et.al.
into switches...

How about moving the READ/WRITE part into MSB?  Checking is just as fast
(if not faster - check for sign vs. checking bit 0).  And turn the
types into straight (dense) enum.

Almost all iov_iter_rw() callers have the form (iov_iter_rw(iter) == READ) or
(iov_iter_rw(iter) == WRITE).  Out of 50-odd callers there are 5 nominal
exceptions:
fs/cifs/smbdirect.c:1936:                        iov_iter_rw(&msg->msg_iter));
fs/exfat/inode.c:442:   int rw = iov_iter_rw(iter);
fs/f2fs/data.c:3639:    int rw = iov_iter_rw(iter);
fs/f2fs/f2fs.h:4082:    int rw = iov_iter_rw(iter);
fs/f2fs/f2fs.h:4092:    int rw = iov_iter_rw(iter);

The first one is debugging printk
        if (iov_iter_rw(&msg->msg_iter) == WRITE) {
                /* It's a bug in upper layer to get there */
                cifs_dbg(VFS, "Invalid msg iter dir %u\n",
                         iov_iter_rw(&msg->msg_iter));
                rc = -EINVAL;
                goto out;
        }
and if you look at the condition, the quality of message is
underwhelming - "Data source msg iter passed by caller" would
be more informative.

Other 4...  exfat one is
        if (rw == WRITE) {
...
	}
...
        if (ret < 0 && (rw & WRITE))
                exfat_write_failed(mapping, size);
IOW, doing
	bool is_write = iov_iter_rw(iter) == WRITE;
would be cleaner.  f2fs.h ones are
	int rw = iov_iter_rw(iter);
	....
	if (.... && rw == WRITE ...
so they are of the same sort (assuming we want that local
variable in the first place).

f2fs/data.c is the least trivial - it includes things like
                if (!down_read_trylock(&fi->i_gc_rwsem[rw])) {
and considering the amount of other stuff done there,
I would suggest something like
	int rw = is_data_source(iter) ? WRITE : READ;

I'll dig myself from under ->d_revalidate() code review, look
through the iov_iter-related series and post review, hopefully
by tonight.