linux-ext4 - Re: [PATCH] ext4: avoid lockdep warning when inheriting encryption context

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Nov 2016 15:18:13 +0100
From:   Jan Kara <jack@...e.cz>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Eric Biggers <ebiggers@...gle.com>, linux-ext4@...r.kernel.org,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Jaegeuk Kim <jaegeuk@...nel.org>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH] ext4: avoid lockdep warning when inheriting encryption
 context

On Wed 16-11-16 10:47:38, Ted Tso wrote:
> On Mon, Nov 14, 2016 at 01:03:36PM -0800, Eric Biggers wrote:
> > If the task actually were to wait for the journal to commit in this
> > case, then it would deadlock because a handle remains open from
> > __ext4_new_inode(), so the running transaction can't be committed yet.
> > Fortunately, __jbd2_journal_force_commit() avoids the deadlock by not
> > allowing the running transaction to be committed while the current task
> > has it open.  However, the above lockdep warning is still triggered.
> 
> So this is a false positive introduced by
> 
>    1eaa566d368b: jbd2: track more dependencies on transaction commit
> 
> Instead of working around the problem here, perhaps it would be better
> to fix __jbd2_journal_force_commit() so that it calls a newly created
> __jbd2_log_wait_commit() which skips the jbd2_might_wait_for_commit()
> (and then have jbd2_log_wait_commit call __jbd2_log_wait_commit with
> the might_wait_for_commit check)?
> 
> This isn't the only place where jbd2_journal_force_commit() is called
> so if the problem is with the lockdep check, maybe we should just fix
> the logic in the jbd2 layer, hmm?

So it is a false positive in the sense that it is not a deadlock
possibility. OTOH it is a valid report in the sense that the code does not
do what it is intended to - i.e., force a transaction commit to free some
blocks and retry block allocation. So I find the original fix actually
worthwhile.

Looking into ext4 codebase there are only 3 callsites of
jbd2_journal_force_commit_nested() - all in some kind of retry loops and
two of the call sites actually should not need the "nested" variant. Just
ext4_should_retry_alloc() may be called when a transaction is held open and
then the caller likely wanted something else anyway so it would be good to
find these cases and fix them and then get rid of the "nested" variant of
forcing a commit since that is easy to use in a wrong way.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html