linux-kernel - Re: [PATCH 16/21] fs: Protect inode->i_state with the inode->i

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]

Message-ID: <20101024021815.GA4820@amd>
Date:	Sun, 24 Oct 2010 13:18:15 +1100
From:	Nick Piggin <npiggin@...nel.dk>
To:	Dave Chinner <david@...morbit.com>
Cc:	Al Viro <viro@...IV.linux.org.uk>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 16/21] fs: Protect inode->i_state with the inode->i_lock

On Fri, Oct 22, 2010 at 02:14:31PM +1100, Dave Chinner wrote:
> AFAICT, moving the inode_lru_lock inside i_lock doesn't affect the
> locking order of anything else, and I agree that putting a single
> trylock in the prune_icache loop is definitely cleaner than anything
> else I've been able to think of that retains the current locking
> order. It will also remove the wart in sync_inode().
> 
> So, I'll swallow my rhetoric and agree with you that inode_lru_lock
> inside the i_lock is the natural and easiest way to nest these
> locks. I'll rework the series to do this. 

I don't know what's so unclean or wrong about the locking order.  I'm
still leaning much further than Al as to putting locks inside i_lock.

With inode-rcu, we _always_ arrive at the inode _first_, before taking
_any_ other locks (because we either come in via an external reference,
or we can find the inode from any of the data structures using RCU
rather than taking their locks).

[The exception is hash insertion uniqueness enforcement, because we have
 no inode to lock, by definition. But in that case we're OK because the
 new inode we're about to insert has no concurrency on its i_lock so that
 can be initialised locked.]

So what we have is an inode, which we need to do stuff to. That stuff
involves moving it on or off data structures, and updating its refcount
and state, etc.

That suggests the i_lock -> data-structure-lock locking order.

And look at the flexibility -- I could implement that without changing
any other code or ordering in the icache. Sure you can reorder things
around to cope with different locking strategies, but I'm not actually
given a good reason _why_ i_lock first is not the "natural" ordering.
Doing that means we simply don't _need_ to change any ordering or
concurrency rules (although it doesn't prevent changes).

--
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/