Re: xen/p2m: m2p_find_override: use list_for_each_entry_safe

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Apr 20, 2012 at 02:36:59PM +0100, Stefano Stabellini wrote:
> 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.

ok, reverted.