lkml.org 
[lkml]   [2014]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] GFS2: Fix unsafe dereference in dump_holder()
From
Date
Steven Whitehouse wrote:
> > Subject: [PATCH] GFS2: Fix unsafe dereference in dump_holder()
> >
> > Since we are not in RCU nor tasklist_lock held, we need to grab a reference on
> > the returned task_struct. Use get_pid_task()/get_task_comm() to safely read the
> > target task's comm name.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > ---
>
> Well this is not entirely true... for 99.9% of calls to this function we
> do have the rcu read lock held (when called from the seq file code). So
> the only issue is when called from the GLOCK_BUG_ON() macro.
>
> Since it appears that rcu_read_lock() will nest, then we could
> presumably just add that around dump_holder or even around
> gfs2_dump_glock ?
>
> I think that is a neater solution than taking extra ref counts and
> copying the string around,

OK. I didn't check what lock protects this dereference. I just noticed this
dereference when I was checking direct ->comm access which might fail to return
consistent comm name. I might propose changing ->comm access to RCU in the
future ( https://lkml.org/lkml/2013/12/25/21 ).

For now add only rcu_read_lock()/rcu_read_unlock(). Patch follows.
----------
>From 63f41a384636be1acd1575ee9f9e0de317cbc131 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 2 Jan 2014 19:41:23 +0900
Subject: [PATCH v2] GFS2: Fix unsafe dereference in dump_holder()

GLOCK_BUG_ON() might call this function without RCU read lock. Make sure that
RCU read lock is held when using task_struct returned from pid_task().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/gfs2/glock.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index c8420f7..6f7a47c 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1655,6 +1655,7 @@ static int dump_holder(struct seq_file *seq, const struct gfs2_holder *gh)
struct task_struct *gh_owner = NULL;
char flags_buf[32];

+ rcu_read_lock();
if (gh->gh_owner_pid)
gh_owner = pid_task(gh->gh_owner_pid, PIDTYPE_PID);
gfs2_print_dbg(seq, " H: s:%s f:%s e:%d p:%ld [%s] %pS\n",
@@ -1664,6 +1665,7 @@ static int dump_holder(struct seq_file *seq, const struct gfs2_holder *gh)
gh->gh_owner_pid ? (long)pid_nr(gh->gh_owner_pid) : -1,
gh_owner ? gh_owner->comm : "(ended)",
(void *)gh->gh_ip);
+ rcu_read_unlock();
return 0;
}

--
1.7.1

\
 
 \ /
  Last update: 2014-01-02 12:01    [W:0.125 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site