Re: [PATCH] io_uring: fix NULL pointer dereference for async cancel close

[Pavel Begunkov <asml.silence@gmail.com>]

On 19/01/2021 13:39, Pavel Begunkov wrote:
> On 19/01/2021 13:12, Joseph Qi wrote:
>> On 1/19/21 7:45 PM, Pavel Begunkov wrote:
>>> On 19/01/2021 08:00, Joseph Qi wrote:
>>>> On 1/19/21 10:38 AM, Pavel Begunkov wrote:
>>>>> On 19/01/2021 01:58, Joseph Qi wrote:
>>>>>>> Hmm, I hastened, for files we need IO_WQ_WORK_FILES,
>>>>>>> +IO_WQ_WORK_BLKCG for same reasons. needs_file would make 
>>>>>>> it to grab a struct file, that is wrong.
>>>>>>> Probably worked out because it just grabbed fd=0/stdin.
>>>>>>>
>>>>>>
>>>>>> I think IO_WQ_WORK_FILES can work since it will acquire
>>>>>> files when initialize async cancel request.
>>>>>
>>>>> That the one controlling files in the first place, need_file
>>>>> just happened to grab them submission.
>>>>>
>>>>>> Don't quite understand why we should have IO_WQ_WORK_BLKCG.
>>>>>
>>>>> Because it's set for IORING_OP_CLOSE, and similar situation
>>>>> may happen but with async_cancel from io-wq.
>>>>>
>>>> So how about do switch and restore in io_run_cancel(), seems it can
>>>> take care of direct request, sqthread and io-wq cases.
>>>
>>> It will get ugly pretty quickly, + this nesting of io-wq handlers
>>> async_handler() -> io_close() is not great...
>>>
>>> I'm more inclined to skip them in io_wqe_cancel_pending_work()
>>> to not execute inline. That may need to do some waiting on the
>>> async_cancel side though to not change the semantics. Can you
>>> try out this direction?
>>>
>> Sure, I'll try this way and send v2.
> 
> There may be a much better way, that's to remove IO_WQ_WORK_NO_CANCEL
> and move -EAGAIN section of io_close() before close_fd_get_file(),
> so not splitting it in 2 and not keeping it half-done.

I believe it is the right way, but there are tricks to that. I hope
you don't mind me and Jens hijacking taking care of it. Enough of
non-technical hassle expected...

Thanks for reporting it!

> 
> IIRC, it was done this way because of historical reasons when we
> didn't put more stuff around files, but may be wrong.
> Jens, do you remember what it was?

-- 
Pavel Begunkov