linux-kernel - Re: [GIT PULL] file locking and writeback error handling patches for v4.13

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:   Wed, 5 Jul 2017 11:10:54 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jeff Layton <jlayton@...hat.com>
Cc:     Kees Cook <keescook@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [GIT PULL] file locking and writeback error handling patches for v4.13

On Mon, Jul 3, 2017 at 6:58 AM, Jeff Layton <jlayton@...hat.com> wrote:
>
> File locking and writeback error handling patches for v4.13

Yeah, I won't be pulling this.

It's a random collection of patches, and the ones I looked at looked
actively wrong.

For example, you add support for remote pid in one commit, and then in
the next commit you convert filesystems to use them.

Which means that in between, locking presumably doesn't work ata ll,
because the locks_translate_pid() function presumably now does the
wrong thing. It used to be conditional of fl_nspid, which hid that
issue, but that now goes away, and you rely on the sign of the pid
instead, but the sign hasn't been *changed* yet.

Now, it's entirely possible that I'm mis-reading all this, but it
means that I find even the file locking changes suspicious.

But that's not what kills this pull request for me.

No, what makes me go "No" is that it then mixes up the file locking
changes with completely unrelated mapping things.

And some of those look like obvious fixes that I have nothing at all against.

And then some just look stupid (but aren't):

-  return ret;
+  err = filemap_check_errors(inode->i_mapping);
+  return ret ? ret : err;

Yeah, why is it pointlessly calling filemap_check_errors() even when
the value won't get used? It turns out that that's because
filemap_check_errors() also *clears* the pending errors, but that
isn't at all obvious from the code, so it definitely merits a comment
somewhere).

Related to that very fact, in other parts "filemap_check_errors()" is
literally used to clear the errors, and a comment *is* added there.
Which makes me think that maybe that function should just be renamed.

But.. Then you introduce the writeback error code which is big and new
and doesn't look wrong, but wasn't really described much in your pull
request, and just makes me go "this is all a surprise, and none of it
has anythign to do with file locking".

So together, all of these issues spell "no, I'm not happy to pull
this" to me. Maybe the code is all fine for various reasons, maybe I'm
mis-reading the non-bisectable semantic change to pid's, maybe it's
all good.

But even if the code is all good, I *really* want this as separate
pull requests that make sense on their own. One for file locking that
was unrelated to everything else, one for the trivial fixes, and one
for the whole new writeback error handling logic.

                           Linus