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: <20200805082845.GA25876@infradead.org>
Date:   Wed, 5 Aug 2020 09:28:45 +0100
From:   Christoph Hellwig <hch@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Kai M??kisara <Kai.Makisara@...umbus.fi>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Joel Becker <jlbec@...lplan.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lenny Szubowicz <lszubowi@...hat.com>
Subject: Re: [GIT PULL] configfs updates for 5.9

On Tue, Aug 04, 2020 at 05:59:19PM -0700, Linus Torvalds wrote:
> So again, doing that
> 
>         if (file_count(file) > 1)
> 
> kind of check is very very wrong even outside of the fundamental race
> with two close() calls at the same time.
> 
> It is a very dangerous pattern, because it likely works in practice
> during testing, and looks like it might work.
> 
> But it is completely and unfixably wrong.
> 
> Again, the only reliable way to do that "last close" is "->release()",
> but you will never get errors from that, since (for all the same
> reasons) it might not even be done by a close. The last releaser of a
> file descriptor might be that mmap/io_uring/proc case now releasing
> the no longer used file, possibly long after the last "close()" call
> has happened.
> 
> One acceptable half-way measure *may* be
> 
>  - do the flush with the above bogus test at ->flush() time, knowing
> it might never trigger at all
> 
>  - do the flush *again* at ->release() time, in case it didn't trigger
> 
>  - add a big comment to the flush-time case to show you understand the
> issue, and understand
> 
> but I'd discourage it because of how unreliable it is.

Yes, but this is a fundamental problem with the commit on close
model that the configfs binary attributes implement, and we have to
work around that somehow.  The current behavior is to entirely ignore
errors, which of course has major issues.  I guess the alternative might
be to just commit on every ->flush as the use case (and yes I know
people could abuse it) for the configfs files is a set of simple
write syscalls.  Then again the above might be better than that.

Lenny, can you look ingo that?