lkml.org 
[lkml]   [2018]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mm: Fix races between address_space dereference and free in page_evicatable
On Mon, Feb 26, 2018 at 02:38:04PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > Hi Jan,
> >
> > On Mon, Feb 19, 2018 at 11:57:35AM +0100, Jan Kara wrote:
> >> Hi Minchan,
> >>
> >> On Sun 18-02-18 18:22:45, Minchan Kim wrote:
> >> > On Mon, Feb 12, 2018 at 04:12:27PM +0800, Huang, Ying wrote:
> >> > > From: Huang Ying <ying.huang@intel.com>
> >> > >
> >> > > When page_mapping() is called and the mapping is dereferenced in
> >> > > page_evicatable() through shrink_active_list(), it is possible for the
> >> > > inode to be truncated and the embedded address space to be freed at
> >> > > the same time. This may lead to the following race.
> >> > >
> >> > > CPU1 CPU2
> >> > >
> >> > > truncate(inode) shrink_active_list()
> >> > > ... page_evictable(page)
> >> > > truncate_inode_page(mapping, page);
> >> > > delete_from_page_cache(page)
> >> > > spin_lock_irqsave(&mapping->tree_lock, flags);
> >> > > __delete_from_page_cache(page, NULL)
> >> > > page_cache_tree_delete(..)
> >> > > ... mapping = page_mapping(page);
> >> > > page->mapping = NULL;
> >> > > ...
> >> > > spin_unlock_irqrestore(&mapping->tree_lock, flags);
> >> > > page_cache_free_page(mapping, page)
> >> > > put_page(page)
> >> > > if (put_page_testzero(page)) -> false
> >> > > - inode now has no pages and can be freed including embedded address_space
> >> > >
> >> > > mapping_unevictable(mapping)
> >> > > test_bit(AS_UNEVICTABLE, &mapping->flags);
> >> > > - we've dereferenced mapping which is potentially already free.
> >> > >
> >> > > Similar race exists between swap cache freeing and page_evicatable() too.
> >> > >
> >> > > The address_space in inode and swap cache will be freed after a RCU
> >> > > grace period. So the races are fixed via enclosing the page_mapping()
> >> > > and address_space usage in rcu_read_lock/unlock(). Some comments are
> >> > > added in code to make it clear what is protected by the RCU read lock.
> >> >
> >> > Is it always true for every FSes, even upcoming FSes?
> >> > IOW, do we have any strict rule FS folks must use RCU(i.e., call_rcu)
> >> > to destroy inode?
> >> >
> >> > Let's cc linux-fs.
> >>
> >> That's actually a good question. Pathname lookup relies on inodes being
> >> protected by RCU so "normal" filesystems definitely need to use RCU freeing
> >> of inodes. OTOH a filesystem could in theory refuse any attempt for RCU
> >> pathname walk (in its .d_revalidate/.d_compare callback) and then get away
> >> with freeing its inodes normally AFAICT. I don't see that happening
> >> anywhere in the tree but in theory it is possible with some effort... But
> >> frankly I don't see a good reason for that so all we should do is to
> >> document that .destroy_inode needs to free the inode structure through RCU
> >> if it uses page cache? Al?
> >
> > Yub, it would be much better. However, how does this patch fix the problem?
> > Although it can make only page_evictable safe, we could go with the page
> > further and finally uses page->mapping, again.
> > For instance,
> >
> > shrink_active_list
> > page_evictable();
> > ..
> > page_referened()
> > page_rmapping
> > page->mapping
>
> This only checks the value of page->mapping, not deference
> page->mapping. So it should be safe.

Oops, you're right. I got confused. However, I want to make the lock
consistent(i.e., use page_lock to protect address_space) but cannot
come with better way.

Sorry for the noise, Huang.

>
> Best Regards,
> Huang, Ying
>
> > I think caller should lock the page to protect entire operation, which
> > have been used more widely to pin a address_space.
> >
> > Thanks.

\
 
 \ /
  Last update: 2018-02-26 08:39    [W:0.031 / U:2.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site