[PATCH] jbd: journal_head unmapping race fix

Fix a race identified by Chris Mason <mason@suse.com>

journal_unmap_buffer -> __dispose_buffers has the j_list_lock and the
jbd_lock_bh_state held.

journal_get_write_access calls journal_put_journal_head, which takes
jbd_lock_bh_journal_head(bh) and doesn't seem to have any other locks held.

Since journal_unmap_buffers trusts the buffer_jbd bit to see if we need to
call __dispose_buffer, and nobody seems to test buffer_jbd after taking
jbd_lock_bh_journal_head.  The kernel dereferences a null jh pointer in
__journal_remove_journal_head.

The patch fixes this by using journal_grab_journal_head() in
journal_unmap_buffer().  It ensures that we either grab and pin the
journal_head if the bh has one, or we bale out if the bh doesn't have a
journal_head.

Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
index 8f69595f9f0a246845f081134753d980ccee37e1..18a678ce2591a96cd97c04fb0e5b454e80e718e4 100644 (file)
--- a/fs/jbd/transaction.c
+++ b/fs/jbd/transaction.c
@@ -1773,14 +1773,10 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh)
        jbd_lock_bh_state(bh);
        spin_lock(&journal->j_list_lock);
 
-       /*
-        * Now we have the locks, check again to see whether kjournald has
-        * taken the buffer off the transaction.
-        */
-       if (!buffer_jbd(bh))
-               goto zap_buffer;
+       jh = journal_grab_journal_head(bh);
+       if (!jh)
+               goto zap_buffer_no_jh;
 
-       jh = bh2jh(bh);
        transaction = jh->b_transaction;
        if (transaction == NULL) {
                /* First case: not on any transaction.  If it
@@ -1811,6 +1807,7 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh)
                        spin_unlock(&journal->j_list_lock);
                        jbd_unlock_bh_state(bh);
                        spin_unlock(&journal->j_state_lock);
+                       journal_put_journal_head(jh);
                        return ret;
                } else {
                        /* There is no currently-running transaction. So the
@@ -1824,6 +1821,7 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh)
                                spin_unlock(&journal->j_list_lock);
                                jbd_unlock_bh_state(bh);
                                spin_unlock(&journal->j_state_lock);
+                               journal_put_journal_head(jh);
                                return ret;
                        } else {
                                /* The orphan record's transaction has
@@ -1847,6 +1845,7 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh)
                spin_unlock(&journal->j_list_lock);
                jbd_unlock_bh_state(bh);
                spin_unlock(&journal->j_state_lock);
+               journal_put_journal_head(jh);
                return 0;
        } else {
                /* Good, the buffer belongs to the running transaction.
@@ -1860,6 +1859,8 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh)
        }
 
 zap_buffer:
+       journal_put_journal_head(jh);
+zap_buffer_no_jh:
        spin_unlock(&journal->j_list_lock);
        jbd_unlock_bh_state(bh);
        spin_unlock(&journal->j_state_lock);