From 59a1cc6bdabf5ed148b48808ad1a418d87f5e6bf Mon Sep 17 00:00:00 2001 From: Steven Whitehouse Date: Fri, 4 Aug 2006 15:41:22 -0400 Subject: [PATCH] [GFS2] Fix lock ordering bug in page fault path Mmapped files were able to trigger a lock ordering bug. Private maps do not need to take the glock so early on. Shared maps do unfortunately, however we can get around that by adding a flag into the flags for the struct gfs2_file. This only works because we are taking an exclusive lock at this point, so we know that nobody else can be racing with us. Fixes Red Hat bugzilla: #201196 Signed-off-by: Steven Whitehouse --- fs/gfs2/incore.h | 1 + fs/gfs2/log.c | 4 ++-- fs/gfs2/ops_address.c | 23 +++++++++++++++++++---- fs/gfs2/ops_vm.c | 20 +++++++------------- fs/gfs2/recovery.c | 9 +++------ 5 files changed, 32 insertions(+), 25 deletions(-) diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index 90e0624d8065..e98c14f30daa 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -279,6 +279,7 @@ static inline struct gfs2_sbd *GFS2_SB(struct inode *inode) enum { GFF_DID_DIRECT_ALLOC = 0, + GFF_EXLOCK = 1, }; struct gfs2_file { diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c index 60fdc94ccc8a..a591fb8fae20 100644 --- a/fs/gfs2/log.c +++ b/fs/gfs2/log.c @@ -256,8 +256,8 @@ static unsigned int current_tail(struct gfs2_sbd *sdp) if (list_empty(&sdp->sd_ail1_list)) tail = sdp->sd_log_head; else { - ai = list_entry(sdp->sd_ail1_list.prev, - struct gfs2_ail, ai_list); + ai = list_entry(sdp->sd_ail1_list.prev, struct gfs2_ail, + ai_list); tail = ai->ai_first; } diff --git a/fs/gfs2/ops_address.c b/fs/gfs2/ops_address.c index fca69f12e4de..bdd4d6b48721 100644 --- a/fs/gfs2/ops_address.c +++ b/fs/gfs2/ops_address.c @@ -237,14 +237,22 @@ static int gfs2_readpage(struct file *file, struct page *page) struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host); struct gfs2_holder gh; int error; + int do_unlock = 0; if (likely(file != &gfs2_internal_file_sentinal)) { + if (file) { + struct gfs2_file *gf = file->private_data; + if (test_bit(GFF_EXLOCK, &gf->f_flags)) + goto skip_lock; + } gfs2_holder_init(ip->i_gl, LM_ST_SHARED, GL_ATIME|GL_AOP, &gh); + do_unlock = 1; error = gfs2_glock_nq_m_atime(1, &gh); if (unlikely(error)) goto out_unlock; } +skip_lock: if (gfs2_is_stuffed(ip)) { error = stuffed_readpage(ip, page); unlock_page(page); @@ -262,7 +270,7 @@ static int gfs2_readpage(struct file *file, struct page *page) return error; out_unlock: unlock_page(page); - if (file != &gfs2_internal_file_sentinal) + if (do_unlock) gfs2_holder_uninit(&gh); goto out; } @@ -291,17 +299,24 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping, struct gfs2_holder gh; unsigned page_idx; int ret; + int do_unlock = 0; if (likely(file != &gfs2_internal_file_sentinal)) { + if (file) { + struct gfs2_file *gf = file->private_data; + if (test_bit(GFF_EXLOCK, &gf->f_flags)) + goto skip_lock; + } gfs2_holder_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_TRY_1CB|GL_ATIME|GL_AOP, &gh); + do_unlock = 1; ret = gfs2_glock_nq_m_atime(1, &gh); if (ret == GLR_TRYFAILED) goto out_noerror; if (unlikely(ret)) goto out_unlock; } - +skip_lock: if (gfs2_is_stuffed(ip)) { struct pagevec lru_pvec; pagevec_init(&lru_pvec, 0); @@ -326,7 +341,7 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping, ret = mpage_readpages(mapping, pages, nr_pages, gfs2_get_block); } - if (likely(file != &gfs2_internal_file_sentinal)) { + if (do_unlock) { gfs2_glock_dq_m(1, &gh); gfs2_holder_uninit(&gh); } @@ -344,7 +359,7 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping, unlock_page(page); page_cache_release(page); } - if (likely(file != &gfs2_internal_file_sentinal)) + if (do_unlock) gfs2_holder_uninit(&gh); goto out; } diff --git a/fs/gfs2/ops_vm.c b/fs/gfs2/ops_vm.c index aff66373b7e1..875a769444a1 100644 --- a/fs/gfs2/ops_vm.c +++ b/fs/gfs2/ops_vm.c @@ -46,13 +46,7 @@ static struct page *gfs2_private_nopage(struct vm_area_struct *area, unsigned long address, int *type) { struct gfs2_inode *ip = GFS2_I(area->vm_file->f_mapping->host); - struct gfs2_holder i_gh; struct page *result; - int error; - - error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &i_gh); - if (error) - return NULL; set_bit(GIF_PAGED, &ip->i_flags); @@ -61,8 +55,6 @@ static struct page *gfs2_private_nopage(struct vm_area_struct *area, if (result && result != NOPAGE_OOM) pfault_be_greedy(ip); - gfs2_glock_dq_uninit(&i_gh); - return result; } @@ -141,7 +133,9 @@ static int alloc_page_backing(struct gfs2_inode *ip, struct page *page) static struct page *gfs2_sharewrite_nopage(struct vm_area_struct *area, unsigned long address, int *type) { - struct gfs2_inode *ip = GFS2_I(area->vm_file->f_mapping->host); + struct file *file = area->vm_file; + struct gfs2_file *gf = file->private_data; + struct gfs2_inode *ip = GFS2_I(file->f_mapping->host); struct gfs2_holder i_gh; struct page *result = NULL; unsigned long index = ((address - area->vm_start) >> PAGE_CACHE_SHIFT) + @@ -156,13 +150,14 @@ static struct page *gfs2_sharewrite_nopage(struct vm_area_struct *area, set_bit(GIF_PAGED, &ip->i_flags); set_bit(GIF_SW_PAGED, &ip->i_flags); - error = gfs2_write_alloc_required(ip, - (uint64_t)index << PAGE_CACHE_SHIFT, + error = gfs2_write_alloc_required(ip, (u64)index << PAGE_CACHE_SHIFT, PAGE_CACHE_SIZE, &alloc_required); if (error) goto out; + set_bit(GFF_EXLOCK, &gf->f_flags); result = filemap_nopage(area, address, type); + clear_bit(GFF_EXLOCK, &gf->f_flags); if (!result || result == NOPAGE_OOM) goto out; @@ -177,8 +172,7 @@ static struct page *gfs2_sharewrite_nopage(struct vm_area_struct *area, } pfault_be_greedy(ip); - - out: +out: gfs2_glock_dq_uninit(&i_gh); return result; diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c index 7aabc03e4abd..bbd44a4b1a1f 100644 --- a/fs/gfs2/recovery.c +++ b/fs/gfs2/recovery.c @@ -153,8 +153,7 @@ static int get_log_header(struct gfs2_jdesc *jd, unsigned int blk, if (lh.lh_header.mh_magic != GFS2_MAGIC || lh.lh_header.mh_type != GFS2_METATYPE_LH || - lh.lh_blkno != blk || - lh.lh_hash != hash) + lh.lh_blkno != blk || lh.lh_hash != hash) return 1; *head = lh; @@ -482,11 +481,9 @@ int gfs2_recover_journal(struct gfs2_jdesc *jd) /* Acquire a shared hold on the transaction lock */ - error = gfs2_glock_nq_init(sdp->sd_trans_gl, - LM_ST_SHARED, + error = gfs2_glock_nq_init(sdp->sd_trans_gl, LM_ST_SHARED, LM_FLAG_NOEXP | LM_FLAG_PRIORITY | - GL_NOCANCEL | GL_NOCACHE, - &t_gh); + GL_NOCANCEL | GL_NOCACHE, &t_gh); if (error) goto fail_gunlock_ji;