[PATCHv3] Protect _dl_profile_fixup data-dependency order [BZ #23690]

[Carlos O'Donell]

On 10/17/18 4:12 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 10/15/18 8:57 AM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>> (3) Fence-to-fence sync.
>>>>
>>>> For fence-to-fence synchronization to work we need an acquire and release
>>>> fence, and we have that.
>>>>
>>>> We are missing the atomic read and write of the guard. Please review below.
>>>> Florian mentioned this in his review. He is correct.
>>>>
>>>> And all the problems are back again because you can't do atomic loads of
>>>> the large guards because they are actually the function descriptor structures.
>>>> However, this is just laziness, we used the addr because it was convenient.
>>>> It is no longer convenient. Just add a 'init' field to reloc_result and use
>>>> that as the guard to synchronize the threads against for initialization of
>>>> the results. This should solve the reloc_result problem (ignorning the issues
>>>> hppa and ia64 have with the fdesc updates across multiple threads in _dl_fixup).
>>>
>>> I think due to various external factors, we should go with the
>>> fence-based solution for now, and change it later to something which
>>> uses an acquire/release on the code address later, using proper atomics.
>>
>> Let me clarify.
>>
>> The fence fix as proposed in v3 is wrong for all architectures.
>>
>> We are emulating C/C++ 11 atomics within glibc, and a fence-to-fence sync
>> *requires* an atomic load / store of the guard, you can't use a non-atomic
>> access. The point of the atomic load/store is to ensure you don't have a
>> data race.
> 
> Carlos, I'm sorry, but I think your position is logically inconsistent.

Yes, it *is* logically inconsistent. I agree with you.

However, to *be* logically consistent I'd have to fix all data races in
this code in one go, and I can't, it's too much work.

All I want is for any *changes* to follow C11 semantics, and I think we
can do that without major surgery.

Consider it an ideological flaw that I want everyone to practice following
a consistent memory model and think about these problems in terms of that
memory model, and evaluate patches using that model.

> Formally, you cannot follow the memory model here without a substantial
> rewrite of the code, breaking up the struct fdesc abstraction.  The
> reason is that without blocking synchronization, you still end up with
> two non-atomic writes to the same object, which is a data race, and
> undefined, even if both threads write the same value.

There are two distinct problems here, and each can be handled distinctly.

The first is the problem at hand, that there is a data-dependency issue
with the update of the struct reloc_result structure. We have multiple
threads writing to the reloc_result structure, in general those threads
write the same value (locks are taken _dl_lookup_symbol_x), and while
this is a data race, I don't care about it and we aren't going to fix 
it. The only thing we should do is that a thread that determines the 
reloc_result is initialized should see all the correct value in the 
structure and not a sheared result. That is all that we are fixing
here, call that the "change."

We can follow the memory model far enough to avoid a sheared result
being read out of the struct reloc_result.

We have not fixed the data races that occur when two threads read a
zero addr value and both use non-atomic writes to update reloc_result,
and I don't intend the patch to fix that. I don't require that.

> As far as I can see, POWER is !USE_ATOMIC_COMPILER_BUILTINS, so our
> relaxed MO store is just a regular store, without a compiler barrier.
> That means after all that rewriting, we basically end up with the same
> code and the same formal data race that we would have when we just used
> fences.

That's fine.

The use of the guard+fence-to-fence sync is, from a C11 perspective,
correct. However, I recommend adding a reloc_result->reloc_init and
using that with release/acquire loads.

> This is different for USE_ATOMIC_COMPILER_BUILTINS architectures, where
> we do use actual atomic stores.  But for !USE_ATOMIC_COMPILER_BUILTINS,
> the fence-based approach is as good as we can get, with or without
> breaking the abstractions.

We can do better.

> So as I said, given the constraints we are working under, we should go
> with the solution based on fences, and have that tested on Aarch64 as
> well.

I whole heartedly appreciate a pragmatic approach to these problems, but
I still challenge that we can do better without much more work.

>>> I don't want to see this bug fix blocked by ia64 and hppa.  The proper
>>> fix needs some reshuffling of the macros here, or maybe use an unused
>>> bit in the flags field as an indicator for initialization.
>>
>> The fix for this is straight forward.
>>
>> Add a new initializer field to the reloc_result, it's an internal data
>> structure. It can be as big as we want and we can optimize it later.
>>
>> You don't need to do any big cleanups, but we *do* have to get the
>> synchronization correct.
> 
> See above; I don't think we can get the synchronization formally
> correct, even with any level of cleanups.  In the data race case, we
> would have
> 
>   atomic acquire MO load of initializer field
>   non-atomic writes to various struct fields
>   atomic release MO store to initializer field
> 
> in each thread.  That's still undefined behavior due to the blocking
> stores in the middle.

That's fine. The changes made are correct, even if the whole algorithm
itself is not.

> Let me reiterate: Just because you say our atomics are C11, it doesn't
> make them so.  They are syntactically different, and they are not
> presented to the compiler as atomics for !USE_ATOMIC_COMPILER_BUILTINS.
> I know that you and Torvald didn't consider this a problem in the past,
> but maybe you can reconsider your position?

My position is unchanged.

If I could summarize our positions, I would write:

(1) The "Pragmatic approach"

- Since we don't have C11 atomics, we should just use the fences because
  they fix the data dependency issue, and stop there. We need not go any
  further until we are ready to fix the underlying algorithm of the result
  updates and *then* we can follow C11.

(2) The "Incremental C11 approach"

- Assume we are C11, and take an incremental approach where we fix the
  data dependency issue using correct synchronization primitives, even
  if it doesn't solve all of the data races.

Did I summarize your position accurately?

I prefer (2).

Tulio can have a preference also.

I spoke to Tulio on IRC and he says he has a working v4 in build-many-glibcs.

I figure we'll commit something probably tomorrow for this.

Just for the record I'm attaching my WIP v4 so you can see what I mean about
the solution. Yes, I did make the structure 4 bytes larger, and that will have
a real impact, but it removes the dependency on the size of the function pointer,
and I like that for future maintenance, and we'll likely rewrite the whole struct
when we fix 23790 to get a more optimal layout.

-- 
Cheers,
Carlos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: swbz23690.patch
Type: text/x-patch
Size: 14510 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20181018/fb682507/attachment.bin>