Re: [PATCH v2 0/5] Complete setup before calling wake_up_new_task() and improve task->comm

[Jens Axboe <axboe@kernel.dk>]

On 3/20/21 1:22 PM, Stefan Metzmacher wrote:
> 
> Am 20.03.21 um 02:24 schrieb Jens Axboe:
>> On 3/19/21 6:00 PM, Stefan Metzmacher wrote:
>>> Hi,
>>>
>>> now that we have an explicit wake_up_new_task() in order to start the
>>> result from create_io_thread(), we should things up before calling
>>> wake_up_new_task().
>>>
>>> There're also some improvements around task->comm:
>>> - We return 0 bytes for /proc/<pid>/cmdline
>>>
>>> While doing this I noticed a few places we check for
>>> PF_KTHREAD, but not PF_IO_WORKER, maybe we should
>>> have something like a PS_IS_KERNEL_THREAD_MASK() macro
>>> that should be used in generic places and only
>>> explicitly use PF_IO_WORKER or PF_KTHREAD checks where the
>>> difference matters.
>>>
>>> There are also quite a number of cases where we use
>>> same_thread_group(), I guess these need to be checked.
>>> Should that return true if userspace threads and their iothreds
>>> are compared?
>>
>> Any particular ones you are worried about here?
> 
> The signal problems and it's used to allow certain modifications
> between threads in the same group.

Gotcha

> With your same_thread_group_account() change it should be all fixed
> magically. I guess the thread also doesn't appear in /proc/pid/tasks/
> any more, correct?

I think it'll still show up there, as they are still linked.

> Would 'top' still hide them with the thread group
> and only show them with 'H' (which show the individual threads)?

I think it'll show them as a thread group still.

> In future we may want to have /proc/pid/iothreads/ instead...

Maybe?

>>> I did some basic testing and found the problems I explained here:
>>> https://lore.kernel.org/io-uring/F3B6EA77-99D1-4424-85AC-CFFED7DC6A4B@kernel.dk/T/#t
>>> They appear with and without my changes.
>>>
>>> Changes in v2:
>>>
>>> - I dropped/deferred these changes:
>>>   - We no longer allow a userspace process to change
>>>     /proc/<pid>/[task/<tid>]/comm
>>>   - We dynamically generate comm names (up to 63 chars)
>>>     via io_wq_worker_comm(), similar to wq_worker_comm()
>>>
>>> Stefan Metzmacher (5):
>>>   kernel: always initialize task->pf_io_worker to NULL
>>>   io_uring: io_sq_thread() no longer needs to reset
>>>     current->pf_io_worker
>>>   io-wq: call set_task_comm() before wake_up_new_task()
>>>   io_uring: complete sq_thread setup before calling wake_up_new_task()
>>>   fs/proc: hide PF_IO_WORKER in get_task_cmdline()
>>>
>>>  fs/io-wq.c     | 17 +++++++++--------
>>>  fs/io_uring.c  | 22 +++++++++++-----------
>>>  fs/proc/base.c |  3 +++
>>>  kernel/fork.c  |  1 +
>>>  4 files changed, 24 insertions(+), 19 deletions(-)
>>
>> I don't disagree with any of this, but view them more as cleanups than
>> fixes. In which case I think 5.13 is fine, and that's where they should
>> go. That seems true for both the first two fixes, and the comm related
>> ones too.
>>
>> If you don't agree, can you detail why? The comm changes seem fine, but
>> doesn't change the visible name. We can make it wider, sure, but any
>> reason to?
> 
> Ok, I guess we want to take only 'fs/proc: hide PF_IO_WORKER in
> get_task_cmdline()' so that ps and top show them as '[iou_mgr_12345]'
> instead of showing the userspace cmd.

That one makes sense, to keep it consistent with earlier to some extent,
and not to have 5.12 be the odd one out compared to later kernels as
well.

> And with your same_thread_group_account() change we only need this hunk:
> 
> @@ -1822,7 +1826,7 @@ void task_dump_owner(struct task_struct *task, umode_t mode,
>         kuid_t uid;
>         kgid_t gid;
> 
> -       if (unlikely(task->flags & PF_KTHREAD)) {
> +       if (unlikely(task->flags & (PF_KTHREAD | PF_IO_WORKER))) {
>                 *ruid = GLOBAL_ROOT_UID;
>                 *rgid = GLOBAL_ROOT_GID;
>                 return;
> 
> From here:
> https://lore.kernel.org/io-uring/97ad63bef490139bb4996e75dea408af1e78fa47.1615826736.git.metze@samba.org/T/#u
> 
> I think we should also take that hunk...
> 
> What do you think?

I'll have to look into that, on the face of it it seems wrong. Why just
assign global root uid/gid for the io worker? It's using the same
credentials as the original task.

-- 
Jens Axboe