| Message ID | 1471436135.14544.96.camel@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
On 08/17/2016 02:15 PM, Torvald Riegel wrote: > _res_hconf.initialized was not suitable for use in a multi-threaded > environment due to the lack of atomics and memory barriers. Use of it > was also unnecessary because _res_hconf_init did the right thing by > using __libc_once. This patch fixes the glibc-internal uses by just > calling _res_hconf_init unconditionally, and switches to a release MO > atomic store for _res_hconf.initialized to fix the glibc side of the > synchronization problem (which will maintain backward compatibility, but > cannot fix the lack of acquire MO on any glibc-external loads). > > [BZ #20477] > * resolv/res_hconf.c (do_init): Use atomic access. > * resolv/res_hconf.h: Add comments. > * nscd/aicache.c (addhstaiX): Call _res_hconf_init unconditionally. > * nss/getXXbyYY_r.c (REENTRANT_NAME): Likewise. > * sysdeps/posix/getaddrinfo.c (gaih_inet): Likewise. Looks good to me. Please commit. Thanks, Florian
On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h > index b97734d..a3d23f3 100644 > --- a/resolv/res_hconf.h > +++ b/resolv/res_hconf.h > @@ -25,6 +25,15 @@ > > struct hconf > { > + /* We keep the INITIALIZED member only for backwards compatibility. New > + code should just call _res_hconf_init unconditionally. For this field > + to be used safely, users must ensure that either (1) a call to > + _res_hconf_init happens-before any load from INITIALIZED, or (2) an ^ > + assignment of zero to INITIALIZED happens-before any load from it, and ^ Those hyphens look wrong. Andreas.
On 08/17/2016 04:51 PM, Andreas Schwab wrote: > On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > >> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >> index b97734d..a3d23f3 100644 >> --- a/resolv/res_hconf.h >> +++ b/resolv/res_hconf.h >> @@ -25,6 +25,15 @@ >> >> struct hconf >> { >> + /* We keep the INITIALIZED member only for backwards compatibility. New >> + code should just call _res_hconf_init unconditionally. For this field >> + to be used safely, users must ensure that either (1) a call to >> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an > ^ >> + assignment of zero to INITIALIZED happens-before any load from it, and > ^ > > Those hyphens look wrong. “happens-before” is third-person singular of the verb “happen-before”. It's a technical term widely used while discussing memory models. Here's an example: <http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5> Florian
On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > On 08/17/2016 04:51 PM, Andreas Schwab wrote: >> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: >> >>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >>> index b97734d..a3d23f3 100644 >>> --- a/resolv/res_hconf.h >>> +++ b/resolv/res_hconf.h >>> @@ -25,6 +25,15 @@ >>> >>> struct hconf >>> { >>> + /* We keep the INITIALIZED member only for backwards compatibility. New >>> + code should just call _res_hconf_init unconditionally. For this field >>> + to be used safely, users must ensure that either (1) a call to >>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an >> ^ >>> + assignment of zero to INITIALIZED happens-before any load from it, and >> ^ >> >> Those hyphens look wrong. > > “happens-before” is third-person singular of the verb > “happen-before”. It's a technical term widely used while discussing memory > models. But writing it without hyphen is standard English and doesn't require special knowledge. Andreas.
On 08/18/2016 09:05 AM, Andreas Schwab wrote: > On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > >> On 08/17/2016 04:51 PM, Andreas Schwab wrote: >>> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: >>> >>>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h >>>> index b97734d..a3d23f3 100644 >>>> --- a/resolv/res_hconf.h >>>> +++ b/resolv/res_hconf.h >>>> @@ -25,6 +25,15 @@ >>>> >>>> struct hconf >>>> { >>>> + /* We keep the INITIALIZED member only for backwards compatibility. New >>>> + code should just call _res_hconf_init unconditionally. For this field >>>> + to be used safely, users must ensure that either (1) a call to >>>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an >>> ^ >>>> + assignment of zero to INITIALIZED happens-before any load from it, and >>> ^ >>> >>> Those hyphens look wrong. >> >> “happens-before” is third-person singular of the verb >> “happen-before”. It's a technical term widely used while discussing memory >> models. > > But writing it without hyphen is standard English and doesn't require > special knowledge. But making sense of this comment requires special knowledge, and writing the verb as “happens-before” makes it clear that the memory relation is meant, and not some foggy concept of ordering. It's like MAY/SHOULD/RECOMMEND in IETF RFCs in this regard. Florian
On Thu, 2016-08-18 at 10:05 +0200, Florian Weimer wrote: > On 08/18/2016 09:05 AM, Andreas Schwab wrote: > > On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 08/17/2016 04:51 PM, Andreas Schwab wrote: > >>> On Aug 17 2016, Torvald Riegel <triegel@redhat.com> wrote: > >>> > >>>> diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h > >>>> index b97734d..a3d23f3 100644 > >>>> --- a/resolv/res_hconf.h > >>>> +++ b/resolv/res_hconf.h > >>>> @@ -25,6 +25,15 @@ > >>>> > >>>> struct hconf > >>>> { > >>>> + /* We keep the INITIALIZED member only for backwards compatibility. New > >>>> + code should just call _res_hconf_init unconditionally. For this field > >>>> + to be used safely, users must ensure that either (1) a call to > >>>> + _res_hconf_init happens-before any load from INITIALIZED, or (2) an > >>> ^ > >>>> + assignment of zero to INITIALIZED happens-before any load from it, and > >>> ^ > >>> > >>> Those hyphens look wrong. > >> > >> “happens-before” is third-person singular of the verb > >> “happen-before”. It's a technical term widely used while discussing memory > >> models. > > > > But writing it without hyphen is standard English and doesn't require > > special knowledge. > > But making sense of this comment requires special knowledge, and writing > the verb as “happens-before” makes it clear that the memory relation is > meant, and not some foggy concept of ordering. It's like > MAY/SHOULD/RECOMMEND in IETF RFCs in this regard. Yes. As Florian says, this is meant to refer to the happens-before relation in the C11 memory model, and not some other notion of ordering. The formalization of the C11 memory model uses "happens-before", so it seemed like a good choice. Nonetheless, I'm open to other ways of referring to it, as long as it's precise and clearly refers to the memory model; in the end, the glibc community needs to be happy with how we talk about concurrency. If anyone has suggestions, let me know.
commit 44d3ca295ade8cb11b1473a7009705f6ec99d9bf
Author: Torvald Riegel <triegel@redhat.com>
Date: Wed Aug 17 13:56:11 2016 +0200
Fix incorrect double-checked locking related to _res_hconf.initialized.
_res_hconf.initialized was not suitable for use in a multi-threaded
environment due to the lack of atomics and memory barriers. Use of it was
also unnecessary because _res_hconf_init did the right thing by using
__libc_once. This patch fixes the glibc-internal uses by just calling
_res_hconf_init unconditionally, and switches to a release MO atomic store
for _res_hconf.initialized to fix the glibc side of the synchronization
problem (which will maintain backward compatibility, but cannot fix the
lack of acquire MO on any glibc-external loads).
[BZ #20477]
* resolv/res_hconf.c (do_init): Use atomic access.
* resolv/res_hconf.h: Add comments.
* nscd/aicache.c (addhstaiX): Call _res_hconf_init unconditionally.
* nss/getXXbyYY_r.c (REENTRANT_NAME): Likewise.
* sysdeps/posix/getaddrinfo.c (gaih_inet): Likewise.
diff --git a/nscd/aicache.c b/nscd/aicache.c
index a2e6cf8..32c8f57 100644
--- a/nscd/aicache.c
+++ b/nscd/aicache.c
@@ -101,8 +101,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
nip = hosts_database;
/* Initialize configurations. */
- if (__glibc_unlikely (!_res_hconf.initialized))
- _res_hconf_init ();
+ _res_hconf_init ();
if (__res_maybe_init (&_res, 0) == -1)
no_more = 1;
diff --git a/nss/getXXbyYY_r.c b/nss/getXXbyYY_r.c
index 93af253..18d3ad6 100644
--- a/nss/getXXbyYY_r.c
+++ b/nss/getXXbyYY_r.c
@@ -274,8 +274,7 @@ INTERNAL (REENTRANT_NAME) (ADD_PARAMS, LOOKUP_TYPE *resbuf, char *buffer,
}
#endif /* need _res */
#ifdef NEED__RES_HCONF
- if (!_res_hconf.initialized)
- _res_hconf_init ();
+ _res_hconf_init ();
#endif /* need _res_hconf */
void *tmp_ptr = fct.l;
diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 5cd1289..093c268 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -348,7 +348,8 @@ do_init (void)
arg_trimdomain_list (ENV_TRIM_OVERR, 1, envval);
}
- _res_hconf.initialized = 1;
+ /* See comments on the declaration of _res_hconf. */
+ atomic_store_release (&_res_hconf.initialized, 1);
}
diff --git a/resolv/res_hconf.h b/resolv/res_hconf.h
index b97734d..a3d23f3 100644
--- a/resolv/res_hconf.h
+++ b/resolv/res_hconf.h
@@ -25,6 +25,15 @@
struct hconf
{
+ /* We keep the INITIALIZED member only for backwards compatibility. New
+ code should just call _res_hconf_init unconditionally. For this field
+ to be used safely, users must ensure that either (1) a call to
+ _res_hconf_init happens-before any load from INITIALIZED, or (2) an
+ assignment of zero to INITIALIZED happens-before any load from it, and
+ these loads use acquire MO if the intent is to skip calling
+ _res_hconf_init if the load returns a nonzero value. Such acquire MO
+ loads will then synchronize with the release MO store to INITIALIZED
+ in do_init in res_hconf.c; see pthread_once for more detail. */
int initialized;
int unused1;
int unused2[4];
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 574ce08..09fbc83 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -816,8 +816,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
nip = __nss_hosts_database;
/* Initialize configurations. */
- if (__glibc_unlikely (!_res_hconf.initialized))
- _res_hconf_init ();
+ _res_hconf_init ();
if (__res_maybe_init (&_res, 0) == -1)
no_more = 1;
_res_hconf.initialized was not suitable for use in a multi-threaded environment due to the lack of atomics and memory barriers. Use of it was also unnecessary because _res_hconf_init did the right thing by using __libc_once. This patch fixes the glibc-internal uses by just calling _res_hconf_init unconditionally, and switches to a release MO atomic store for _res_hconf.initialized to fix the glibc side of the synchronization problem (which will maintain backward compatibility, but cannot fix the lack of acquire MO on any glibc-external loads). [BZ #20477] * resolv/res_hconf.c (do_init): Use atomic access. * resolv/res_hconf.h: Add comments. * nscd/aicache.c (addhstaiX): Call _res_hconf_init unconditionally. * nss/getXXbyYY_r.c (REENTRANT_NAME): Likewise. * sysdeps/posix/getaddrinfo.c (gaih_inet): Likewise.