lkml.org 
[lkml]   [2011]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] tmpfs: fix race between umount and writepage
Andrew Morton wrote:
> On Thu, 21 Apr 2011 10:41:50 +0400
> Konstantin Khlebnikov<khlebnikov@openvz.org> 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?
>


\
 
 \ /
  Last update: 2011-04-22 06:09    [W:0.058 / U:1.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site