* [PATCH] GFS2: Fix unsafe dereference in dump_holder()
@ 2013-12-24 11:51 Tetsuo Handa
2014-01-02 10:08 ` Steven Whitehouse
0 siblings, 1 reply; 4+ messages in thread
From: Tetsuo Handa @ 2013-12-24 11:51 UTC (permalink / raw)
To: swhiteho, rpeterso; +Cc: linux-kernel
>From efee681e4ca6152d9549a50e93ca932b00f03014 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 24 Dec 2013 18:04:43 +0900
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>
---
fs/gfs2/glock.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index c8420f7..7bda965 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1654,15 +1654,22 @@ static int dump_holder(struct seq_file *seq, const struct gfs2_holder *gh)
{
struct task_struct *gh_owner = NULL;
char flags_buf[32];
+ char name[TASK_COMM_LEN];
if (gh->gh_owner_pid)
- gh_owner = pid_task(gh->gh_owner_pid, PIDTYPE_PID);
+ gh_owner = get_pid_task(gh->gh_owner_pid, PIDTYPE_PID);
+ if (gh_owner) {
+ get_task_comm(name, gh_owner);
+ put_task_struct(gh_owner);
+ } else {
+ strlcpy(name, "(ended)", sizeof(name));
+ }
gfs2_print_dbg(seq, " H: s:%s f:%s e:%d p:%ld [%s] %pS\n",
state2str(gh->gh_state),
hflags2str(flags_buf, gh->gh_flags, gh->gh_iflags),
gh->gh_error,
gh->gh_owner_pid ? (long)pid_nr(gh->gh_owner_pid) : -1,
- gh_owner ? gh_owner->comm : "(ended)",
+ name,
(void *)gh->gh_ip);
return 0;
}
--
1.7.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] GFS2: Fix unsafe dereference in dump_holder()
2013-12-24 11:51 [PATCH] GFS2: Fix unsafe dereference in dump_holder() Tetsuo Handa
@ 2014-01-02 10:08 ` Steven Whitehouse
2014-01-02 10:52 ` Tetsuo Handa
0 siblings, 1 reply; 4+ messages in thread
From: Steven Whitehouse @ 2014-01-02 10:08 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: rpeterso, linux-kernel
Hi,
On Tue, 2013-12-24 at 20:51 +0900, Tetsuo Handa wrote:
> >From efee681e4ca6152d9549a50e93ca932b00f03014 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 24 Dec 2013 18:04:43 +0900
> 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,
Steve.
> fs/gfs2/glock.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index c8420f7..7bda965 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -1654,15 +1654,22 @@ static int dump_holder(struct seq_file *seq, const struct gfs2_holder *gh)
> {
> struct task_struct *gh_owner = NULL;
> char flags_buf[32];
> + char name[TASK_COMM_LEN];
>
> if (gh->gh_owner_pid)
> - gh_owner = pid_task(gh->gh_owner_pid, PIDTYPE_PID);
> + gh_owner = get_pid_task(gh->gh_owner_pid, PIDTYPE_PID);
> + if (gh_owner) {
> + get_task_comm(name, gh_owner);
> + put_task_struct(gh_owner);
> + } else {
> + strlcpy(name, "(ended)", sizeof(name));
> + }
> gfs2_print_dbg(seq, " H: s:%s f:%s e:%d p:%ld [%s] %pS\n",
> state2str(gh->gh_state),
> hflags2str(flags_buf, gh->gh_flags, gh->gh_iflags),
> gh->gh_error,
> gh->gh_owner_pid ? (long)pid_nr(gh->gh_owner_pid) : -1,
> - gh_owner ? gh_owner->comm : "(ended)",
> + name,
> (void *)gh->gh_ip);
> return 0;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] GFS2: Fix unsafe dereference in dump_holder()
2014-01-02 10:08 ` Steven Whitehouse
@ 2014-01-02 10:52 ` Tetsuo Handa
2014-01-02 12:21 ` Steven Whitehouse
0 siblings, 1 reply; 4+ messages in thread
From: Tetsuo Handa @ 2014-01-02 10:52 UTC (permalink / raw)
To: swhiteho; +Cc: rpeterso, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] GFS2: Fix unsafe dereference in dump_holder()
2014-01-02 10:52 ` Tetsuo Handa
@ 2014-01-02 12:21 ` Steven Whitehouse
0 siblings, 0 replies; 4+ messages in thread
From: Steven Whitehouse @ 2014-01-02 12:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: rpeterso, linux-kernel
Hi,
Thanks - that looks good. I've applied it to the GFS2 -nmw and -fixes
trees,
Steve.
On Thu, 2014-01-02 at 19:52 +0900, Tetsuo Handa wrote:
> 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;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-02 12:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-24 11:51 [PATCH] GFS2: Fix unsafe dereference in dump_holder() Tetsuo Handa
2014-01-02 10:08 ` Steven Whitehouse
2014-01-02 10:52 ` Tetsuo Handa
2014-01-02 12:21 ` Steven Whitehouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome