[Devel] [PATCH] fs/fuse: move FUSE_S_FAIL_IMMEDIATELY check before kio req send

[Pavel Butsykin]

On 24.01.2019 12:21, Kirill Tkhai wrote:
> On 24.01.2019 12:17, Pavel Butsykin wrote:
>> Yes, I missed this synchronization idea, the check and list_add should
>> be together, will fix.
> 
> Also, __fuse_request_send() may need to be fixed, since it does not look
> as having appropriate in already existing code (I haven't checked deeply).

Yep, a similar race is present. Moreover, there may be kernel panic in
fuse_kill_requests(fc, inode, &fiq->pending) since fiq->pending list is
passed without fiq->waitq.lock.

>   
>> On 24.01.2019 11:45, Kirill Tkhai wrote:
>>> On 23.01.2019 20:22, Pavel Butsykin wrote:
>>>>
>>>> 23.01.2019 16:55, Kirill Tkhai пишет:
>>>>> On 23.01.2019 14:49, Pavel Butsykin wrote:
>>>>>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new
>>>>>> requests. But in case of kio requests it doesn't work because the status check
>>>>>> is located behind kio.op->req_send(). To fix this let's move the status check
>>>>>> before kio.op->req_send().
>>>>>>
>>>>>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because
>>>>>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle().
>>>>>>
>>>>>> Signed-off-by: Pavel Butsykin <pbutsykin at virtuozzo.com>
>>>>> Why is this safe?
>>>>>
>>>>> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files()
>>>>> may become work not as expected.
>>>>
>>>> test_bit is atomic operation. Which type of race do you mean?
>>>
>>> fuse_request_send_background()                                 fuse_invalidate_files()
>>>     test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>>>
>>>                                                                    spin_lock(&fc->lock);
>>>                                                                    list_for_each_entry(ff, &fi->rw_files, rw_entry)
>>>                                                                      set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state);
>>>                                                                    spin_unlock(&fc->lock);
>>>
>>>                                                                    spin_lock(&fc->lock);
>>>                                                                    fuse_kill_requests(fc, inode, &fc->bg_queue);
>>>                                                                    spin_unlock(&fc->lock);
>>>
>>>
>>>     spin_lock(&fc->lock);
>>>     if (fc->connected) {
>>>       fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that
>>>                                                         requests for all immediate files already killed.
>>>
>>>