Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list

[Joseph Qi via Ocfs2-devel <ocfs2-devel@oss.oracle.com>]

On 3/7/22 10:51 PM, Dan Carpenter via Ocfs2-devel wrote:
> Hello OCFS Devs,
> 
> There is a big push to re-write list_for_each_entry() so that you
> can't use the list iterator outside the loop.  I wrote a check for that
> but this code has me puzzled.
> 
> The patch b0d4f817ba5d: "ocfs2/dlm: Fix race in adding/removing
> lockres' to/from the tracking list" from Dec 16, 2008, leads to the
> following Smatch static checker warning:
> 
> 	fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start()
> 	warn: iterator used outside loop: 'res'
> 
> fs/ocfs2/dlm/dlmdebug.c
>     539 static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>     540 {
>     541         struct debug_lockres *dl = m->private;
>     542         struct dlm_ctxt *dlm = dl->dl_ctxt;
>     543         struct dlm_lock_resource *oldres = dl->dl_res;
>     544         struct dlm_lock_resource *res = NULL;
>     545         struct list_head *track_list;
>     546 
>     547         spin_lock(&dlm->track_lock);
>     548         if (oldres)
>     549                 track_list = &oldres->tracking;
>     550         else {
>     551                 track_list = &dlm->tracking_list;
>     552                 if (list_empty(track_list)) {
>     553                         dl = NULL;
>     554                         spin_unlock(&dlm->track_lock);
>     555                         goto bail;
>     556                 }
> 
> Why not do the list_empty() check after the else statement.  In the
> current code if "&oldres->tracking" is empty it will lead to a crash.
> 
lockres will be added into tracking_list during initialize.


>     557         }
>     558 
>     559         list_for_each_entry(res, track_list, tracking) {
>     560                 if (&res->tracking == &dlm->tracking_list)
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This should never be possible.  How is it possible?  If
> &dlm->tracking_list is the list head the it's not possible without
> memory corruption.  If &oldres->tracking is the list head then I do not
> see how it is possible without memory corruption.  We can't mix different
> types of list entries on the same list head?>
In case of oldres, and the iterator points to dlm_ctxt.
In this case, the lockres is not a valid one.
 
>     561                         res = NULL;
>     562                 else
>     563                         dlm_lockres_get(res);
>     564                 break;
> 
> If we using list_first_entry() instead of open coding it then this
> function becomes a lot simpler.  See patch below:
> 
>     565         }
>     566         spin_unlock(&dlm->track_lock);
>     567 
>     568         if (oldres)
>     569                 dlm_lockres_put(oldres);
>     570 
>     571         dl->dl_res = res;
>     572 
> --> 573         if (res) {
>     574                 spin_lock(&res->spinlock);
> 
> Here is where the crash would happen if the &oldres->tracking list is
> empty.
> 
>     575                 dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
>     576                 spin_unlock(&res->spinlock);
>     577         } else
>     578                 dl = NULL;
>     579 
>     580 bail:
>     581         /* passed to seq_show */
>     582         return dl;
>     583 }
> 
> Here is a patch which removes the "if (&res->tracking == &dlm->tracking_list)"
> and uses list_first_entry_or_null().  It removes 13 lines of code.
> 
> regards,
> dan carpenter
> 
> ---
>  fs/ocfs2/dlm/dlmdebug.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index d442cf5dda8a..fb61cdfe0d06 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -547,37 +547,24 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>  	spin_lock(&dlm->track_lock);
>  	if (oldres)
>  		track_list = &oldres->tracking;
> -	else {
> +	else
>  		track_list = &dlm->tracking_list;
> -		if (list_empty(track_list)) {
> -			dl = NULL;
> -			spin_unlock(&dlm->track_lock);
> -			goto bail;
> -		}
> -	}
>  
> -	list_for_each_entry(res, track_list, tracking) {
> -		if (&res->tracking == &dlm->tracking_list)
> -			res = NULL;
> -		else
> -			dlm_lockres_get(res);
> -		break;
> -	}
> +	res = list_first_entry_or_null(track_list, struct dlm_lock_resource,
> +				       tracking);
>  	spin_unlock(&dlm->track_lock);
> +	if (!res)
> +		return NULL;
>  
>  	if (oldres)
>  		dlm_lockres_put(oldres);
>  
>  	dl->dl_res = res;
>  
> -	if (res) {
> -		spin_lock(&res->spinlock);
> -		dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
> -		spin_unlock(&res->spinlock);
> -	} else
> -		dl = NULL;
> +	spin_lock(&res->spinlock);
> +	dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
> +	spin_unlock(&res->spinlock);
>  
> -bail:
>  	/* passed to seq_show */
>  	return dl;
>  }

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel