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

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, Jul 22, 2021 at 11:30:35PM +0000, Al Viro wrote:

> 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?

PS: processes with ->files == NULL can be observed; e.g. access via procfs
can very well race with exit().  If procfs acquires task_struct reference
before exit(), the object won't get freed until we do put_task_struct().
However, the process in question can get through the entire do_exit(),
become a zombie, be successfull reaped, etc., so its state can be very
thoroughly taken apart while procfs tries to access it.

There the checks for tsk->files == NULL are meaningful; doing them for
current, OTOH, is basically asking "am I rather deep into do_exit()?"

	Once upon a time we had exit_files() done kernel threads.
Not for the last 9 years since 864bdb3b6cbd ("new helper:
daemonize_descriptors()"), though (and shortly after that the entire
daemonize() thing has disappeared - kernel threads are spawned by
kthreadd, and inherit ->files from it just fine).

	Should've killed the useless checks for NULL ->files at the same
time, hadn't...  FWIW, the checks in fget_task(), task_lookup_fd_rcu(),
task_lookup_next_fd_rcu(), task_state(), fs/proc/fd.c:seq_show()
and iterate_fd() are there for good reason.  The ones in unshare_fd(),
copy_files(), fs/proc/task_nommu.c:task_mem() and in exit_files() itself
are noise.  I'll throw their removal in vfs.git#work.misc...

	Anyway, if you intended to check for some(?) kernel threads,
that place needs fixing.  If not, I'd suggest just passing a boolean
to that thing (and giving it less confusing name)...