Re: [PATCH 3/3] io_uring: refactor io_sq_offload_create()

[Jens Axboe <axboe@kernel.dk>]

On 7/22/21 5:30 PM, Al Viro wrote:
> On Thu, Jul 22, 2021 at 05:06:24PM -0600, Jens Axboe wrote:
> 
>> But yes, that is not great and obviously a bug, and we'll of course get
>> it fixed up asap.
> 
> Another fun question: in do_exit() you have
>         io_uring_files_cancel(tsk->files);
> 
> with
> 
> static inline void io_uring_files_cancel(struct files_struct *files)
> {
>         if (current->io_uring)
> 		__io_uring_cancel(files);
> }
> 
> and
> 
> void __io_uring_cancel(struct files_struct *files)
> {
>         io_uring_cancel_generic(!files, NULL);
> }
> 
> What the hell is that about?  What are you trying to check there?
> 
> All assignments to ->files:
> init/init_task.c:116:   .files          = &init_files,
> 	Not NULL.
> fs/file.c:433:          tsk->files = NULL;
> 	exit_files(), sets to NULL
> fs/file.c:741:          me->files = cur_fds;
> 	__close_range(), if the value has been changed at all, the new one
> 	came from if (fds) swap(cur_fds, fds), so it can't become NULL here.
> kernel/fork.c:1482:     tsk->files = newf;
> 	copy_files(), immediately preceded by verifying newf != NULL
> kernel/fork.c:3044:                     current->files = new_fd;
> 	ksys_unshare(), under if (new_fd)
> kernel/fork.c:3097:     task->files = copy;
> 	unshare_files(), with if (error || !copy) return error;
> 	slightly upstream.
> 
> IOW, task->files can be NULL *ONLY* after exit_files().  There are two callers
> of that; one is for stillborns in copy_process(), another - in do_exit(),
> well past that call of io_uring_files_cancel().  And around that call we have
> 
>         if (unlikely(tsk->flags & PF_EXITING)) {
> 		pr_alert("Fixing recursive fault but reboot is needed!\n");
> 		futex_exit_recursive(tsk);
> 		set_current_state(TASK_UNINTERRUPTIBLE);
> 		schedule();
> 	}
>         io_uring_files_cancel(tsk->files);
> 	exit_signals(tsk);  /* sets PF_EXITING */
> 
> So how can we possibly get there with tsk->files == NULL and what does it
> have to do with files, anyway?

It's not the clearest, but the files check is just to distinguish between
exec vs normal cancel. For exec, we pass in files == NULL. It's not
related to task->files being NULL or not, we explicitly pass NULL for
exec.

-- 
Jens Axboe