Re: [PATCH v2] tmpfs: fix race between umount and writepage

[Konstantin Khlebnikov]

Andrew Morton wrote:
> On Thu, 21 Apr 2011 10:41:50 +0400
> Konstantin Khlebnikov<khlebnikov@xxxxxxxxxx>  wrote:
>
> > shmem_writepage() call igrab() on the inode for the page which is came from
> > reclaimer to add it later into shmem_swaplist for swap-unuse operation.
> >
> > This igrab() can race with super-block deactivating process:
> >
> > shrink_inactive_list()		deactivate_super()
> > pageout()			tmpfs_fs_type->kill_sb()
> > shmem_writepage()		kill_litter_super()
> > 				generic_shutdown_super()
> > 				 evict_inodes()
> >   igrab()
> > 				  atomic_read(&inode->i_count)
> > 				   skip-inode
> >   iput()
> > 				 if (!list_empty(&sb->s_inodes))
> > 					printk("VFS: Busy inodes after...
> >
> > This igrap-iput pair was added in commit 1b1b32f2c6f6bb3253
> > based on incorrect assumptions:
> >
> > : Ah, I'd never suspected it, but shmem_writepage's swaplist manipulation
> > : is unsafe: though still hold page lock, which would hold off inode
> > : deletion if the page were i pagecache, it doesn't hold off once it's in
> > : swapcache (free_swap_and_cache doesn't wait on locked pages).  Hmm: we
> > : could put the the inode on swaplist earlier, but then shmem_unuse_inode
> > : could never prune unswapped inodes.
> >
> > Attached locked page actually protect inode from deletion because
> > truncate_inode_pages_range() will sleep on this, so igrab not required.
> > This patch actually revert last hunk from that commit.
>
> hm, is that last paragraph true?  Let's look at the resulting code.
>
>
> : 	if (swap.val&&  add_to_swap_cache(page, swap, GFP_ATOMIC) == 0) {
> : 		delete_from_page_cache(page);
>
> Here, the page is removed from inode->i_mapping.  So
> truncate_inode_pages() won't see that page and will not block on its
> lock.

Oops, right. Sorry. It produce use-after-free race, but it is quiet and small.
My test is using too few files to catch it in a reasonable time,
and I ran it without slab poisoning.

So, v1 patch is correct but little ugly, while v2 -- broken.

> : 		shmem_swp_set(info, entry, swap.val);
> : 		shmem_swp_unmap(entry);
> : 		spin_unlock(&info->lock);
> : 		if (list_empty(&info->swaplist)) {
> : 			mutex_lock(&shmem_swaplist_mutex);
> : 			/* move instead of add in case we're racing */
> : 			list_move_tail(&info->swaplist,&shmem_swaplist);
> : 			mutex_unlock(&shmem_swaplist_mutex);
> : 		}
>
> Here, the code plays with `info', which points at storage which is
> embedded within the inode's filesystem-private part.
>
> But because the inode now has no attached locked page, a concurrent
> umount can free the inode while this code is using it.

I guess we can try to put delete_from_page_cache(page); right before swap_writepage
but it move it outside info->lock...

> : 		swap_shmem_alloc(swap);
> : 		BUG_ON(page_mapped(page));
> : 		swap_writepage(page, wbc);
> : 		return 0;
> : 	}
>
> However, I assume that you reran your testcase with the v2 patch and
> that things ran OK.  How come?  Either my analysis is wrong or the
> testcase doesn't trigger races in this code path?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/