[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Zebediah Figura]

On 2/3/22 23:50, Jinoh Kang wrote:
> On 2/4/22 03:20, Zebediah Figura wrote:
>> On 2/3/22 09:28, Jinoh Kang wrote:
>>> On 2/3/22 08:45, Zebediah Figura wrote:
>>>> On 1/29/22 01:47, Jinoh Kang wrote:
>>>
>>> ...
>>>
>>>>> diff --git a/server/async.c b/server/async.c
>>>>> index 7aef28355f0..e169bb23225 100644
>>>>> --- a/server/async.c
>>>>> +++ b/server/async.c
>>>>> @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
>>>>>             return async->wait_handle;
>>>>>         }
>>>>>     +    if (!async->pending && get_error() == STATUS_ALERTED)
>>>>
>>>> Why "!async->pending"?
>>>
>>> The rationale is twofold:
>>>
>>> 1. async_handoff() can be called with "async->pending = 1".
>>>      Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
>>>      Also, there exists another conditional statement below which explicitly tests for "async->pending".
>>
>> Yes, but they don't set the last error to STATUS_ALERTED, do they?
> 
> They don't (yet), hence the idea of "assert()".
> I suppose it's better to quit early so it'd be easiler to debug if STATUS_ALERTED _somehow_ finds its way to sneak in and screw up everything due to missing APC...
> 
>>
>> Anything that's triggered below should only apply to asyncs whose initial status is known. Consider that "unknown_status" not only means the initial NTSTATUS is unknown, but it also means the initial pending state is unknown.
> 
> It appears that existing code dictates that the initial pending state of unknown_status async is, well, "not pending."  More on it below.
> 
>>
>>>
>>> 2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.
>>>
>>>      - There exists an established invariant that "async->unknown_status" implies "!async->pending".
>>>        It would be invalid that "async->unknown_status && async->pending", since an async
>>>        "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.
>>>
>>>      - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
>>>        Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.
>>>
>>> That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
>>> However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
>>> simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
>>> I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...
>>
>> If you consider the above, "async->pending" is semantically undefined if async->unknown_status is 1. We shouldn't depend on it being one thing or the other, and checking for that (even in an assert) I think gives the wrong idea that we care.
> 
> On the contrary, there are a total of three places in async.c that do rely on the fact that "async->unknown_status" implies "!async->pending".
> For example, note the following code in async_set_result():
>>         /* don't signal completion if the async failed synchronously
>>          * this can happen if the initial status was unknown (i.e. for device files)
>>          * note that we check the IOSB status here, not the initial status */
>>         if (async->pending || !NT_ERROR( status ))
>>         {
>>             if (async->data.apc)
>>             {
>>                 apc_call_t data;
>>                 memset( &data, 0, sizeof(data) );
>>                 data.type         = APC_USER;
>>                 data.user.func    = async->data.apc;
>>                 data.user.args[0] = async->data.apc_context;
>>                 data.user.args[1] = async->data.iosb;
>>                 data.user.args[2] = 0;
>>                 thread_queue_apc( NULL, async->thread, NULL, &data );
>>             }
>>             else if (async->data.apc_context && (async->pending ||
>>                      !(async->comp_flags & FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)))
>>             {
>>                 add_async_completion( async, async->data.apc_context, status, total );
>>             }
>>
>>             if (async->event) set_event( async->event );
>>             else if (async->fd) set_fd_signaled( async->fd, 1 );
>>         }

Well, no, not really. Note that async->unknown_status actually has to be 
zero here. The point is that *usually* we won't get into 
async_set_result() if (!async->pending && NT_ERROR( status )), because, 
well, we don't need to. The server request (read, write, ioctl, 
whatever) will return failure and no async handle, because we don't need 
to fill the IOSB or signal completion in any way.

Note also that "was async->unknown_status ever true?" is also completely 
orthogonal of "is async->pending currently true?". The latter is set 
according to whether the IRP was marked pending via IoMarkIrpPending(), 
and it's supposed to have the exact same semantics.

> 
> There's another one in async_terminate():
>>         /* this can happen if the initial status was unknown (i.e. for device
>>          * files). the client should not fill the IOSB in this case; pass it as
>>          * NULL to communicate that.
>>          * note that we check the IOSB status and not the initial status */
>>         if (NT_ERROR( status ) && (!is_fd_overlapped( async->fd ) || !async->pending))
>>             data.async_io.sb = 0;
>>         else
>>             data.async_io.sb = async->data.iosb;

Same deal here.

> 
> (The other one is in async_handoff, but the check is not done if unknown_status is set anyway.)
> 
> Notice that they are using "!async->pending" as a covering condition for "async->unknown_status".
> So I'd argue it's not semantically undefined at all; the semantics of "unknown_status" actually _entails_ that the async is not "pending".
> It turns out this plays a significant role in the lifecycle of IRP-based asyncs.  Assuming the device file is not opened for synchronous I/O, the process is as follows:
> 
> 1. Initially, queue_irp() marks the IRP async as having unknown status.
> 
> 2. async_handoff() sees that the unknown_status flag is set, and keeps the wait handle open. The wait handle returned to the client.
> 
> 3. The client starts to block on the wait handle.
> 
> 4. Later, the driver signals the initial status of the IRP as STATUS_PENDING (the IRP has been queued internally by the driver).
> 
> 5. The get_next_device_request handler sets the initial status and pending state of async, and wakes up the client.
> 
> 6. The client stops waiting and returns the status, which is possibly STATUS_PENDING.
> 
> In steps 1-4, the async stays in the "not pending" state.  Had the async been in "pending" state and the I/O failed, step 5 would erroneously trigger APC / IOCP notification mechanism since the failure is treated as asynchronous instead of synchronous.
> 
> In this regard, I would argue that the pending flag is best documented as "has the async entered the asynchronous phrase, so that the status would be reported to the client even if it failed?".
> 
> In conclusion, we depend on it and we should care.

Except that I don't think that's how you should be thinking about 
async->pending. async->pending is meant to answer the question "will we 
fill the IOSB and signal completion even if the async fails?" If 
async->unknown_status, the answer to that is "how should I know? we 
haven't got that far yet".

The only times we ever care, or should care, about that flag, are when 
we're determining whether (or sometimes, as a consequence, how) to queue 
completion. We shouldn't care about it if the initial status isn't even 
known yet.