linux-kernel - Re: The bug of iput() removal from flusher thread?

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:	Wed, 21 Nov 2012 19:22:19 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Jan Kara <jack@...e.cz>,
	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>,
	Al Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: The bug of iput() removal from flusher thread?

On Wed, Nov 21, 2012 at 12:05:33AM -0800, Andrew Morton wrote:
> On Wed, 21 Nov 2012 02:48:51 +0100 Jan Kara <jack@...e.cz> wrote:
> 
> > +/*
> > + * Add inode to LRU if needed (inode is unused and clean).
> > + *
> > + * Needs inode->i_lock held.
> > + */
> > +void inode_add_lru(struct inode *inode)
> > +{
> > +	if (!(inode->i_state & (I_DIRTY | I_FREEING | I_SYNC)) &&
> > +	    !atomic_read(&inode->i_count) && inode->i_sb->s_flags & MS_ACTIVE)
> > +		inode_lru_list_add(inode);
> > +}
> 
> Is i_lock sufficient to stabilise i_count?
> 
> <looks at the code a bit>
> 
> Is evict_inodes() wrong to test i_count outside i_lock?
> 
> invalidate_inodes() looks better.
> 
> can_unuse() must be called under i_lock, and is.  Apparently this
> requirement was sufficiently obvious to not meed documenting.

It is documented. can_unuse looks at i_state and i_count, and both
are documented as requiring the i_lock at the top of the file in
the locking rules section. Also, see __iget(), also mentioned in
the locking rules....

> prune_icache_sb() gets it right.
> 
> iput() gets it right.
> 
> So to answer my own question: yes, it is sufficient.  But a) the
> comment for inode.i_lock is out of date

If you means the one in fs.h, then yeah, it's way out of date....
>
> and b) evict_inodes() looks
> fishy.

As I understand it, evict_inodes() is special - it's only called
from generic_shutdown_super() after the MS_ACTIVE flag has been
removed from the filesytem, the dcache has been pruned and all the
inodes cleaned. So there should be no new references to the inodes
occurring, and hence we don't need to hold the lock to serialise
against new references being taken....

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/