Re: [Xen-devel] xen/p2m: m2p_find_override: use list_for_each_entry_safe

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 20 Apr 2012, Dan Carpenter wrote:
> On Fri, Apr 20, 2012 at 12:23:21PM +0100, Stefano Stabellini wrote:
> > On Fri, 20 Apr 2012, Dan Carpenter wrote:
> > > Hi Stefano,
> > > 
> > > I had a question about 8f2854c74ff4: "xen/p2m: m2p_find_override: use
> > > list_for_each_entry_safe".
> > > 
> > > I think there is a misunderstanding about what the
> > > list_for_each_entry_safe() macro does.  It has nothing to do with
> > > locking, so the spinlock is still needed.  Without the lock ->next could
> > > point to an element which has been deleted in another thread.  Probably
> > > the patch should be reverted.
> > 
> > I thought that list_for_each_entry_safe is safe against deletion, is it
> > not?
> > It doesn't matter whether we get up to date entries or old entries
> > here as long as walking through the list doesn't break if a concurrent
> > thread adds or removes items.
> > 
> 
> It's safe against deletion in the same thread.  But not against
> deletion from another thread.
> 
> At the beginning of the loop it stores a pointer to the next
> element.  If you delete the element you are on, no problem because
> you already have a pointer to the next one.  But if another thread
> deletes the next element, now you have a pointer which is wrong.

The problem is not that the next element is wrong because we should be
able to cope with that.
The problem is that the next->next pointer would be set LIST_POISON1.

Maybe replacing our call to list_del with __list_del would be sufficient
to solve the problem?
Probably it is best to revert the patch at this stage.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel