linux-kernel - Re: [PATCH 1/2] fs/ocfs2/dlm: Eliminate update of list_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.1008120735200.16027@ask.diku.dk>
Date:	Thu, 12 Aug 2010 07:46:13 +0200 (CEST)
From:	Julia Lawall <julia@...u.dk>
To:	Joel Becker <Joel.Becker@...cle.com>
Cc:	Mark Fasheh <mfasheh@...e.com>, ocfs2-devel@....oracle.com,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Sunil Mushran <sunil.mushran@...cle.com>
Subject: Re: [PATCH 1/2] fs/ocfs2/dlm: Eliminate update of list_for_each_entry
 loop cursor

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@...u.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/