[PATCH v6 5/9] nptl: Remove g_refs from condition variables

[Carlos O'Donell]

On 12/5/24 3:30 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>> index df54eef6f7..a3d482f80f 100644
>> --- a/sysdeps/nptl/bits/thread-shared-types.h
>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>> @@ -95,8 +95,7 @@ struct __pthread_cond_s
>>  {
>>    __atomic_wide_counter __wseq;
>>    __atomic_wide_counter __g1_start;
>> -  unsigned int __g_refs[2] __LOCK_ALIGNMENT;
>> -  unsigned int __g_size[2];
>> +  unsigned int __g_size[2] __LOCK_ALIGNMENT;
>>    unsigned int __g1_orig_size;
>>    unsigned int __wrefs;
>>    unsigned int __g_signals[2];
> 
> This looks like an unnecessary change to the on-disk layout.  Shouldn't
> we keep the field as __g_refs_unused for now?

You are correct, it is an unnecessary change, but it also follows with the
deletion of the code that accesses the member so it is a correct cleanup.

Upstream glibc has no reason to keep unused fields across release boundaries,
so my currently plan is to keep this patch as-is for the 2.41 release, and we
discussed this offline.

One potential benefit is if retaining the field help downstream in some way
with their implementation of an in-place upgrade strategy, so let me discuss some
of that strategy now.

Upstream glibc considers in-place upgrades are unsupported, and doing them is up to
downstream to resolve.

Even if I were to exclude this patch, there is still a risk here, and backporting
this patch as-is into release branches creates an upgrade hazard for process shared
condition variables. Solving this in the upgrade and downgrade direction is very
difficult, largely owning to the fact that the waiters do not take locks and carry
out a sequence of atomic operations like this:

  wseq++
  wrefs++
  signal-- or {grefs++, wait, signal--, grefs--}
  wrefs--

A post-fix implementation might reserve a high bit somewhere to indicate the fix
is active, perhaps bit 32 in wrefs, and can with good probability (yes, I'm using
that in the probabilistic sense) detect a pre-fix process shared condvar in use.
Then with the detection in place you can switch back to the buggy old version
of the functions which are no worse.

Harder is the downgrade scenario or a process attaching late to a shared memory
segment after an upgrade. In that case the code is going to be the old code, and
we can't change it, so we must present it with data that it does something reasonable
with. We could attempt to show the condvar as locked with no signals available yet.

- Old glibc using new condvar:
  - pthread_cond_init = onwership changes to pre-fix condvar (OK)
  - pthread_cond_signal, pthread_cond_broadcast = hangs, no forward progress (permissible)
  - pthread_cond_*wait = waits forever, no forward progress (permissible)

This leads to hangs, but it is better than corrupted data IMO. Doing this is much harder
though and requires two things:

- Making pre-fix __g_signals[2] zero (not possible without 8 more bytes)
- Making __wrefs bits 0,1,2 as-if locked (easily possible)

This means the new __g_signals post-fix needs to move somewhere else, but it has to move
somewhere the old algorithm won't look, but the old algorithm unconditionally touches
wseq, wrefs, __g_refs[2], and __g_signals[2] which is 28/48 bytes. However, since __g_signals
must be zero, nothing can move into that region except a field we don't use, but the field
we don't use __g_refs, is touched and can't move.

Pre-fix:
struct __pthread_cond_s
{
  __atomic_wide_counter __wseq;                              (modified)
  __atomic_wide_counter __g1_start;                          (8-byte free to reorder)

  unsigned int __g_refs[2] __LOCK_ALIGNMENT;                 (modified)
  unsigned int __g_size[2];                                  (8-byte free to reorder)

  unsigned int __g1_orig_size;                               (4-byte free to reorder)

  unsigned int __wrefs;                                      (modified, LSBs must show as-if locked)
  unsigned int __g_signals[2];                               (must be zero)
};


Post-fix:
struct __pthread_cond_s
{
  __atomic_wide_counter __wseq;                              (")
  __atomic_wide_counter __g1_start;                          (MSBs can be a lock)

  unsigned int __g_refs[2] __LOCK_ALIGNMENT;                 (")
  unsigned int __g_signals[2];                               (8-byte used, new __g_signals)

  unsigned int __g1_orig_size;

  unsigned int __wrefs;                                      (", LSBs show always locked)
  unsigned int unused[2];                                    (", old __g_signals)

  unsigned int __g_size[2];                                  (+8 bytes)
};

Can we get rid of __g_size? No, since cancellations need to be recorded as negative values
here to avoid needing to signal cancelled threads (otherwise the size isn't needed since
we're sequence relative with the __g_signal value).

Without 8 more bytes the downgrade can't be done safely IMO.

Attached is a sketch at how I'd fix this in a backport:

- Use wrefs MSB as the "fix present" bit.
- If the fix is not present then use the pre-fix wait, signal, and broadcast.
- If the fix is present then use the new code.

The patch works on x86_64 and i686, and since wrefs is unsigned int, it can work across
the 32-bit and 64-bit targets (without consideration for 64-bit atomics).

The attached patch is likely what I'll go with to backport the the fix into 2.39 and 2.40
to fix Fedora with and allow the other distributions to copy the example.

In summary:

- Keeping patch 5/9.
- Have a solution for the backport (attached) that we can use later.

-- 
Cheers,
Carlos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: swbz25847-backport.diff
Type: text/x-patch
Size: 61039 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250117/f6e22979/attachment-0001.bin>