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

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

[Julia Lawall]

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.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
iterator name list_for_each_entry;
expression x,E;
position p1,p2;
@@

list_for_each_entry at p1(x,...) { <... x =@p2 E ...> }

@@
expression x,E;
position r.p1,r.p2;
statement S;
@@

*x =@p2 E
...
list_for_each_entry at p1(x,...) S
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
I don't know whether this is the right solution, but it seems plausible
considering the subsequent test on lock.  In any case, setting lock to NULL
and then going back to the top of the loop does not work.

 fs/ocfs2/dlm/dlmrecovery.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;

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

[Joel Becker]

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 at oracle.com
Phone: (650) 506-8127

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

[Julia Lawall]

On Wed, 11 Aug 2010, 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;
>  			}

This seems a bit ugly to me, since it exposes the implementation of the 
list abstraction.  What about the following:

lock = NULL;
list_for_each_entry(x, tmpq, list) {
	if (x->ml.cookie == ml->cookie) {
	   	lock = x;
		break;
	}
}

julia

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

[Dan Carpenter]

On Thu, Aug 12, 2010 at 07:46:13AM +0200, Julia Lawall wrote:
> 
> This seems a bit ugly to me, since it exposes the implementation of the 
> list abstraction.  What about the following:
> 
> lock = NULL;
> list_for_each_entry(x, tmpq, list) {
> 	if (x->ml.cookie == ml->cookie) {
> 	   	lock = x;
> 		break;
> 	}
> }
> 

I agree with you that it's ugly as pants.  Maybe someone could write an
at_list_start() macro?

regards,
dan carpenter

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

[Joel Becker]

On Thu, Aug 12, 2010 at 07:46:13AM +0200, Julia Lawall wrote:
 
> This seems a bit ugly to me, since it exposes the implementation of the 
> list abstraction.  What about the following:

	Actually, we're going to go back to the original list_for_each()
and list_entry() version.  It's a well understood idiom.
	We're also going to re-audit the rest of the patch that changed
list_for_each() to list_for_each_entry(), just in case we missed another
semantic error.

Joel

-- 

Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

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

[Sunil Mushran]

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?

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 at 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 at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joel Becker]

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 at 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 at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at 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 at evilplan.org