Re: [PATCH RFC v3 17/17] fuse: {uring} Pin the user buffer

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 9/4/24 21:40, Jens Axboe wrote:
> On 9/4/24 1:25 PM, Bernd Schubert wrote:
>>
>>
>> On 9/4/24 18:16, Jens Axboe wrote:
>>> On 9/4/24 10:08 AM, Bernd Schubert wrote:
>>>> Hi Jens,
>>>>
>>>> thanks for your help.
>>>>
>>>> On 9/4/24 17:47, Jens Axboe wrote:
>>>>> On 9/1/24 7:37 AM, Bernd Schubert wrote:
>>>>>> This is to allow copying into the buffer from the application
>>>>>> without the need to copy in ring context (and with that,
>>>>>> the need that the ring task is active in kernel space).
>>>>>>
>>>>>> Also absolutely needed for now to avoid this teardown issue
>>>>>
>>>>> I'm fine using these helpers, but they are absolutely not needed to
>>>>> avoid that teardown issue - well they may help because it's already
>>>>> mapped, but it's really the fault of your handler from attempting to map
>>>>> in user pages from when it's teardown/fallback task_work. If invoked and
>>>>> the ring is dying or not in the right task (as per the patch from
>>>>> Pavel), then just cleanup and return -ECANCELED.
>>>>
>>>> As I had posted on Friday/Saturday, it didn't work. I had added a 
>>>> debug pr_info into Pavels patch, somehow it didn't trigger on PF_EXITING 
>>>> and I didn't further debug it yet as I was working on the pin anyway.
>>>> And since Monday occupied with other work...
>>>
>>> Then there's something wrong with that patch, as it definitely should
>>> work. How did you reproduce the teardown crash? I'll take a look here.
>>
>> Thank you! In this specific case
>>
>> 1) Run passthrough_hp with --debug-fuse
>>
>> 2) dd if=/dev/zero of=/scratch/test/testfile bs=1M count=1
>>
>> Then on the console that has passthrough_hp output and runs slow with my
>> ASAN/etc kernel: ctrl-z and kill -9 %
>> I guess a pkill -9 passthrough_hp should also work
> 
> Eerily similar to what I tried, but I managed to get it to trigger.
> Should work what's in there, but I think checking for task != current is
> better and not race prone like PF_EXITING is. So maybe? Try with the
> below incremental.
> 
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 55bdcb4b63b3..fa5a0f724a84 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -121,7 +121,8 @@ static void io_uring_cmd_work(struct io_kiocb *req, struct io_tw_state *ts)
>  	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
>  	unsigned flags = IO_URING_F_COMPLETE_DEFER;
>  
> -	if (req->task->flags & PF_EXITING)
> +	/* Different task should only happen if the original is going away */
> +	if (req->task != current)
>  		flags |= IO_URING_F_TASK_DEAD;
>  
>  	/* task_work executor checks the deffered list completion */
> 

Thanks, just tested this version works fine!
My user of that (patch 16/17) left the fuse ring entry in bad state -
fixed in my v4 branch.

Thanks,
Bernd