Re: [PATCH 4/8] IPoIB: fix mcast_dev_flush/mcast_restart_task race

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On Sat, 2014-08-30 at 08:39 -0700, Wendy Cheng wrote:
> On Fri, Aug 29, 2014 at 2:53 PM, Wendy Cheng <s.wendy.cheng-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Aug 12, 2014 at 4:38 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> Our mcast_dev_flush routine and our mcast_restart_task can race against
> >> each other.  In particular, they both hold the priv->lock while
> >> manipulating the rbtree and while removing mcast entries from the
> >> multicast_list and while adding entries to the remove_list, but they
> >> also both drop their locks prior to doing the actual removes.  The
> >> mcast_dev_flush routine is run entirely under the rtnl lock and so has
> >> at least some locking.  The actual race condition is like this:
> >>
> >> Thread 1                                Thread 2
> >> ifconfig ib0 up
> >>   start multicast join for broadcast
> >>   multicast join completes for broadcast
> >>   start to add more multicast joins
> >>     call mcast_restart_task to add new entries
> >>                                         ifconfig ib0 down
> >>                                           mcast_dev_flush
> >>                                             mcast_leave(mcast A)
> >>     mcast_leave(mcast A)
> >>
> >> As mcast_leave calls ib_sa_multicast_leave, and as member in
> >> core/multicast.c is ref counted, we run into an unbalanced refcount
> >> issue.  To avoid stomping on each others removes, take the rtnl lock
> >> specifically when we are deleting the entries from the remove list.
> >
> > Isn't "test_and_clear_bit()" atomic so it is unlikely that
> > ib_sa_free_multicast() can run multiple times  ?
> 
> Oops .. how about if the structure itself gets freed ? My bad !

Well, just like the last email, the code you are referring to is in the
original code, and had other issues.  After my patches it does not look
like that.

> However, isn't that the remove_list a local list on the caller's stack
> ? .. and  the original list entry moving (to remove_list) is protected
> by the spin lock (priv->lock), it is unlikely that the
> ib_sa_free_multicast() can operate on the same entry ?

Yes, you're right.  I had it in my mind that the remove_list was part of
the ipoib private dev, not local on the stack.  So you are right that we
could probably get away with removing the rtnl lock there (although it
would need to be in a later patch than the one you are reviewing
here...here there would still be a race between the restart task and the
downing of the interface because they all still share the same work
queue, but once we switch to the per device work queues in patch #6,
this can happen in parallel safely with the flush task I think).

> The patch itself is harmless though .. but adding the rntl_lock is
> really not ideal.
> 
> -- Wendy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]