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)...
