[PATCH v2 4/4] libio: Fix race access to _IO_FLAGS2_NEED_LOCK

[Adhemerval Zanella]

On 10/05/2021 16:33, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 10/05/2021 16:18, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> I see that my first assumption of moving the _IO_FLAGS2_NEED_LOCK after
>>>> the lock should be suffice.  The data race is not really related to 
>>>> _IO_FLAGS2_NEED_LOCK set in fact, but rather the small window where 
>>>> the _flags2 read and update with _IO_FLAGS2_NEED_LOCK might lost some
>>>> bits update.
>>>
>>> Right.
>>>
>>>> Maybe the comment:
>>>>
>>>> /* The _IO_FLAGS2_NEED_LOCK is required here to enable the non-atomic
>>>>    fast path on function that call _IO_need_lock.  However it requires
>>>>    to be set with stream locked to avoid a potential data race.  */
>>>
>>> s/fast path/path/ I suppose, and “it necessary to set it with the stream
>>> locked”.
>>
>>
>> /* The _IO_FLAGS2_NEED_LOCK is required here to enable the non-atomic
>>    path on function that call _IO_need_lock.  However it is necessary
>>    to set it with the stream locked  to avoid a potential data race.  */
> 
> Meh, I'm confused.  Aren't we disabling the non-atomic path?

With this discussion it seems to me that the _IO_FLAGS2_NEED_LOCK set on
flockfile is indeed superfluous.  On the code:

  { 
    flockfile (arq);

    write (STDERR_FILENO, "(1)\n", sizeof ("(1)\n"));
    feof (arq);

    funlockfile (arq);
  }

  { 
    write (STDERR_FILENO, "(2)\n", sizeof ("(2)\n"));
    feof (arq);
  }

The second feof will always take the lock because _IO_FLAGS2_NEED_LOCK is
sticky, even though this is fine to use the fast non-atomic path (I am
assuming single-thread here).

> 
>>> Why not?  The single-thread optimization would still avoid the atomic
>>> compare-and-set, which should be a significant win on AArch64 and POWER.
>>> There is more work to be done for the single-threaded case compared to
>>> _IO_FLAGS2_NEED_LOCK, but all that is without atomics, so I think those
>>> should be reasonably fast.
>>
>> Because it would solve only for single-thread case, for multithread
>> case the single-thread optimization won't give the function that
>> call _IO_need_lock the correct information to elide the lock.
>>
>> The seqeuence:
>>
>>   flockfile (arq);
>>
>>   feof (arq);
>>   ferror (arq);
>>   ...
>>
>>   funlockfile (arq);
>>
>> Should only incur is atomic operation on flockfile and funlockfile
>> in both single and multi-thread case.  BY tying the feof taking the
>> lock to single-thread optimization it incur in extra atomic operations
>> on multi-thread case.
> 
> There are two relevant locking optimizations: flockfile acquires the
> lock for the current thread.  At that point, the ownership check for the
> recursive lock succeeds.  That check doesn't use an atomic load, so it's
> fast.
> 
> The other optimization is generally enabled via __fsetlocking (arq,
> FSETLOCKING_BYCALLER), which enables the _IO_USER_LOCK flag that
> disables locking in feof and ferror as well (but not for flockfile and
> funlockfile).
> 
> The latter is perhaps not necessary because the application could just
> call flockfile.

This is indeed messy, ideally I think we would like something like:

  int 
  _IO_feof (FILE *fp)
  {
    __flockfile (fp);
    int result = _IO_feof_unlocked ();
    __funlockfile (fp);
    return result;
  }

  void
  __flockfile (FILE *stream)
  {
    _IO_lock_lock (*stream->lock);
  }

  static inline void
  _IO_lock_lock (_IO_lock_t *lock)
  {
    void *self = THREAD_SELF;
    if (lock->owner != self)
      {
        lll_lock (name->lock, LLL_PRIVATE);
        lock->owner = self;
      }
    ++lock->cnt;
  }

The __flockfile should provide single-thread optimization through the
_IO_lock_lock (and the recursive lock).  However we can't really use
__flockfile internally because of __fsetlocking (FSETLOCKING_BYCALLER),
so we need a wrapper to check for _IO_USER_LOCK (which is provided by
the _IO_need_lock wrapper).

The _IO_need_lock and _unlocked call really seems unnecessary, at least
on Linux that already provides a recursive lock with owner.