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

[Adhemerval Zanella]

On 10/05/2021 18:42, Adhemerval Zanella wrote:
> 
> 
> 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.
> 

Sigh... we still need for the open_memstream case (de895ddcd7fc45c).
Maybe we make open_memstream use a invalid owner value (either NULL
or -1) to force the _IO_lock_lock always call lll_lock