From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96709 invoked by alias); 5 Oct 2017 20:23:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 96698 invoked by uid 89); 5 Oct 2017 20:23:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=structured, Discussion, slash X-HELO: mail-qk0-f181.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=Cm6IscoaHLNBGzqBhYbYXATm9PQRh7ym3XaQzBIt1JY=; b=bvryyfuMT7YhDX55xh1glHndemeeGSNvpudsr6rurp4gBwsKsWnt3XSKH1+t1fkP71 kvsoOM+aWww4nDk4fhyzbY5cHGymwzTDrXNebshGFVr7FdJ3iNEkec9YOVgIW8FZCaMl AFvzxHXWe0rZTm1pg7kid6a+9k+OGDsgH4KU5WvP4pGoPdBztHCkdxOLIy3yY3n9RX3K 97amV7BU9j3JO4HB7g0hWSj0OqychCP038BgQs7NmXzTGpA35L1T8+f5t1sXMaHkS24l HKCjv9KDCwzg69rp+HybnMF9WUCg5lp22dnae8ZemjBU6Ut7crGOqPweXeT+pbI1PvES aTTw== X-Gm-Message-State: AMCzsaVM9pDVDs2KRZ+AF9yZ8ZaX3HjoCmoCIvJYa5ifBfOYPUujWwIk gfY80Qjklaw2lW4zBAUiJaVD0w== X-Google-Smtp-Source: AOwi7QAXIr2mQzNq6v78QXWAUXTbf+KwzmcCVkqmnxlK13mHOllZ3lRwlz+IO4ATAo4L7+SQq1B/8g== X-Received: by 10.55.44.67 with SMTP id s64mr19558770qkh.284.1507234976375; Thu, 05 Oct 2017 13:22:56 -0700 (PDT) Subject: Re: [PATCH v2] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062] To: Arjun Shankar , libc-alpha@sourceware.org Cc: Florian Weimer References: <20171002143148.GA41930@aloka.lostca.se> From: Carlos O'Donell Message-ID: <940c9be0-37ae-ebf7-1013-e6e6a4991028@redhat.com> Date: Thu, 05 Oct 2017 20:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171002143148.GA41930@aloka.lostca.se> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-10/txt/msg00299.txt.bz2 On 10/02/2017 07:31 AM, Arjun Shankar wrote: > These two functions running in different threads could encounter a data race > if the CPU or compiler reordered the store of __gconv_path_elem so as to be > earlier than writes to the content that it points to. This has now been > corrected by using atomics every time the variable is accessed. My reviews are structured into 3 pats: high level, design, and implementation. (a) At a high level I see a problem with this change. All of the callers of __gconv_read_conf do so using __libc_once. Why can't we just delete all the double checked locking assuming that __libc_once works correctly? The use of __libc_once ensures that __gconv_read_conf is called only once, and that takes into account all threads. The only scenario it doesn't take into account is if an in-progress call to __libc_once was interrupted by a dlopen of libpthread that then started threads. Those threads might attempt a parallel initialization. Though this problem has existed *before* the changes you are making. Either way, because of the use of __libc_once, the function need not do any locking or atomics. Was the __libc_once usage by the callers considered when the patch was created? (b) At the design level, the changes look good. What you've done is a correct fix of DCLP (thought at a high level it might not be needed and we might just delete the code and update the comment to say it must be called with __libc_once). I see perhaps one case where an atomic operation is not needed, and I've commented on that below. (c) The details of the patch are great, and the comments are good. > ChangeLog: > > 2017-10-02 Arjun Shankar > > [BZ #22062] > * iconv/gconv_conf.c: Include . > (__gconv_get_path): Access __gconv_path_elem using atomics. > (__gconv_read_conf): Likewise. > (libc_freeres_fn): Likewise. > --- > Discussion on PATCH v1: > https://sourceware.org/ml/libc-alpha/2017-09/msg00779.html > > iconv/gconv_conf.c | 42 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c > index f1c28ce..58fd28e 100644 > --- a/iconv/gconv_conf.c > +++ b/iconv/gconv_conf.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include OK. > > #include > #include > @@ -428,8 +429,13 @@ __gconv_get_path (void) > > __libc_lock_lock (lock); > All of the caller of __gconv_read_conf do so using __libc_once, which already does locking using pthread_once to avoid __gconv_read_conf ever being called by a another thread? e.g. iconv/gconv_db.c: __libc_once (once, __gconv_read_conf); iconv/gconv_db.c: __libc_once (once, __gconv_read_conf); See my comments above. I will continue reviewing the DCLP usage below because it has value. > - /* Make sure there wasn't a second thread doing it already. */ > - result = (struct path_elem *) __gconv_path_elem; > + /* __gconv_read_conf uses double-checked locking and therefore can make > + a redundant call to this function while another thread is already > + executing it. So first we make sure another thread has not already done > + the work we want to do. > + > + A relaxed MO load is sufficient since we already have the lock. */ > + result = atomic_load_relaxed (&__gconv_path_elem); OK. Good, this removes the data race between the unordered read by a thread in __gconv_read_conf and a write by another thread in __gonv_get_path. > if (result == NULL) > { > /* Determine the complete path first. */ > @@ -519,7 +525,10 @@ __gconv_get_path (void) > result[n].len = 0; > } > > - __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem; > + /* This store synchronizes with the acquire MO load in > + __gconv_read_conf. */ > + atomic_store_release (&__gconv_path_elem, > + result ?: (struct path_elem *) &empty_path_elem); OK. Good comment. > > free (cwd); > } > @@ -538,6 +547,7 @@ __gconv_read_conf (void) > size_t nmodules = 0; > int save_errno = errno; > size_t cnt; > + struct path_elem *gconv_path_elem_local; OK. > > /* First see whether we should use the cache. */ > if (__gconv_load_cache () == 0) > @@ -549,13 +559,20 @@ __gconv_read_conf (void) > > #ifndef STATIC_GCONV > /* Find out where we have to look. */ > - if (__gconv_path_elem == NULL) > - __gconv_get_path (); > > - for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) > + /* This load is synchronized with the release MO store done during the > + initialization of __gconv_path_elem in __gconv_get_path. */ OK. Perfect. If the acquire sees NULL it knows that the other thread might not have completed the work, and so proceeds to acquire the lock and check again (double checked locking). If it sees non-NULL it knows the other function completed all of the work and the element data should be visible. > + gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem); > + if (gconv_path_elem_local == NULL) > + { > + __gconv_get_path (); Multiple reads by threads are safe. In this case we can have multiple threads here, but all write to __gconv_path_elem are complete. All we are doing is having multiple readers, which is not a data race. Is there any reason we need this atomic load? > + gconv_path_elem_local = atomic_load_relaxed (&__gconv_path_elem); > + } > + > + for (cnt = 0; gconv_path_elem_local[cnt].name != NULL; ++cnt) > { > - const char *elem = __gconv_path_elem[cnt].name; > - size_t elem_len = __gconv_path_elem[cnt].len; > + const char *elem = gconv_path_elem_local[cnt].name; > + size_t elem_len = gconv_path_elem_local[cnt].len; OK. > char *filename; > > /* No slash needs to be inserted between elem and gconv_conf_filename; > @@ -606,6 +623,11 @@ __gconv_read_conf (void) > /* Free all resources if necessary. */ > libc_freeres_fn (free_mem) > { > - if (__gconv_path_elem != NULL && __gconv_path_elem != &empty_path_elem) > - free ((void *) __gconv_path_elem); > + /* __gconv_path_elem is always accessed atomically because it is used in > + double-checked locking. Since this function is called when the process > + has become single-threaded, it is enough to used a relaxed MO load. */ OK. We use an atomic load to see the update from a thread that initialized __gconv_path_elem. > + struct path_elem *gconv_path_elem_local > + = atomic_load_relaxed (&__gconv_path_elem); > + if (gconv_path_elem_local != NULL && gconv_path_elem_local != &empty_path_elem) > + free ((void *) gconv_path_elem_local); > } > -- Cheers, Carlos.