From patchwork Wed May 18 12:47:15 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jacek Luczak X-Patchwork-Id: 96163 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 9B3CA1007D1 for ; Wed, 18 May 2011 22:47:20 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933109Ab1ERMrQ (ORCPT ); Wed, 18 May 2011 08:47:16 -0400 Received: from mail-pv0-f174.google.com ([74.125.83.174]:62390 "EHLO mail-pv0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933098Ab1ERMrP convert rfc822-to-8bit (ORCPT ); Wed, 18 May 2011 08:47:15 -0400 Received: by pvg12 with SMTP id 12so686499pvg.19 for ; Wed, 18 May 2011 05:47:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=gr1SAXNGQQ2GBFbj/g/YxhF40wMdtaT2+Renj7ARJOw=; b=QGr6YSsej6KMPzNKmHmh11wvVNzmvHrxAK8N5YszL5rDVLt5kTV8TummxhOjQqi8c+ htpKQIJ36WbvSBFMNNUnqx6V535U4ij/PidniSY12FFPSeXIxSqNxx00dHsx6Rwjlsxg 2v9beGo4kWjoaIn9ZBHA2B64b6b30X5HtfIPY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=kke1lt5yQSPcmAaq1ULkYyVf9aymC9+2Axc9CeNOKmssrMC37jVC1GYbf+Vsrt1ipO 1Zgyq0LdSuRjdavflfXND215Y2fmbePgRaulqtXFfIlFBBQ8qXoDxLCr+O3ptxHZwEyB YfgGXgF822N+npUNe2aESiBy47W3xtdR+qhJc= MIME-Version: 1.0 Received: by 10.68.31.34 with SMTP id x2mr3045319pbh.153.1305722835385; Wed, 18 May 2011 05:47:15 -0700 (PDT) Received: by 10.68.48.199 with HTTP; Wed, 18 May 2011 05:47:15 -0700 (PDT) In-Reply-To: <4DD3BC8F.9050802@hp.com> References: <1305704885.2983.4.camel@edumazet-laptop> <1305707358.2983.14.camel@edumazet-laptop> <4DD38B30.9090601@cn.fujitsu.com> <4DD3BC8F.9050802@hp.com> Date: Wed, 18 May 2011 14:47:15 +0200 Message-ID: Subject: Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict() From: Jacek Luczak To: Vladislav Yasevich Cc: Wei Yongjun , Eric Dumazet , netdev@vger.kernel.org Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 2011/5/18 Vladislav Yasevich : > On 05/18/2011 05:02 AM, Wei Yongjun wrote: > >> fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just >> need to remove the socket from the port hash before empty the bind address list. >> some thing like this, not test. >> >> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c >> index c8cc24e..924d846 100644 >> --- a/net/sctp/endpointola.c >> +++ b/net/sctp/endpointola.c >> @@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep) >> >>       /* Cleanup. */ >>       sctp_inq_free(&ep->base.inqueue); >> -     sctp_bind_addr_free(&ep->base.bind_addr); >> >>       /* Remove and free the port */ >>       if (sctp_sk(ep->base.sk)->bind_hash) >>               sctp_put_port(ep->base.sk); >> >> +     sctp_bind_addr_free(&ep->base.bind_addr); >> + >>       /* Give up our hold on the sock. */ >>       if (ep->base.sk) >>               sock_put(ep->base.sk); >> >> > > I am not sure that this will guarantee avoidance of this crash, even though it is the right > thing to do in general. > > We simply make the race condition much smaller and much harder to hit.  Potentially, one > cpu may be doing lookup of the socket while the other is doing the destroy.  If the lookup cpu > finds the socket just as this code removes the socket from the hash, we still have this potential > race condition. > > I agree with Eric, rcu_read_lock() is not strictly necessary, as what we are really after is call_rcu() > based destruction.  We need to delay memory destruction for the rcu grace period. > > Thinking a little more about how bind_addr_clean() is used, it would probably benefit from getting > converted to call_rcu().  That function is used as local clean-up in case of malloc failure; however, > that doesn't preclude a potential race.  The fact that we do not have this race simply points out that > we don't have any kind of parallel lookup that can hit it (and the code confirms it).  This doesn't > mean that we wouldn't have it in the future. > OK then, at the end what Eric suggested is IMO valid: I will test this. Should be safe, avoid race not only in that case and it consistent. -Jacek --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c index faf71d1..0025d90 100644 --- a/net/sctp/bind_addr.c +++ b/net/sctp/bind_addr.c @@ -144,10 +144,9 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp) struct list_head *pos, *temp; /* Empty the bind address list. */ - list_for_each_safe(pos, temp, &bp->address_list) { - addr = list_entry(pos, struct sctp_sockaddr_entry, list); - list_del(pos); - kfree(addr); + list_for_each_entry(pos, &bp->address_list, list) { + list_del_rcu(&pos->list); + call_rcu(&pos->rcu, sctp_local_addr_free); SCTP_DBG_OBJCNT_DEC(addr); } }