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

[Dan Carpenter via Ocfs2-devel <ocfs2-devel@oss.oracle.com>]

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.

    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?

    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;
 }
-- 
2.20.1


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