Re: [Ocfs2-devel] [PATCH 1/2] fs/ocfs2/dlm: Eliminate update of list_for_each_entry loop cursor

[Joel Becker <jlbec@evilplan.org>]

On Wed, Nov 02, 2011 at 10:05:16AM -0700, Sunil Mushran wrote:
> I think it got lost in the shuffle. We had decided to use the list_for_each().
> The code is simpler to understand than the other proposed fix.
> 
> Joel, do you want me to send a patch?

Please do.

> 
> On 11/02/2011 12:39 AM, Dan Carpenter wrote:
> > What ever happened with this?  The bug is still there in the latest
> > kernel.
> >
> > I think from previous discussion about this that we only ever have
> > one lock so lock->ml.cookie is always equal to ml->cookie and we
> > never set lock to NULL.  So we never actually hit the NULL deref.
> > But it should probably still be cleaned up.
> >
> > regards,
> > dan carpenter
> >
> > On Wed, Aug 11, 2010 at 05:03:56PM -0700, Joel Becker wrote:
> >> On Sat, Aug 07, 2010 at 11:09:13AM +0200, Julia Lawall wrote:
> >>> From: Julia Lawall<julia@diku.dk>
> >>>
> >>> list_for_each_entry uses its first argument to move from one element to the
> >>> next, so modifying it can break the iteration.
> >> 	Thanks for catching the bug.  It was introduced by 800deef3
> >> [ocfs2: use list_for_each_entry where benefical].  I blame Christoph.
> >>
> >>> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> >>> index 9dfaac7..7084a11 100644
> >>> --- a/fs/ocfs2/dlm/dlmrecovery.c
> >>> +++ b/fs/ocfs2/dlm/dlmrecovery.c
> >>> @@ -1792,10 +1792,10 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm,
> >>>   			for (j = DLM_GRANTED_LIST; j<= DLM_BLOCKED_LIST; j++) {
> >>>   				tmpq = dlm_list_idx_to_ptr(res, j);
> >>>   				list_for_each_entry(lock, tmpq, list) {
> >>> -					if (lock->ml.cookie != ml->cookie)
> >>> +					if (lock->ml.cookie != ml->cookie) {
> >>>   						lock = NULL;
> >>> -					else
> >>>   						break;
> >>> +					}
> >>>   				}
> >>>   				if (lock)
> >>>   					break;
> >> 	However, this is not the correct solution.  The goal of the
> >> original code, which used to use list_for_each(), was to leave lock
> >> non-NULL if the cookie was found.  Your version merely exits the loop on
> >> the first non-matching entry, always leaving lock==NULL if there is a
> >> non-matching entry.
> >> 	One possible solution is to return the original code:
> >>
> >> --8<-----------------------------------------------------------------
> >> @@ -1747,7 +1747,7 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm,
> >>   				     struct dlm_migratable_lockres *mres)
> >>   {
> >>   	struct dlm_migratable_lock *ml;
> >> -	struct list_head *queue;
> >> +	struct list_head *queue, *iter;
> >>   	struct list_head *tmpq = NULL;
> >>   	struct dlm_lock *newlock = NULL;
> >>   	struct dlm_lockstatus *lksb = NULL;
> >> @@ -1791,11 +1791,12 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm,
> >>   			spin_lock(&res->spinlock);
> >>   			for (j = DLM_GRANTED_LIST; j<= DLM_BLOCKED_LIST; j++) {
> >>   				tmpq = dlm_list_idx_to_ptr(res, j);
> >> -				list_for_each_entry(lock, tmpq, list) {
> >> -					if (lock->ml.cookie != ml->cookie)
> >> -						lock = NULL;
> >> -					else
> >> +				list_for_each(iter, tmpq) {
> >> +					lock = list_entry(iter, struct dlm_lock, list);
> >> +
> >> +					if (lock->ml.cookie == ml->cookie)
> >>   						break;
> >> +					lock = NULL;
> >>   				}
> >>   				if (lock)
> >>   					break;
> >> -->8-----------------------------------------------------------------
> >>
> >> 	Another approach would be to keep list_for_each_entry() around,
> >> but use a better check for entry existence:
> >>
> >> --8<-----------------------------------------------------------------
> >> @@ -1792,13 +1792,12 @@ static int dlm_process_recovery_data(struct dlm_ctxt *dlm,
> >>   			for (j = DLM_GRANTED_LIST; j<= DLM_BLOCKED_LIST; j++) {
> >>   				tmpq = dlm_list_idx_to_ptr(res, j);
> >>   				list_for_each_entry(lock, tmpq, list) {
> >> -					if (lock->ml.cookie != ml->cookie)
> >> -						lock = NULL;
> >> -					else
> >> +					if (lock->ml.cookie == ml->cookie)
> >>   						break;
> >>   				}
> >> -				if (lock)
> >> +				if (&lock->list != tmpq)
> >>   					break;
> >> +				lock = NULL;
> >>   			}
> >>
> >>   			/* lock is always created locally first, and
> >> -->8-----------------------------------------------------------------
> >>
> >> 	I think I like the second one better.  Sunil, what do you think?
> >>
> >> Joel
> >>
> >> -- 
> >>
> >> Life's Little Instruction Book #335
> >>
> >> 	"Every so often, push your luck."
> >>
> >> Joel Becker
> >> Consulting Software Developer
> >> Oracle
> >> E-mail: joel.becker@oracle.com
> >> Phone: (650) 506-8127
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

-- 

Life's Little Instruction Book #43

	"Never give up on somebody.  Miracles happen every day."

			http://www.jlbec.org/
			jlbec@evilplan.org