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: <93388ab0-abbd-a680-66e0-e1ba77981479@gmail.com>
Date:   Sat, 9 Jan 2021 21:19:21 +0000
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
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 09/01/2021 17:03, Al Viro wrote:
> 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.

Didn't realise that approach before, sounds good. Most of it will be
replaced with sign jcc, and the rest will be (t >> 31) or movcc, so it
should not be of concern.

type_mask = 255;
iov_iter_type(i) { return i->type & ~type_mask; }

I hope this stuff won't add much, because the original patch completely
optimises this "&" out. I guess it'll turn into extra

xor m m
notb8 m
and m & type

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

Great, thanks Al. Without it being optimised right my other patches keep
worsening iov_iter, and I obviously want to avoid that.

-- 
Pavel Begunkov