linux-ext4 - Re: [PATCH 03/22] ext4: Do not iput inode under running transaction in ext4

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: <20191024120958.GC1124@mit.edu>
Date:   Thu, 24 Oct 2019 08:09:58 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-ext4@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 03/22] ext4: Do not iput inode under running transaction
 in ext4_mkdir()

On Thu, Oct 24, 2019 at 12:19:06PM +0200, Jan Kara wrote:
> Correct on both points. Thanks for spotting this! Now I still don't think
> that calling iput() with running transaction is good. It complicates
> matters with revoke record reservation but it is also fragile for other
> reasons - e.g. flush worker could find the allocated inode just before we
> will call iput() on it, try to write it out, block on starting transaction
> and we get a deadlock with inode_wait_for_writeback() inside evict(). Now
> inode *probably* won't be dirty yet by the time we get to ext4_add_nondir()
> or similar, that's why I say above it's just fragile, not an outright bug.

But we don't ever write the inode itself via
inode_wait_for_writeback(), because how ext4 journalling works.  (See
the comments before ext4_mark_inode_dirty()).  And for the special
inodes (directories, device nodes, etc.) there's no data dirtyness to
worry about.  For regular files, we hit this code path when have just
created the inode, but were not able to add a link to the parent
directory; the fd wasn't been released to userspace yet, so it can't
be data dirty either.

So unless I'm missing something, I don't think the deadlock described
above is possible?

We can certainly add it to the orphan list if it's necessary, but it's
extra overhead and adds a global contention point.  So if it's not
necessary, I'd rather avoid it if possible, and I think it's safe to
do so in this case.

						- Ted