linux-kernel - Re: [PATCH 4/8] xfs: Move ilock before transaction start in xfs_setattr

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:	Tue, 24 Jan 2012 17:59:45 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Jan Kara <jack@...e.cz>
Cc:	linux-fsdevel@...r.kernel.org, Eric Sandeen <sandeen@...deen.net>,
	Dave Chinner <dchinner@...hat.com>,
	Surbhi Palande <csurbhi@...il.com>,
	Kamal Mostafa <kamal@...onical.com>,
	Christoph Hellwig <hch@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>, xfs@....sgi.com,
	linux-ext4@...r.kernel.org, Ben Myers <bpm@....com>,
	Alex Elder <elder@...nel.org>
Subject: Re: [PATCH 4/8] xfs: Move ilock before transaction start in
 xfs_setattr_size()

On Fri, Jan 20, 2012 at 09:34:42PM +0100, Jan Kara wrote:
> In xfs we first take ilock and start transaction afterwards.

The correct order is to allocate the transaction, reserve the space
for it and then take the ilock.  We cannot hold the ilock over the
transaction reservation because that can deadlock the journal.

That is, to make space for the new transaction reservation, we may
need to take the ilock to flush the inode and allow the journal tail
to move forwards to make space for the new transaction.  If we
already hold the ilock, then it can't be flushed, we can't make
space available in the journal and hence deadlock.

Maybe you confused the ilock vs the iolock.  We can hold the iolock
over the trans alloc/reserve because that lock is not required to
move the tail of the journal, so the deadlock doesn't exist.

> We should obey
> this order in all places because otherwise we can create the following deadlock
> with filesystem freezing: One process holds ilock and blocks on s_frozen ==
> SB_FREEZE_TRANS in xfs_trans_alloc(), another process has a transaction started
> (thus blocking freezing) and blocks on ilock. So we have to take ilock earlier
> in xfs_setattr_size().

Where are we taking the ilock and then calling xfs_trans_alloc()?
That's the caller needs to be fixed, not the 40-odd that do the
right thing by taking the ilock -after- the trans alloc/reserve
calls.

Cheers,

Dave.

-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/