linux-kernel - Re: [git pull] vfs fix

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]

Date:   Sat, 26 Nov 2016 17:48:54 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [git pull] vfs fix

On Sat, Nov 26, 2016 at 5:13 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Al Viro (1):
>       fix default_file_splice_read()

Ugh. I absolutely _hate_ this:

        BUG_ON(dummy);

because it makes no sense.

I'm assuming that "dummy" here is "start_offset", and that you want to
make sure that there are no initial offsets that would affect the
nrpages calculation.

But dammit, if so, just *call* it "start_offset", not "dummy". A dummy
value is just a place-holder, it makes no sense to have BUG_ON() on
such a value.

So adding random BUG_ON() statements is evil to begin with, but when
you do it on something that is mis-named and makes no sense, that's
just wrong.

I'm further assuming that the reason we can do that is because
"iov_iter_pipe()" has set iov_offset to zero, and as a result we end
up havin g

  iov_iter_get_pages_alloc() ->
    pipe_get_pages_alloc() ->
      data_start() will set *offp to zero.

but quite frankly, you can not tell that from the code itself, which
makes no sense. You have to go digging.

I was hoping the splice code would become more readable, not filled
with more crazy nonsensical code.

So I've pulled this, but _please_:

 - rename "dummy" (which isn't dummy at all now that you *do* things
to it!) to something sane.

   Like perhaps 'pg_offset' or 'iter_offset' or something.

 - Does the "BUG_ON()" really make sense? If the issue is that you
didn't use the offset in calculations, maybe you should just do so, ie
instead of

        BUG_ON(dummy);
        nr_pages = DIV_ROUND_UP(res, PAGE_SIZE);

   just do

        nr_pages = DIV_ROUND_UP(res + iter_offset, PAGE_SIZE);

or something? Even if "iter_offset" ends up always being zero, why is
that worthy of a BUG_ON()? The BUG_ON() is more expensive than just
doing the natural math..

That's what all the other users do, and that's what should be the
"right usage pattern", afaik. The number of pages really *is*
calculated as

       int n = DIV_ROUND_UP(result + offs, PAGE_SIZE);

in other iov_iter_get_pages_alloc() callers, although tghe nfs code
open-codes it as

        npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;

so it's not a very strong pattern.

                Linus