Re: [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>]

On Wed, Mar 09, 2022 at 03:46:11PM +0800, Joseph Qi wrote:
> 
> 
> 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.
> 

I am not sure we understand each other.

This function is iterating over the list.  Nothing should be modifying
the list until after we have released the spinlock.  Any modifications
is a race condition.  Which initialize function are you talking about?

In the currect code if oldres is non-NULL and list_empty(&oldres->tracking)
is true then it leads to a crash.  There is no reason that we cannot do:

	if (oldres)
		track_list = &oldres->tracking;
	else
		track_list = &dlm->tracking_list;

	if (list_empty(track_list)) {
		dl = NULL;
		spin_unlock(&dlm->track_lock);
		goto bail;
	}

This is cleaner and now we do not have know from outside context
whether the &oldres->tracking list can be empty.

> 
> >     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.

That doesn't make sense.  :/  This condition is doing pointer math.
The offset of ->tracking is 136 bytes and ->tracking list is 88 bytes
into the dlm struct.

	if ((void *)res + 136 == (void *)dlm_ctxt + 88)

Use algebra to subtract 88 from both sides.

	if ((void *)res + 48 == (void *)dlm_ctxt)

So dlm points to somewhere in the middle of "res" struct.

>  
> >     561                         res = NULL;
> >     562                 else
> >     563                         dlm_lockres_get(res);
> >     564                 break;
> > 

regards,
dan carpenter


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