| 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
| ||
|
Message-ID: <20111117084338.GB7319@noexit.corp.google.com>
Date: Thu, 17 Nov 2011 00:43:38 -0800
From: Joel Becker <jlbec@...lplan.org>
To: Sunil Mushran <sunil.mushran@...cle.com>
Cc: Dan Carpenter <dan.carpenter@...cle.com>,
Mark Fasheh <mfasheh@...e.com>,
kernel-janitors@...r.kernel.org, Julia Lawall <julia@...u.dk>,
linux-kernel@...r.kernel.org, ocfs2-devel@....oracle.com
Subject: Re: [Ocfs2-devel] [PATCH 1/2] fs/ocfs2/dlm: Eliminate update of
list_for_each_entry loop cursor
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@...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;
> >> }
> >>
> >> /* 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@...cle.com
> >> Phone: (650) 506-8127
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> >> the body of a message to majordomo@...r.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@....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@...lplan.org
--
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/
Powered by blists - more mailing lists