[PATCH] ocfs2: fix crash issue if access released lockres in debugfs

[PATCH] ocfs2: fix crash issue if access released lockres in debugfs

[Rock Li]

Access locking_state of dlm debugfs may cause crash as scene below:

Proc A:                  Proc that access debuginfo:
add_lockres_tracking(lockresA)
...
                         ocfs2_dlm_seq_next():
                           //priv->p_iter_res points to next
                           //lockres e.g. B. priv->p_tmp_res hold
                           //copy of lockres A before leave
                         ocfs2_dlm_seq_show()
...
remove_lockres_tracking(lockres B):
  //free lockres B, l_debug_list in
  //priv->p_ter_res is updated but not
  //priv->p_tmp_res
...
                         ocfs2_dlm_seq_next():
			   //priv->p_tmp_res which holds a old copy of
                           //lockres A, the l_debug_list holds a
                           //out-of-date succeed pointer, which will
                           //cause crash as //access invalid memory
                           iter = v; //priv->p_tmp_res
                           iter = ocfs2_dlm_next_res(iter, priv)

The root cause of this issue is that private->p_iter_res acts as the
agent of accessing lockres and is protected by ocfs2_dlm_tracking_lock
while p_tmp_res is only a copy of the lockres and will be out-of-dated
after leave critial region of ocfs2_dlm_tracking_lock. We should use
priv->p_ter_res as the forward iterater instead.

Signed-off-by: Rock Li <lihongweizz@inspur.com>
---
 fs/ocfs2/dlmglue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index c28bc98..5d84350 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -3109,7 +3109,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void *v, loff_t *pos)
 	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
 
 	spin_lock(&ocfs2_dlm_tracking_lock);
-	iter = ocfs2_dlm_next_res(iter, priv);
+	iter = ocfs2_dlm_next_res(dummy, priv);
 	list_del_init(&dummy->l_debug_list);
 	if (iter) {
 		list_add(&dummy->l_debug_list, &iter->l_debug_list);
-- 
1.8.3.1

Re: [PATCH] ocfs2: fix crash issue if access released lockres in debugfs

[Joseph Qi]

Hi,
Sorry for the late reply.
It seems it is indeed an issue and I'll get into it more deeply.
I'm curious about how you figure out this? Is it a real issue you've
encountered?

Thanks,
Joseph

On 9/20/22 3:36 PM, Rock Li wrote:
> Access locking_state of dlm debugfs may cause crash as scene below:
> 
> Proc A:                  Proc that access debuginfo:
> add_lockres_tracking(lockresA)
> ...
>                          ocfs2_dlm_seq_next():
>                            //priv->p_iter_res points to next
>                            //lockres e.g. B. priv->p_tmp_res hold
>                            //copy of lockres A before leave
>                          ocfs2_dlm_seq_show()
> ...
> remove_lockres_tracking(lockres B):
>   //free lockres B, l_debug_list in
>   //priv->p_ter_res is updated but not
>   //priv->p_tmp_res
> ...
>                          ocfs2_dlm_seq_next():
> 			   //priv->p_tmp_res which holds a old copy of
>                            //lockres A, the l_debug_list holds a
>                            //out-of-date succeed pointer, which will
>                            //cause crash as //access invalid memory
>                            iter = v; //priv->p_tmp_res
>                            iter = ocfs2_dlm_next_res(iter, priv)
> 
> The root cause of this issue is that private->p_iter_res acts as the
> agent of accessing lockres and is protected by ocfs2_dlm_tracking_lock
> while p_tmp_res is only a copy of the lockres and will be out-of-dated
> after leave critial region of ocfs2_dlm_tracking_lock. We should use
> priv->p_ter_res as the forward iterater instead.
> 
> Signed-off-by: Rock Li <lihongweizz@inspur.com>
> ---
>  fs/ocfs2/dlmglue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index c28bc98..5d84350 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -3109,7 +3109,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void *v, loff_t *pos)
>  	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
>  
>  	spin_lock(&ocfs2_dlm_tracking_lock);
> -	iter = ocfs2_dlm_next_res(iter, priv);
> +	iter = ocfs2_dlm_next_res(dummy, priv);
>  	list_del_init(&dummy->l_debug_list);
>  	if (iter) {
>  		list_add(&dummy->l_debug_list, &iter->l_debug_list);

Re: [PATCH] ocfs2: fix crash issue if access released lockres in debugfs

[Rock Li(李宏伟)]

Hi Joseph,

Thanks for your reply. 
In our use case, a userspace daemon tool will periodically read /sys/kernel/debug/o2dlm/<uuid>/locking_state to check the lock request state. System crashes casually after a long time running. After analyzed the vmcore file, I found the daemon tool process is accessing an invalid pointer inside the seqfile iteration when reading locking_state. 

I need to correct my patch comment slightly that adding lockresA and removing lockresB do not have to be the same process, the key point is that the lock tracking list is changed during seqfile iteration.

Br,
Rock

> Re: [PATCH] ocfs2: fix crash issue if access released lockres in debugfs
> 
> Hi,
> Sorry for the late reply.
> It seems it is indeed an issue and I'll get into it more deeply.
> I'm curious about how you figure out this? Is it a real issue you've encountered?
> 
> Thanks,
> Joseph
> 
> On 9/20/22 3:36 PM, Rock Li wrote:
> > Access locking_state of dlm debugfs may cause crash as scene below:
> >
> > Proc A:                  Proc that access debuginfo:
> > add_lockres_tracking(lockresA)
> > ...
> >                          ocfs2_dlm_seq_next():
> >                            //priv->p_iter_res points to next
> >                            //lockres e.g. B. priv->p_tmp_res hold
> >                            //copy of lockres A before leave
> >                          ocfs2_dlm_seq_show() ...
> > remove_lockres_tracking(lockres B):
> >   //free lockres B, l_debug_list in
> >   //priv->p_ter_res is updated but not
> >   //priv->p_tmp_res
> > ...
> >                          ocfs2_dlm_seq_next():
> > 			                  //priv->p_tmp_res which holds a old copy of
> >                            //lockres A, the l_debug_list holds a
> >                            //out-of-date succeed pointer, which will
> >                            //cause crash as //access invalid memory
> >                            iter = v; //priv->p_tmp_res
> >                            iter = ocfs2_dlm_next_res(iter, priv)
> >
> > The root cause of this issue is that private->p_iter_res acts as the
> > agent of accessing lockres and is protected by ocfs2_dlm_tracking_lock
> > while p_tmp_res is only a copy of the lockres and will be out-of-dated
> > after leave critial region of ocfs2_dlm_tracking_lock. We should use
> > priv->p_ter_res as the forward iterater instead.
> >
> > Signed-off-by: Rock Li <lihongweizz@inspur.com>
> > ---
> >  fs/ocfs2/dlmglue.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c index
> > c28bc98..5d84350 100644
> > --- a/fs/ocfs2/dlmglue.c
> > +++ b/fs/ocfs2/dlmglue.c
> > @@ -3109,7 +3109,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file
> *m, void *v, loff_t *pos)
> >  	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
> >
> >  	spin_lock(&ocfs2_dlm_tracking_lock);
> > -	iter = ocfs2_dlm_next_res(iter, priv);
> > +	iter = ocfs2_dlm_next_res(dummy, priv);
> >  	list_del_init(&dummy->l_debug_list);
> >  	if (iter) {
> >  		list_add(&dummy->l_debug_list, &iter->l_debug_list);