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  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:	Tue, 3 Jan 2012 16:32:45 +0100
From:	Jan Kara <jack@...e.cz>
To:	Toshiyuki Okajima <toshi.okajima@...fujitsu.com>
Cc:	tytso@....edu, adilger.kernel@...ger.ca,
	Yongqiang Yang <xiaoqiangnk@...il.com>,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/2] jbd2: jbd2_journal_stop needs an exclusive control
 to synchronize around t_update operations

  Hello,

On Thu 22-12-11 20:56:50, Toshiyuki Okajima wrote:
> The following statements need an exclusive control for the critical code section
> around t_update operations:
> [jbd2_journal_stop()]
> 1445         /*
> 1446          * Once we drop t_updates, if it goes to zero the transaction
> 1447          * could start committing on us and eventually disappear.  So
> 1448          * once we do this, we must not dereference transaction
> 1449          * pointer again.
> 1450          */
> 1451         tid = transaction->t_tid;
> +    read_lock(&journal->j_state_lock);
> ----- critical code section ------------------------------------------------
> 1452         if (atomic_dec_and_test(&transaction->t_updates)) {
> 1453                 wake_up(&journal->j_wait_updates);
> 1454                 if (journal->j_barrier_count)
> 1455                         wake_up(&journal->j_wait_transaction_locked);
> 1456         }
> -----------------------------------------------------------------------------
> +    read_unlock(&journal->j_state_lock);
> 1457 
> 
> Because the functions which have the other critical code sections around t_update 
> operations, 
>  - jbd2_journal_commit_transaction
>  - start_this_handle
>  - jbd2_journal_lock_updates
> can not synchronize with jbd2_journal_stop.
> 
> ex) jbd2_journal_lock_updates
>  505 void jbd2_journal_lock_updates(journal_t *journal)
>  506 {
>  507         DEFINE_WAIT(wait);
>  508 
>  509         write_lock(&journal->j_state_lock);
>  510         ++journal->j_barrier_count;
>  511 
>  512         /* Wait until there are no running updates */
>  513         while (1) {
>  514                transaction_t *transaction = journal->j_running_transaction;
>  515 
>  516                 if (!transaction)
>  517                         break;
>  518 
>  519                 spin_lock(&transaction->t_handle_lock);
> ----- critical code section ------------------------------------------------
>  520                 if (!atomic_read(&transaction->t_updates)) {
>  521                         spin_unlock(&transaction->t_handle_lock);
>  522                         break;
>  523                 }
>  524                 prepare_to_wait(&journal->j_wait_updates, &wait,
>  525                                 TASK_UNINTERRUPTIBLE);
> -----------------------------------------------------------------------------
>  526                 spin_unlock(&transaction->t_handle_lock);
>  527                 write_unlock(&journal->j_state_lock);
>  528                 schedule();
>  529                 finish_wait(&journal->j_wait_updates, &wait);
>  530                 write_lock(&journal->j_state_lock);
>  531         }
>  532         write_unlock(&journal->j_state_lock);
> 
> Thefore, the following steps causes a hang-up of process1:
> 1) (process1) line 520 in jbd2_journal_lock_updates
>       transaction->t_updates is equal to 1, and then goto 4).
> 2) (process2) line 1452 in jbd2_journal_stop
>       transaction->t_updates becomes to 0, and then goto 3).
> 3) (process2) line 1453 in jbd2_journal_stop
>       wake_up(&journal->j_wait_updates) tries to wake someone up.
> 4) (process1) line 524 in jbd2_journal_lock_updates
>       prepare to sleep itself, and then goto 5).
> 5) (process1) line 528 in jbd2_journal_lock_updates
>       sleep forever because process2 doesn't wake it up anymore.
  Thanks for the analysis. Actually, you fix adds unnecessary overhead.
The problem really is the wrong ordering of prepare_to_wait() and t_updates
check. So attached patch should fix the issue as well without introducing
the overhead.

> Similar problem also exists for j_barrier_count operations but it can be
> fixed, too:
> [jbd2_journal_lock_updates]
>  505 void jbd2_journal_lock_updates(journal_t *journal)
>  506 {
>  507         DEFINE_WAIT(wait);
>  508 
>  509         write_lock(&journal->j_state_lock);
> ----------------------------------------------------------------------------
>  510         ++journal->j_barrier_count;
> ----------------------------------------------------------------------------
> ...
>  532         write_unlock(&journal->j_state_lock);
> 
> [jbd2_journal_stop]
> 1445         /*
> 1446          * Once we drop t_updates, if it goes to zero the transaction
> 1447          * could start committing on us and eventually disappear.  So
> 1448          * once we do this, we must not dereference transaction
> 1449          * pointer again.
> 1450          */
> 1451         tid = transaction->t_tid;
> +    read_lock(&journal->j_state_lock);
> 1452         if (atomic_dec_and_test(&transaction->t_updates)) {
> 1453                 wake_up(&journal->j_wait_updates);
> ----------------------------------------------------------------------------
> 1454                 if (journal->j_barrier_count)
> 1455                         wake_up(&journal->j_wait_transaction_locked);
> ----------------------------------------------------------------------------
> 1456         }
> +    read_unlock(&journal->j_state_lock);
> 1457 
  Here I don't agree. We use wait_event() to wait for j_barrier_count to
drop to zero and wait_event() has proper ordering of prepare_to_wait() and
test.

								Honza
--
Jan Kara <jack@...e.cz>
SUSE Labs, CR

View attachment "0001-jbd2-Fix-hung-processes-in-jbd2_journal_lock_updates.patch" of type "text/x-patch" (2347 bytes)

Powered by blists - more mailing lists