From patchwork Mon Oct 12 15:44:51 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 529178 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 744881400CB for ; Tue, 13 Oct 2015 02:45:03 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b=YTxX+E9Q; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type; q=dns; s=default; b=WNNa Nqb5bvfpb+DJa5uS19AVwNepZYRZUvHXWgsjBmbQrz9m/ojJ6kPeJrMgS5KP0sWk yy//nJGF/R57PMR8ZmTLnngHTW3tgP8ZmvusmjevfvieAcMRf6BafQ034ZUahhTC HqaqbcHkcmS8Br6c1QHD/XwZanMdyA2wUIMaJ/c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type; s=default; bh=7thlF0bSqv IOcmn5tL/bIgYbrT4=; b=YTxX+E9QBnG+LFxowOD+OXmEZrwEBbVpUMZ+W603L+ nRug71ppGbYYreUwSS7Xj+eilDGw0989ZBjAiB4nGfLE4KehrArFUVk4cOVaVAiY KsfHNo5R8GZ2h3QoLp5Cm3JP27QuEWfjDvYE0SPfQcGfVVA0QkHDDAZ0PhSHosue I= Received: (qmail 39128 invoked by alias); 12 Oct 2015 15:44:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 38483 invoked by uid 89); 12 Oct 2015 15:44:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Subject: [PATCH v3] Fix double-checked locking in _res_hconf_reorder_addrs [BZ #19074] To: Torvald Riegel References: <5613BF47.9000503@redhat.com> <5613D5F5.7020603@reserved-bit.com> <5613DBB6.8020109@redhat.com> <1444213721.25110.68.camel@localhost.localdomain> <56152831.7020707@redhat.com> <1444317267.25110.195.camel@localhost.localdomain> Cc: Siddhesh Poyarekar , GNU C Library From: Florian Weimer Message-ID: <561BD573.40003@redhat.com> Date: Mon, 12 Oct 2015 17:44:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1444317267.25110.195.camel@localhost.localdomain> On 10/08/2015 05:14 PM, Torvald Riegel wrote: > And now with actual comments on the patch... Thanks. I tried to address your comments. I'm attaching the new version. Florian 2015-10-12 Florian Weimer [BZ #19074] * resolv/res_hconf.c (_res_hconf_reorder_addrs): Use atomics to load and store num_ifs. diff --git a/NEWS b/NEWS index a0a91b5..805c7b9 100644 --- a/NEWS +++ b/NEWS @@ -49,7 +49,7 @@ Version 2.22 18533, 18534, 18536, 18539, 18540, 18542, 18544, 18545, 18546, 18547, 18549, 18553, 18557, 18558, 18569, 18583, 18585, 18586, 18592, 18593, 18594, 18602, 18612, 18613, 18619, 18633, 18635, 18641, 18643, 18648, - 18657, 18676, 18694, 18696, 18887. + 18657, 18676, 18694, 18696, 18887, 19074. * Cache information can be queried via sysconf() function on s390 e.g. with _SC_LEVEL1_ICACHE_SIZE as argument. diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c index 692d948..00b620d 100644 --- a/resolv/res_hconf.c +++ b/resolv/res_hconf.c @@ -45,6 +45,7 @@ #include "ifreq.h" #include "res_hconf.h" #include +#include #if IS_IN (libc) # define fgets_unlocked __fgets_unlocked @@ -391,9 +392,14 @@ _res_hconf_reorder_addrs (struct hostent *hp) { #if defined SIOCGIFCONF && defined SIOCGIFNETMASK int i, j; - /* Number of interfaces. */ + /* Number of interfaces. Also serves as a flag for the + double-checked locking idiom. */ static int num_ifs = -1; - /* We need to protect the dynamic buffer handling. */ + /* Local copy of num_ifs, for non-atomic access. */ + int num_ifs_local; + /* We need to protect the dynamic buffer handling. The lock is only + acquired during initialization. Afterwards, a positive num_ifs + value indicates initialization. */ __libc_lock_define_initialized (static, lock); /* Only reorder if we're supposed to. */ @@ -404,7 +410,10 @@ _res_hconf_reorder_addrs (struct hostent *hp) if (hp->h_addrtype != AF_INET) return; - if (num_ifs <= 0) + /* This load synchronizes with the release MO store in the + initialization block below. */ + num_ifs_local = atomic_load_acquire (&num_ifs); + if (num_ifs_local <= 0) { struct ifreq *ifr, *cur_ifr; int sd, num, i; @@ -421,9 +430,18 @@ _res_hconf_reorder_addrs (struct hostent *hp) /* Get lock. */ __libc_lock_lock (lock); - /* Recheck, somebody else might have done the work by now. */ - if (num_ifs <= 0) + /* Recheck, somebody else might have done the work by now. A + relaxed load is sufficient because we have the lock, and + num_ifs is only updated under the lock. */ + num_ifs_local = atomic_load_relaxed (num_ifs); + if (num_ifs_local <= 0) { + /* This is the only block which writes to num_ifs. It can + be executed several times (sequentially) if + initialization does not yield any interfaces, and num_ifs + remains zero. However, once we stored a positive value + in num_ifs below, this block cannot be entered again due + to the condition above. */ int new_num_ifs = 0; /* Get a list of interfaces. */ @@ -472,7 +490,14 @@ _res_hconf_reorder_addrs (struct hostent *hp) /* Release lock, preserve error value, and close socket. */ errno = save; - num_ifs = new_num_ifs; + /* Advertise successful initialization if new_num_ifs is + positive (and no updates to ifaddrs are permitted after + that). Otherwise, num_ifs remains unchanged, at zero. + This store synchronizes with the initial acquire MO + load. */ + atomic_store_release (&num_ifs, new_num_ifs); + /* Keep the local copy current, to save another load. */ + num_ifs_local = new_num_ifs; } __libc_lock_unlock (lock); @@ -480,15 +505,43 @@ _res_hconf_reorder_addrs (struct hostent *hp) __close (sd); } - if (num_ifs == 0) + /* num_ifs_local cannot be negative because the if statement above + covered this case. It can still be zero if we just performed + initialization, but could not find any interfaces. */ + if (num_ifs_local == 0) return; + /* The code below accesses ifaddrs, so we need to ensure that the + initialization happens-before this point. + + The actual initialization is sequenced-before the release store + to num_ifs, and sequenced-before the end of the critical section. + + This means there are three possible executions: + + (1) The thread that initialized the data also uses it, so + sequenced-before is sufficient to ensure happens-before. + + (2) The release MO store of num_ifs synchronizes-with the acquire + MO load, and the acquire MO load is sequenced before the use + of the initialized data below. + + (3) We enter the critical section, and the relaxed MO load of + num_fis yields a positive value. The write to ifaddrs is + sequenced-before leaving the critical section. Leaving the + critical section happens-before we entered the critical + section ourselves, which means that the the write to ifaddrs + happens-before this point. + + Consequently, all potential writes to ifaddrs (and the data it + points to) happens-before this point. */ + /* Find an address for which we have a direct connection. */ for (i = 0; hp->h_addr_list[i]; ++i) { struct in_addr *haddr = (struct in_addr *) hp->h_addr_list[i]; - for (j = 0; j < num_ifs; ++j) + for (j = 0; j < num_ifs_local; ++j) { u_int32_t if_addr = ifaddrs[j].u.ipv4.addr; u_int32_t if_netmask = ifaddrs[j].u.ipv4.mask; -- 2.4.3