linux-kernel - Re: [GIT PULL] configfs updates for 5.9

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: <CAHk-=wg1h_XfPbXvisfAUsXU-WiOeCJDUibhxZGu=x9w-VKB0A@mail.gmail.com>
Date:   Tue, 4 Aug 2020 17:27:17 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Joel Becker <jlbec@...lplan.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] configfs updates for 5.9

On Mon, Aug 3, 2020 at 7:07 AM Christoph Hellwig <hch@...radead.org> wrote:
>
>  - propagate binary attribute errors (Lenny Szubowicz)

I pulled this, but I then unpulled again.

The commit is completely confused and wrong.

In particular, this part:

+       /* Only commit the data if no more shared refs to file */
+       if (file_count(file) > 1)
+               return 0;

is bogus and prone to races, meaning that if there are multiple
openers, *none* of them flush.

You can have two threads that close() at (roughly) the same time, both
call ->flush(), and both of those see "file_count()" being 2 and never
do anything.

They then both call "fput()" and are done, and that's when the file
count gets decremented.

The fact is, "flush()" is called for each close(), and you should
flush any pending data. Checking for some file_count() thing is bogus
and completely wrong.

So make up your mind: either you flush synchronously at *every*
close() (->flush) and you can get error notification.

Or you flush at the last close (->release), but then you absolutely
cannot get any error reporting, because the last put of the file may
be long after the last close (because other things than just
open/close can increase the refcount).

You can't try to do some middle ground, because it _will_ be buggy.
You have the above two options, not some racy third one.

             Linus