Re: [PATCH v2 1/1] ntdll: Fix reading stale Information from IOSB.

[Jin-oh Kang]

On Tue, May 31, 2022, 7:52 AM Zebediah Figura wrote:
> On 5/30/22 14:20, Jinoh Kang wrote:
> > -
> > - /* GetOverlappedResultEx() skips waiting for the event/file handle
>
> if the
> > -
> > * Status field indicates completion, and returns the result from
>
> the
> > -
> > * Information field.
> > -
> > *
> > -
> > * Hence, we have to ensure that writes to the Information field
>
> are seen
> > -
> > * from the other threads *before* writes to the Status field.
> > -
> > *
> > -
> > * Otherwise, a race condition results: if writing the Status field
>
> has
> > -
> > * been completed but the subsequent update to the Information
>
> field has
> > -
> > * not, the application may end up reading stale value from it.
> > -
> > *
> > -
> > * The race condition can be especially problematic with
>
> applications that
> > -
> > * use the HasOverlappedIoCompleted() macro in a tight loop to
>
> busy-wait
> > -
> > * for completion of overlapped I/O.
> > -
> > */
> > - MemoryBarrier();  /* TODO: consider replacing with a store-release.
>
> */
> > -
> >
> > *(volatile NTSTATUS *)status_ptr = status;
> > }
> >
> > static inline client_ptr_t iosb_client_ptr( IO_STATUS_BLOCK *io )
>
> Unpaired MemoryBarrier() is pretty much always wrong. (In fact, I think
> unpaired anything would be, here.)

By pairing, do you mean that another MemoryBarrier() should follow the
status write?
> It also doesn't seem clear to me that it's actually better than a
> store-release, as you point out. Ideally what we want is ReadAcquire()
> and WriteRelease(), but even setting that aside, a pair of interlocked
> operations will probably be an improvement by themselves.

As in, convert both status and information writes to interlocked
operations, or replace the barrier with a dummy atomic op?
>