| Message ID | 20180813194214.20929-1-hollis_blanchard@mentor.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fix build break around __atomic_*() with GCC<4.7 | expand |
On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard <hollis_blanchard@mentor.com> wrote: > > The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat > Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to > use the (most conservative) __sync_synchronize() primitive provided by those > older GCC versions. > > Fixes https://github.com/SELinuxProject/selinux/issues/97 > > (Really, no __atomic or __sync operations are needed here at all, since POSIX > 4.12 "Memory Synchronization" says pthread_mutex_lock() and > pthread_mutex_unlock() "synchronize memory with respect to other threads"...) That section means that pthread_mutex_lock() and pthread_mutex_unlock() will perform an acquire / release operation respectively, so if you're guarding shared data with them, you don't need additional memory synchronization. In this case however, the fast path does not call pthread_mutex_lock() and thus there is no acquire operation. pthread_mutex_unlock() will perform a release operation in the thread that actually compiled the regex (so technically, we don't actually need the __atomic_store_n), but we still need an acquire operation on the fast path, which is why we need the atomic. It's a common pattern, https://en.wikipedia.org/wiki/Double-checked_locking, and always uses atomics. The rest of the change looks fine to me though. Tom Tom > > --- > libselinux/src/label_file.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h > index 2fa85474..47859baf 100644 > --- a/libselinux/src/label_file.h > +++ b/libselinux/src/label_file.h > @@ -351,8 +351,14 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, > * init_routine does not take a parameter, it's not possible > * to use, so we generate the same effect with atomics and a > * mutex */ > +#ifdef __ATOMIC_RELAXED > regex_compiled = > __atomic_load_n(&spec->regex_compiled, __ATOMIC_ACQUIRE); > +#else > + /* GCC <4.7 */ > + __sync_synchronize(); > + regex_compiled = spec->regex_compiled; > +#endif > if (regex_compiled) { > return 0; /* already done */ > } > @@ -360,8 +366,14 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, > __pthread_mutex_lock(&spec->regex_lock); > /* Check if another thread compiled the regex while we waited > * on the mutex */ > +#ifdef __ATOMIC_RELAXED > regex_compiled = > __atomic_load_n(&spec->regex_compiled, __ATOMIC_ACQUIRE); > +#else > + /* GCC <4.7 */ > + __sync_synchronize(); > + regex_compiled = spec->regex_compiled; > +#endif > if (regex_compiled) { > __pthread_mutex_unlock(&spec->regex_lock); > return 0; > @@ -404,7 +416,13 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, > } > > /* Done. */ > +#ifdef __ATOMIC_RELAXED > __atomic_store_n(&spec->regex_compiled, true, __ATOMIC_RELEASE); > +#else > + /* GCC <4.7 */ > + spec->regex_compiled = true; > + __sync_synchronize(); > +#endif > __pthread_mutex_unlock(&spec->regex_lock); > return 0; > } > -- > 2.13.0 >
On 08/13/2018 01:45 PM, Tom Cherry wrote: > On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard > <hollis_blanchard@mentor.com> wrote: >> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat >> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to >> use the (most conservative) __sync_synchronize() primitive provided by those >> older GCC versions. >> >> Fixes https://github.com/SELinuxProject/selinux/issues/97 >> >> (Really, no __atomic or __sync operations are needed here at all, since POSIX >> 4.12 "Memory Synchronization" says pthread_mutex_lock() and >> pthread_mutex_unlock() "synchronize memory with respect to other threads"...) > That section means that pthread_mutex_lock() and > pthread_mutex_unlock() will perform an acquire / release operation > respectively, so if you're guarding shared data with them, you don't > need additional memory synchronization. > > In this case however, the fast path does not call pthread_mutex_lock() > and thus there is no acquire operation. pthread_mutex_unlock() will > perform a release operation in the thread that actually compiled the > regex (so technically, we don't actually need the __atomic_store_n), > but we still need an acquire operation on the fast path, which is why > we need the atomic. You really don't. The fast path access will race whether it's "atomic" or not. Luckily, it doesn't matter if you get false positives or false negatives, because it will be checked for real under the mutex. Hollis Blanchard Mentor Graphics Emulation Division
On Mon, Aug 13, 2018 at 1:49 PM Hollis Blanchard <hollis_blanchard@mentor.com> wrote: > > On 08/13/2018 01:45 PM, Tom Cherry wrote: > > On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard > > <hollis_blanchard@mentor.com> wrote: > >> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat > >> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to > >> use the (most conservative) __sync_synchronize() primitive provided by those > >> older GCC versions. > >> > >> Fixes https://github.com/SELinuxProject/selinux/issues/97 > >> > >> (Really, no __atomic or __sync operations are needed here at all, since POSIX > >> 4.12 "Memory Synchronization" says pthread_mutex_lock() and > >> pthread_mutex_unlock() "synchronize memory with respect to other threads"...) > > That section means that pthread_mutex_lock() and > > pthread_mutex_unlock() will perform an acquire / release operation > > respectively, so if you're guarding shared data with them, you don't > > need additional memory synchronization. > > > > In this case however, the fast path does not call pthread_mutex_lock() > > and thus there is no acquire operation. pthread_mutex_unlock() will > > perform a release operation in the thread that actually compiled the > > regex (so technically, we don't actually need the __atomic_store_n), > > but we still need an acquire operation on the fast path, which is why > > we need the atomic. > You really don't. The fast path access will race whether it's "atomic" > or not. Luckily, it doesn't matter if you get false positives or false > negatives, because it will be checked for real under the mutex. It does matter if you get false positives. False positives do not lock the mutex and return immediately that the regex has been compiled. Without the acquire operation, it's possible that spec->regex_compiled is true, but the value of spec->regex hasn't been made visible to the processor executing this thread, which results in an error. Keep in mind, this code runs on ARM as well as x86, and ARM doesn't guarantee strong memory ordering. > > Hollis Blanchard > Mentor Graphics Emulation Division >
On 08/13/2018 02:18 PM, Tom Cherry wrote: > On Mon, Aug 13, 2018 at 1:49 PM Hollis Blanchard > <hollis_blanchard@mentor.com> wrote: >> On 08/13/2018 01:45 PM, Tom Cherry wrote: >>> On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard >>> <hollis_blanchard@mentor.com> wrote: >>>> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat >>>> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to >>>> use the (most conservative) __sync_synchronize() primitive provided by those >>>> older GCC versions. >>>> >>>> Fixes https://github.com/SELinuxProject/selinux/issues/97 >>>> >>>> (Really, no __atomic or __sync operations are needed here at all, since POSIX >>>> 4.12 "Memory Synchronization" says pthread_mutex_lock() and >>>> pthread_mutex_unlock() "synchronize memory with respect to other threads"...) >>> That section means that pthread_mutex_lock() and >>> pthread_mutex_unlock() will perform an acquire / release operation >>> respectively, so if you're guarding shared data with them, you don't >>> need additional memory synchronization. >>> >>> In this case however, the fast path does not call pthread_mutex_lock() >>> and thus there is no acquire operation. pthread_mutex_unlock() will >>> perform a release operation in the thread that actually compiled the >>> regex (so technically, we don't actually need the __atomic_store_n), >>> but we still need an acquire operation on the fast path, which is why >>> we need the atomic. >> You really don't. The fast path access will race whether it's "atomic" >> or not. Luckily, it doesn't matter if you get false positives or false >> negatives, because it will be checked for real under the mutex. > It does matter if you get false positives. False positives do not > lock the mutex and return immediately that the regex has been > compiled. Without the acquire operation, it's possible that > spec->regex_compiled is true, but the value of spec->regex hasn't been > made visible to the processor executing this thread, which results in > an error. Well, since there are no objections to this patch, I am happy to have it committed to fix the build problem. :-) Hollis Blanchard Mentor Graphics Emulation Division
On Tue, Aug 14, 2018 at 2:02 PM Hollis Blanchard <hollis_blanchard@mentor.com> wrote: > > On 08/13/2018 02:18 PM, Tom Cherry wrote: > > On Mon, Aug 13, 2018 at 1:49 PM Hollis Blanchard > > <hollis_blanchard@mentor.com> wrote: > >> On 08/13/2018 01:45 PM, Tom Cherry wrote: > >>> On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard > >>> <hollis_blanchard@mentor.com> wrote: > >>>> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat > >>>> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to > >>>> use the (most conservative) __sync_synchronize() primitive provided by those > >>>> older GCC versions. > >>>> > >>>> Fixes https://github.com/SELinuxProject/selinux/issues/97 > >>>> > >>>> (Really, no __atomic or __sync operations are needed here at all, since POSIX > >>>> 4.12 "Memory Synchronization" says pthread_mutex_lock() and > >>>> pthread_mutex_unlock() "synchronize memory with respect to other threads"...) > >>> That section means that pthread_mutex_lock() and > >>> pthread_mutex_unlock() will perform an acquire / release operation > >>> respectively, so if you're guarding shared data with them, you don't > >>> need additional memory synchronization. > >>> > >>> In this case however, the fast path does not call pthread_mutex_lock() > >>> and thus there is no acquire operation. pthread_mutex_unlock() will > >>> perform a release operation in the thread that actually compiled the > >>> regex (so technically, we don't actually need the __atomic_store_n), > >>> but we still need an acquire operation on the fast path, which is why > >>> we need the atomic. > >> You really don't. The fast path access will race whether it's "atomic" > >> or not. Luckily, it doesn't matter if you get false positives or false > >> negatives, because it will be checked for real under the mutex. > > It does matter if you get false positives. False positives do not > > lock the mutex and return immediately that the regex has been > > compiled. Without the acquire operation, it's possible that > > spec->regex_compiled is true, but the value of spec->regex hasn't been > > made visible to the processor executing this thread, which results in > > an error. > Well, since there are no objections to this patch, I am happy to have it > committed to fix the build problem. :-) I have applied this patch. Thanks! Nicolas
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h index 2fa85474..47859baf 100644 --- a/libselinux/src/label_file.h +++ b/libselinux/src/label_file.h @@ -351,8 +351,14 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, * init_routine does not take a parameter, it's not possible * to use, so we generate the same effect with atomics and a * mutex */ +#ifdef __ATOMIC_RELAXED regex_compiled = __atomic_load_n(&spec->regex_compiled, __ATOMIC_ACQUIRE); +#else + /* GCC <4.7 */ + __sync_synchronize(); + regex_compiled = spec->regex_compiled; +#endif if (regex_compiled) { return 0; /* already done */ } @@ -360,8 +366,14 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, __pthread_mutex_lock(&spec->regex_lock); /* Check if another thread compiled the regex while we waited * on the mutex */ +#ifdef __ATOMIC_RELAXED regex_compiled = __atomic_load_n(&spec->regex_compiled, __ATOMIC_ACQUIRE); +#else + /* GCC <4.7 */ + __sync_synchronize(); + regex_compiled = spec->regex_compiled; +#endif if (regex_compiled) { __pthread_mutex_unlock(&spec->regex_lock); return 0; @@ -404,7 +416,13 @@ static inline int compile_regex(struct saved_data *data, struct spec *spec, } /* Done. */ +#ifdef __ATOMIC_RELAXED __atomic_store_n(&spec->regex_compiled, true, __ATOMIC_RELEASE); +#else + /* GCC <4.7 */ + spec->regex_compiled = true; + __sync_synchronize(); +#endif __pthread_mutex_unlock(&spec->regex_lock); return 0; }