On Mon 05-01-09 18:05:21, Vlad Yasevich wrote: > Karsten, Michal Hi Vlad, > > I think I found this cursed race. > > The problem is that nothing is guarding the asoc->base.sk reference. This > reference changes during the accept()/peeloff() code paths and the only > guard around it is the socket lock. But that lock is not taken when > the reference is used for reading as it is done in sctp_rcv(). > > So, when we do sctp_bh_lock_sock(sk) in sctp_rcv(), we may be using > a stale cached copy of the sk which might have changed during sctp_accept(). > > What this allows us to do is to have a user sending a packet on a socket > at the same time we may be processing and incoming packet on the same socket. > We just end up taking different locks which totally hoses us. > > -vlad > > > diff --git a/net/sctp/input.c b/net/sctp/input.c > index bf612d9..2e4a864 100644 > --- a/net/sctp/input.c > +++ b/net/sctp/input.c > @@ -249,6 +249,19 @@ int sctp_rcv(struct sk_buff *skb) > */ > sctp_bh_lock_sock(sk); > > + if (sk != rcvr->sk) { > + /* Our cached sk is different from the rcvr->sk. This is > + * because migrate()/accept() may have moved the association > + * to a new socket and released all the sockets. So now we > + * are holding a lock on the old socket while the user may > + * be doing something with the new socket. Switch our veiw > + * of the current sk. > + */ > + sctp_bh_unlock_sock(sk); > + sk = rcvr->sk; > + sctp_bh_lock_sock(sk); > + } > + > if (sock_owned_by_user(sk)) { > SCTP_INC_STATS_BH(SCTP_MIB_IN_PKT_BACKLOG); > sctp_add_backlog(sk, skb); Can you reproduce with this patch? Unfortunately, I don't have any HW configuration available at the moment. Our SLES10SP2 kernel contains similar code: [...] /* Acquire access to the sock lock. Note: We are safe from other * bottom halves on this lock, but a user may be in the lock too, * so check if it is busy. */ sctp_bh_lock_sock(sk); /* It is possible that the association could have moved to a different * socket if it is peeled off. If so, update the sk. */ if (sk != rcvr->sk) { sctp_bh_lock_sock(rcvr->sk); sctp_bh_unlock_sock(sk); sk = rcvr->sk; } if (sock_owned_by_user(sk)) sk_add_backlog(sk, skb); else sctp_backlog_rcv(sk, skb); /* Release the sock and the sock ref we took in the lookup calls. * The asoc/ep ref will be released in sctp_backlog_rcv. */ sctp_bh_unlock_sock(sk); sock_put(sk); return ret; [...] This test, however, has been removed by 61c9fed41638249f8b6ca5345064eb1beb50179f. AFAIU this patch deals with similar race in the receive path when socket is moved. As you can see the test&relock part differs only in ordering of the old unlock and the new lock. Is this ordering important? Do we have to unlock old socket sooner to enable rcvr->sk change? Personally, I don't think so, because we would see deadlock otherwise, right? Are there any other patches which depend on the above one? -- Michal Hocko L3 team SUSE LINUX s.r.o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html