Race condition between iget_locked() and evict_inodes()

Race condition between iget_locked() and evict_inodes()

[Anton Altaparmakov]

Hi Al,

I think there is a race condition between iget_locked() and evict_inodes().

evict_inodes() checks i_count and if zero proceeds to take i_lock then set I_FREEING and eventually disposes of the inode.

But a concurrent iget_locked() takes i_lock and then increments i_count.

Thus if the events happen in this order:

evict_inodes()				iget_locked() in find_inode_fast()
atomic_read(&inode->i_count) -> 0	take i_lock
wait on i_lock				__iget(inode); -> i_count now 1
set I_FREEING				drop i_lock
evict()					return inode to caller

The inode is now gone due to the evict() call whilst it is happily being used with an elevated i_count by the iget_locked() calling process.  It seems to me like evict_inodes() should be checking i_count inside i_lock either as the only check or it should at least re-check it.

Please tell me what I am missing here.  I assume there must be something providing exclusion and I am just too blind to see it but I thought it worth bringing to your attention in case it really is simply broken.

Best regards,

	Anton
-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: Race condition between iget_locked() and evict_inodes()

[Al Viro]

On Thu, Sep 29, 2016 at 11:53:21AM +0000, Anton Altaparmakov wrote:
> Thus if the events happen in this order:
> 
> evict_inodes()				iget_locked() in find_inode_fast()

... you are buggered, because somebody is trying to grab a reference
to inode on a filesystem that is being shut down.  Look at evict_inode()
caller...

Re: Race condition between iget_locked() and evict_inodes()

[Anton Altaparmakov]

Hi Al,

> On 29 Sep 2016, at 13:17, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> On Thu, Sep 29, 2016 at 11:53:21AM +0000, Anton Altaparmakov wrote:
>> Thus if the events happen in this order:
>> 
>> evict_inodes()				iget_locked() in find_inode_fast()
> 
> ... you are buggered, because somebody is trying to grab a reference
> to inode on a filesystem that is being shut down.  Look at evict_inode()
> caller...

But what if that somebody is simply the file system being shutdown trying to flush some dirty metadata to disk which is stored in a file and thus accessed via an inode and thus iget on the inode is needed?  Surely that is allowed even during shutdown.  Once the write is complete iput() is called which then immediately evicts the inode as MS_ACTIVE is clear...

Best regards,

	Anton
-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: Race condition between iget_locked() and evict_inodes()

[Al Viro]

On Thu, Sep 29, 2016 at 12:56:49PM +0000, Anton Altaparmakov wrote:
> Hi Al,
> 
> > On 29 Sep 2016, at 13:17, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > 
> > On Thu, Sep 29, 2016 at 11:53:21AM +0000, Anton Altaparmakov wrote:
> >> Thus if the events happen in this order:
> >> 
> >> evict_inodes()				iget_locked() in find_inode_fast()
> > 
> > ... you are buggered, because somebody is trying to grab a reference
> > to inode on a filesystem that is being shut down.  Look at evict_inode()
> > caller...
> 
> But what if that somebody is simply the file system being shutdown trying to flush some dirty metadata to disk which is stored in a file and thus accessed via an inode and thus iget on the inode is needed?  Surely that is allowed even during shutdown.  Once the write is complete iput() is called which then immediately evicts the inode as MS_ACTIVE is clear...

If it's a per-superblock inode, just keep it referenced until ->put_super()
and be done with that.  Besides, the caller has just done sync_filesystem()
there, so any dirty metadata would better be already flushed.