lkml.org 
[lkml]   [2007]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: PREEMPT_RCU breaks anon_vma locking ?
    On Sat, Feb 24, 2007 at 12:23:03AM +0300, Oleg Nesterov wrote:
    > If my understanding correct, vmscan can find a page which lives in a already
    > anon_vma_unlink'ed vma. This is ok, the page is pinned, and page->mapping is
    > not cleared until free_hot_cold_page().
    >
    > So page_lock_anon_vma() works correctly due to SLAB_DESTROY_BY_RCU even if
    > anon_vma_unlink() has already freed anon_vma. In that case we should see
    > list_empty(&anon_vma->head), we are safe.
    >
    > However, we are doing spin_unlock(anon_vma->lock) after page_lock_anon_vma(),
    > and this looks unsafe to me because page_lock_anon_vma() does rcu_read_unlock()
    > on return.

    This would indeed be bad when using CONFIG_PREEMPT_RCU! Good catch!!!

    > This worked before because spin_lock() implied rcu_read_lock(), so rcu was
    > blocked if page_lock_anon_vma() returns !NULL. With CONFIG_PREEMPT_RCU this
    > is not true (yes?), so it is possible that the slab returns the memory to
    > the system and it is re-used when we write to anon_vma->lock.
    >
    > IOW, don't we need something like this
    >
    > static struct anon_vma *page_lock_anon_vma(struct page *page)
    > {
    > struct anon_vma *anon_vma;
    > unsigned long anon_mapping;
    >
    > rcu_read_lock();
    > anon_mapping = (unsigned long) page->mapping;
    > if (!(anon_mapping & PAGE_MAPPING_ANON))
    > goto out;
    > if (!page_mapped(page))
    > goto out;
    >
    > anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
    > spin_lock(&anon_vma->lock);
    > return anon_vma;
    >
    > out:
    > rcu_read_unlock();
    > return NULL;
    > }
    >
    > static inline void page_lock_anon_vma(struct anon_vma *anon_vma)
    > {
    > spin_unlock(&anon_vma->lock);
    > rcu_read_unlock();
    > }
    > ?

    This look like a valid fix to me, at least as long as the lock is never
    dropped in the meantime (e.g., to do I/O). If the lock -is- dropped in
    the meantime, then presumably whatever is done to keep the page from
    vanishing should allow an rcu_read_unlock() to be placed after each
    spin_unlock(&...->lock) and an rcu_read_lock() to be placed before each
    spin_lock(&...->lock).

    Thanx, Paul
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-02-23 23:45    [W:0.028 / U:62.228 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site