linux-ext4 - Re: [v2] Updated locking documentation for transaction

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:   Sun, 7 Apr 2019 12:52:06 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Alexander Lochmann <alexander.lochmann@...dortmund.de>
Cc:     Horst Schirmeier <horst.schirmeier@...dortmund.de>,
        Jan Kara <jack@...e.com>, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v2] Updated locking documentation for transaction_t

On Mon, Mar 18, 2019 at 07:42:37PM +0100, Alexander Lochmann wrote:
>  	/*t
> -	 * Where in the log does this transaction's commit start? [no locking]
> +	 * Where in the log does this transaction's commit start?
> +	 * [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_log_start;

Well, technically, that's not quite right.  It's only assigned in one
location, and we hold j_state_lock, yes.  But that's because we need
to access journal->j_head.  At the point where we set t_log_start, the
transaction has already been locked down (transaction->t_state >
T_LOCKED).

Similarly, we happen to be holding j_state where it is currently being
accessed, but it's not because we needed the lock in order to access
t_log_start safely.

>  	/*
> -	 * When transaction started
> +	 * When transaction started [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_start;

And again, not really.  The primary place where t_start is set is when
the transaction is firstt created, before it's visible anywhere else.
after that, it is used exclusively by the commit thread, and so no
locking is necessary.  It's true that in the places where it is used,
j_state_lock happens to be taken, but it's strictly not necessary.

>  
>  	/*
> -	 * When commit was requested
> +	 * When commit was requested [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_requested;

Yes, that appears to be correct.

>  
>  	/*
> -	 * Checkpointing stats [j_checkpoint_sem]
> +	 * Checkpointing stats [journal_t.j_list_lock]
>  	 */
>  	struct transaction_chp_stats_s t_chp_stats;
>

This appears to be correct.

						- Ted