Hi,

On 31/01/2019 10:55, Ross Lagerwall wrote:
Each gfs2_bufdata stores a reference to a glock but the reference count
isn't incremented. This causes an occasional use-after-free of the
glock. Fix by taking a reference on the glock during allocation and
dropping it when freeing.

Another good bit of debugging. It would be nice if we can (longer term) avoid using the ref count though, since that will have some overhead, but for the time being, the correctness is the important thing,

Steve.


Found by KASAN:

BUG: KASAN: use-after-free in revoke_lo_after_commit+0x8e/0xe0 [gfs2]
Write of size 4 at addr ffff88801aff6134 by task kworker/0:2H/20371

CPU: 0 PID: 20371 Comm: kworker/0:2H Tainted: G O 4.19.0+0 #1
Hardware name: Dell Inc. PowerEdge R805/0D456H, BIOS 4.2.1 04/14/2010
Workqueue: glock_workqueue glock_work_func [gfs2]
Call Trace:
  dump_stack+0x71/0xab
  print_address_description+0x6a/0x270
  kasan_report+0x258/0x380
  ? revoke_lo_after_commit+0x8e/0xe0 [gfs2]
  revoke_lo_after_commit+0x8e/0xe0 [gfs2]
  gfs2_log_flush+0x511/0xa70 [gfs2]
  ? gfs2_log_shutdown+0x1f0/0x1f0 [gfs2]
  ? __brelse+0x48/0x50
  ? gfs2_log_commit+0x4de/0x6e0 [gfs2]
  ? gfs2_trans_end+0x18d/0x340 [gfs2]
  gfs2_ail_empty_gl+0x1ab/0x1c0 [gfs2]
  ? inode_go_dump+0xe0/0xe0 [gfs2]
  ? inode_go_sync+0xe4/0x220 [gfs2]
  inode_go_sync+0xe4/0x220 [gfs2]
  do_xmote+0x12b/0x290 [gfs2]
  glock_work_func+0x6f/0x160 [gfs2]
  process_one_work+0x461/0x790
  worker_thread+0x69/0x6b0
  ? process_one_work+0x790/0x790
  kthread+0x1ae/0x1d0
  ? kthread_create_worker_on_cpu+0xc0/0xc0
  ret_from_fork+0x22/0x40

Allocated by task 20805:
  kasan_kmalloc+0xa0/0xd0
  kmem_cache_alloc+0xb5/0x1b0
  gfs2_glock_get+0x14b/0x620 [gfs2]
  gfs2_inode_lookup+0x20c/0x640 [gfs2]
  gfs2_dir_search+0x150/0x180 [gfs2]
  gfs2_lookupi+0x272/0x360 [gfs2]
  __gfs2_lookup+0x8b/0x1d0 [gfs2]
  gfs2_atomic_open+0x77/0x100 [gfs2]
  path_openat+0x1454/0x1c10
  do_filp_open+0x124/0x1d0
  do_sys_open+0x213/0x2c0
  do_syscall_64+0x69/0x160
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Freed by task 0:
  __kasan_slab_free+0x130/0x180
  kmem_cache_free+0x78/0x1e0
  rcu_process_callbacks+0x2ad/0x6c0
  __do_softirq+0x111/0x38c

The buggy address belongs to the object at ffff88801aff6040
  which belongs to the cache gfs2_glock(aspace) of size 560
The buggy address is located 244 bytes inside of
  560-byte region [ffff88801aff6040, ffff88801aff6270)
...

Signed-off-by: Ross Lagerwall <ross.lagerw...@citrix.com>
---
  fs/gfs2/aops.c    | 3 +--
  fs/gfs2/lops.c    | 2 +-
  fs/gfs2/meta_io.c | 2 +-
  fs/gfs2/trans.c   | 9 ++++++++-
  fs/gfs2/trans.h   | 2 ++
  5 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 05dd78f4b2b3..8c2b572a7fb1 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -868,9 +868,8 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
                        gfs2_assert_warn(sdp, bd->bd_bh == bh);
                        if (!list_empty(&bd->bd_list))
                                list_del_init(&bd->bd_list);
-                       bd->bd_bh = NULL;
                        bh->b_private = NULL;
-                       kmem_cache_free(gfs2_bufdata_cachep, bd);
+                       gfs2_free_bufdata(bd);
                }
bh = bh->b_this_page;
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 94dcab655bc0..f40be71677d1 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -847,7 +847,7 @@ static void revoke_lo_after_commit(struct gfs2_sbd *sdp, 
struct gfs2_trans *tr)
                gl = bd->bd_gl;
                atomic_dec(&gl->gl_revokes);
                clear_bit(GLF_LFLUSH, &gl->gl_flags);
-               kmem_cache_free(gfs2_bufdata_cachep, bd);
+               gfs2_free_bufdata(bd);
        }
  }
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index be9c0bf697fe..868caa0eb104 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -355,7 +355,7 @@ void gfs2_remove_from_journal(struct buffer_head *bh, int 
meta)
                        gfs2_trans_add_revoke(sdp, bd);
                } else if (was_pinned) {
                        bh->b_private = NULL;
-                       kmem_cache_free(gfs2_bufdata_cachep, bd);
+                       gfs2_free_bufdata(bd);
                }
                spin_unlock(&sdp->sd_ail_lock);
        }
diff --git a/fs/gfs2/trans.c b/fs/gfs2/trans.c
index cd9a94a6b5bb..423cbee8fa08 100644
--- a/fs/gfs2/trans.c
+++ b/fs/gfs2/trans.c
@@ -133,9 +133,16 @@ static struct gfs2_bufdata *gfs2_alloc_bufdata(struct 
gfs2_glock *gl,
        bd->bd_gl = gl;
        INIT_LIST_HEAD(&bd->bd_list);
        bh->b_private = bd;
+       gfs2_glock_hold(gl);
        return bd;
  }
+void gfs2_free_bufdata(struct gfs2_bufdata *bd)
+{
+       gfs2_glock_put(bd->bd_gl);
+       kmem_cache_free(gfs2_bufdata_cachep, bd);
+}
+
  /**
   * gfs2_trans_add_data - Add a databuf to the transaction.
   * @gl: The inode glock associated with the buffer
@@ -265,7 +272,7 @@ void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 
blkno, unsigned int len)
                        list_del_init(&bd->bd_list);
                        gfs2_assert_withdraw(sdp, sdp->sd_log_num_revoke);
                        sdp->sd_log_num_revoke--;
-                       kmem_cache_free(gfs2_bufdata_cachep, bd);
+                       gfs2_free_bufdata(bd);
                        tr->tr_num_revoke_rm++;
                        if (--n == 0)
                                break;
diff --git a/fs/gfs2/trans.h b/fs/gfs2/trans.h
index ad70087d0597..276ddca7bbe9 100644
--- a/fs/gfs2/trans.h
+++ b/fs/gfs2/trans.h
@@ -46,4 +46,6 @@ extern void gfs2_trans_add_meta(struct gfs2_glock *gl, struct 
buffer_head *bh);
  extern void gfs2_trans_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata 
*bd);
  extern void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 blkno, unsigned 
int len);
+void gfs2_free_bufdata(struct gfs2_bufdata *bd);
+
  #endif /* __TRANS_DOT_H__ */

Reply via email to