From patchwork Wed Oct 7 14:12:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 527304 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 5D3A8140D88 for ; Thu, 8 Oct 2015 01:12:13 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b=SKyr8Aks; 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=mFhy pbLt/DGX1H+7DzrjE4jNIkkcD7qFxW1YBibXrsSm/OJ0S7tQK4vq+mwldwKZ+TEy 8+2I//fdQlQe69GNSJay1LzE4W+WrwSCeaK7+Zk7bxuhpn9fpovpsceDxPq0bqM7 lwQktq6s41AGsaVyeNAtYcUrjPB8teW1N00lTos= 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=RJnQ1GJvVU K3RNbOiU/lIR48kI8=; b=SKyr8AksJ6aKVcjtPM8TIOfzHwPZKm5nQmGQK7oBuD raw7n5GDUFH2hoWwxBL8DrnL6Qp+fJfvBx/aBuvmUcJBSon0oSu2ZbMkU28s2qqa LGqs1IPEdn1GV8w9/YZuS2BC7VtNc97PcQ9dydSiyrX/8ODVr7TxnnI5NtxcS44f E= Received: (qmail 113572 invoked by alias); 7 Oct 2015 14:12:06 -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 113560 invoked by uid 89); 7 Oct 2015 14:12:06 -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 v2] 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> Cc: Siddhesh Poyarekar , GNU C Library From: Florian Weimer X-Enigmail-Draft-Status: N1110 Message-ID: <56152831.7020707@redhat.com> Date: Wed, 7 Oct 2015 16:12:01 +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: <1444213721.25110.68.camel@localhost.localdomain> On 10/07/2015 12:28 PM, Torvald Riegel wrote: > If you should write this page, check whether you really need the acquire > MO in the load in the critical section, or whether relaxed MO is > sufficient ;) Acquire/release is needed because the access to the actual data needs to be synchronized as well. I'm struggling with the cppmem syntax to show that. Is there a grammar published anywhere? >> It's probably best to document things that work and way, and stick to >> that, instead of using ad-hoc schemes all over the place. > > I agree that it's good to follow common patterns. Nonetheless, you > still need to say why the pattern applies, and make sure that all > readers see the pattern and can check that the pattern is applied > properly. This can be brief -- but if people in the community feel like > they need more details in the comments, it should be provided in most > cases, I think. It's not double-checked locking exactly, the lock can be taken multiple times because the initialization might yield 0, in which case it is retried (<= 0 is used instead of < 0). I don't know if this intentional, but I'm not going to change that. It also means that I can't use __libc_once. >> (I think the >> code I showed is sufficiently close to the usual double-checked locking >> idiom, except that the guarded section may run multiple times.) > > Are you referring to that the critical section may be entered > unnecessarily? That is common for double-checked locking. Or are you > referring to something else? This part: if (num_ifs <= 0) { struct ifreq *ifr, *cur_ifr; int sd, num, i; … sd = __socket (AF_INET, SOCK_DGRAM, 0); if (sd < 0) return; … __libc_lock_lock (lock); … if (num_ifs <= 0) { The standard idiom would create the socket *after* the final if statement in that sequence above. Nothing is gained by doing the socket creation in parallel. I'm attaching a new version of the patch with some additional comments. I also fixed one logic bug not present in the original code and relaxed one of the loads. However, I'm not sure if this is the right approach. I wonder if it is better to separate the concurrency aspects in a helper function (and maybe simplify it at the time), and have the actual code use the abstraction instead. Florian 2015-10-07 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..1497b05 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 @@ -369,148 +370,191 @@ libc_freeres_ptr ( static struct netaddr { int addrtype; union { struct { u_int32_t addr; u_int32_t mask; } ipv4; } u; } *ifaddrs); # endif /* Reorder addresses returned in a hostent such that the first address is an address on the local subnet, if there is such an address. Otherwise, nothing is changed. Note that this function currently only handles IPv4 addresses. */ void _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 + taken during initialization, afterwards, a positive num_ifs value + indicates initialization. */ __libc_lock_define_initialized (static, lock); /* Only reorder if we're supposed to. */ if ((_res_hconf.flags & HCONF_FLAG_REORDER) == 0) return; /* Can't deal with anything but IPv4 for now... */ if (hp->h_addrtype != AF_INET) return; - if (num_ifs <= 0) + num_ifs_local = atomic_load_acquire (&num_ifs); + if (num_ifs_local <= 0) { struct ifreq *ifr, *cur_ifr; int sd, num, i; /* Save errno. */ int save = errno; /* Initialize interface table. */ /* The SIOCGIFNETMASK ioctl will only work on an AF_INET socket. */ sd = __socket (AF_INET, SOCK_DGRAM, 0); if (sd < 0) return; /* 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 + regular load is sufficient because we have the lock, and + num_ifs is only updated under the lock. */ + num_ifs_local = 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. */ __ifreq (&ifr, &num, sd); if (!ifr) goto cleanup; ifaddrs = malloc (num * sizeof (ifaddrs[0])); if (!ifaddrs) goto cleanup1; /* Copy usable interfaces in ifaddrs structure. */ for (cur_ifr = ifr, i = 0; i < num; cur_ifr = __if_nextreq (cur_ifr), ++i) { union { struct sockaddr sa; struct sockaddr_in sin; } ss; if (cur_ifr->ifr_addr.sa_family != AF_INET) continue; ifaddrs[new_num_ifs].addrtype = AF_INET; ss.sa = cur_ifr->ifr_addr; ifaddrs[new_num_ifs].u.ipv4.addr = ss.sin.sin_addr.s_addr; if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0) continue; ss.sa = cur_ifr->ifr_netmask; ifaddrs[new_num_ifs].u.ipv4.mask = ss.sin.sin_addr.s_addr; /* Now we're committed to this entry. */ ++new_num_ifs; } /* Just keep enough memory to hold all the interfaces we want. */ ifaddrs = realloc (ifaddrs, new_num_ifs * sizeof (ifaddrs[0])); assert (ifaddrs != NULL); cleanup1: __if_freereq (ifr, num); cleanup: /* 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. */ + 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); __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; + /* At this point, access to ifaddrs is free from data races: + + (1) num_ifs is positive, so we know the initialization code has + completed in some thread because the release-store to num_ifs + is dependency-ordered before the acquire-load. (If this + thread performed the initialization, it is sequence-ordered + before this point.) + + (2) The initialization of ifaddrs (both of ifaddrs itself and the + data pointed to it) is sequenced before the write to the + update to new_num_ifs. + + (3) Any potential write would have to happen in the + initialization code, but no consistent execution can re-enter + it after num_ifs is positive. + + Consequently, all potential writes to ifaddrs (and the data it + points to) happened 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; if (((haddr->s_addr ^ if_addr) & if_netmask) == 0) { void *tmp; tmp = hp->h_addr_list[i]; hp->h_addr_list[i] = hp->h_addr_list[0]; hp->h_addr_list[0] = tmp; return; } } } #endif /* defined(SIOCGIFCONF) && ... */ } /* If HOSTNAME has a postfix matching any of the trimdomains, trim away that postfix. Notice that HOSTNAME is modified inplace. Also, the original code applied all trimdomains in order, meaning that the same domainname could be trimmed multiple times. I believe this was unintentional. */ void