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:	Fri, 22 Oct 2010 02:56:22 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Dave Chinner <david@...morbit.com>
Cc:	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 Thu, Oct 21, 2010 at 11:49:41AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@...hat.com>
> 
> We currently protect the per-inode state flags with the inode_lock.
> Using a global lock to protect per-object state is overkill when we
> coul duse a per-inode lock to protect the state.  Use the
> inode->i_lock for this, and wrap all the state changes and checks
> with the inode->i_lock.

> @@ -1424,22 +1449,30 @@ static void iput_final(struct inode *inode)
>  	if (!drop) {
>  		if (sb->s_flags & MS_ACTIVE) {
>  			inode->i_state |= I_REFERENCED;
> -			if (!(inode->i_state & (I_DIRTY|I_SYNC)))
> +			if (!(inode->i_state & (I_DIRTY|I_SYNC)) &&
> +			    list_empty(&inode->i_lru)) {
> +				spin_unlock(&inode->i_lock);
>  				inode_lru_list_add(inode);
> +				return;

Sorry, that's broken.  Leaving aside leaking inode_lock here (you remove
taking it later anyway), this is racy.

Look: inode is hashed.  It's alive and well.  It just has no references outside
of the lists.  Right?  Now, what's to stop another CPU from
	a) looking it up in icache
	b) doing unlink()
	c) dropping the last reference
	d) freeing the sucker
... just as you are about to call inode_lru_list_add() here?

For paths in iput() where we do set I_FREEING/I_WILL_FREE it's perfectly
fine to drop all locks once that's done.  Inode is ours, nobody will pick
it and we are free to do as we wish.

For the path that leaves the inode alive and hashed - sorry, can't do.
AFAICS, unlike hash, wb and sb locks, lru lock should nest *inside*
->i_lock.  And yes, it means trylock in prune_icache(), with "put it in
the end of the list for one more spin" if we fail.  In that case it's
really cleaner that way.
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ