Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH V3 5/9] fork: add helper to clone a process
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     geert@linux-m68k.org, vverma@digitalocean.com, hdanton@sina.com,
        hch@infradead.org, stefanha@redhat.com, jasowang@redhat.com,
        mst@redhat.com, sgarzare@redhat.com,
        virtualization@lists.linux-foundation.org, axboe@kernel.dk,
        linux-kernel@vger.kernel.org
References: <20211004192128.381453-1-michael.christie@oracle.com>
 <20211004192128.381453-6-michael.christie@oracle.com>
 <20211005125018.5nzmhwdxivyye5on@wittgenstein>
From:   Mike Christie <michael.christie@oracle.com>
Message-ID: <00d724df-5781-035f-54ad-e0432ec92646@oracle.com>
Date:   Tue, 5 Oct 2021 12:10:55 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <20211005125018.5nzmhwdxivyye5on@wittgenstein>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Skw5aG8xK0ovODFFNkcvTEVGZ0pyZEdQNFlhdVZmZlhqWU43djVPclN3V2Jn?=
 =?utf-8?B?NmxLUnp0VDZiZFFOTlcvSWNrUXpHMDZYTm9TWnRaM1VXUGRXREkxVkUwdWpy?=
 =?utf-8?B?MTJ3Wk1ScmtyTG9YRHJ4OWt5eG5oVkhzMTdpUTY1alJ3MUtwUGd1TnN0RVo2?=
 =?utf-8?B?b2c2QWZCamIxVVpqMGFDcjhUdzBDSUJnVDlJaEt3azlWQlpvZThKSFVxd0lF?=
 =?utf-8?B?RzhReUk2Y1NYdG5oMEQrckxURmczWllPaUg0V1B4Q2dZaEpRUTdrb1pWK2NL?=
 =?utf-8?B?VS9haVU4a0ZWRHdkbFVyTUluUmdqSDVhS0RxRnhONXg2RnBXQWxXUlAyWGEv?=
 =?utf-8?B?K1M5ZGdyQWRRODZuOUh6NDZKcVRtSlFybzRtYktReDJuNTVGdXFjUlF1TDdL?=
 =?utf-8?B?UGlpOWl3T0ZUT3lwSUhoVkk2L1I4L28wMzhvSldGaVJJMlY1aW9RMGZlOFdF?=
 =?utf-8?B?Sk5leGQ4MlkrNTZzVlhNK05majhuYXFqek9PV09uYVZ5S0xoSmtrTS83VU9E?=
 =?utf-8?B?WVFIL0hUNkR5TW1iS3JtUnA0Z3Y1c0NPWFM5eXpOZCtSVGZoMnZnMlVCVFZD?=
 =?utf-8?B?dldxMk94Njl5YjFTcUFQZWFwekxwbk8yQWg0RmhmaGlpbXdIbkRtWkFOSU9C?=
 =?utf-8?B?ZGtrMDR0a1hEcVhGUXpib29nZkZHRjJaU0Y3T01xeHFsaTk4MThFNHV5Q3Yw?=
 =?utf-8?B?NjRtMEh4VHZsTzZhZmx5cndHMHZRZHhHeHVtRUpzbHVyZDFObHpmeS84N2ZK?=
 =?utf-8?B?SVdKSjI0RVRKYkhRT1NjajBaK3JEclBIQzE3THVSTnY2TXlNOStQSjNoUmVw?=
 =?utf-8?B?UkNZNGp1ZE0xQ29HMjFtY05vSEIyTE5CUnc2OUU1dVZFcHJrUUZONjcwNjhG?=
 =?utf-8?B?NVFnQnR1aEkwN0hWYS91emd2elB0QmNQV2pCcEszOFhFR2V3UGplUzNXR3FR?=
 =?utf-8?B?TWg0V0FuSmVaUWNNRzNCMFJ4UTFjTUhxQ29FcGsxd0NmaHgycEJKSjlVN1Q1?=
 =?utf-8?B?WjIyVm1kc20wUWIwRWllT0FKeUF5SlUxeXdIL1B0eFBMUEk3Z2R1c0xDNmts?=
 =?utf-8?B?Y3ZrYXVYRUNWTkhjZGxjSW42Q2Y4SmRIRDdueTN3eGFqRnRxNXBMdU5CVGxQ?=
 =?utf-8?B?TW5VMVBBTHhmbUdia2NiK1Y0UVFkd2Y3bXlJTUJlcGhwMG5Vd0JqeVdvc2Ft?=
 =?utf-8?B?Q3E1bjBLZGUrMzNuejV0WEJLL1BqZVdPSkN4NVdncjdjSDR6VDJsQ0pPZk4x?=
 =?utf-8?B?N1E4RVZObFJqSG9zUGo5SlFGc0xTOTFKMFFsZVpTM2RjZjlNOHQwc1RKWVA0?=
 =?utf-8?B?TFFJbW9jRXRJQnVUbjk2dzRFVmJUSFVhNUNybGdDTkhFdWxGUHdYRmpjNFpV?=
 =?utf-8?B?U1hXNGhWVDliZEVwaGpPUk1CbE9JWHAwalBoMjF2Uks1R3JNVjhzbGg1QzlJ?=
 =?utf-8?B?WEIwMno3ZkFwcm41WVJkbjIxOXhZUmlyWjN4ZWJzRHpBLzBVT3pUbEdrVlA0?=
 =?utf-8?B?dWswQVVIYVV3bklDSkI5UnFUV25SU0Y3b2FMWDRFc3E2ekp1cnN0Qlk1SzhQ?=
 =?utf-8?B?U3grRmsyNjV4Wk8rYlZoa05MR0wxbWlPdy9KSXJwTkpkdENFL1FtYTZkbysz?=
 =?utf-8?B?TWFoa2ZKK0RBL3RFRGRHbDFiL2daRUgxeEk5NW9kWTZZUkFYeng0SXl2ZTB5?=
 =?utf-8?B?OHFLZklPUm9YUitseTgvMHJCMWM2Wk1vM09CSkxHZHg1SXZUTlBZY3VsTDRx?=
 =?utf-8?Q?Nd5zOEoJL9vj7pC1SSqUUgPsLge9KNWs1ObjT9F?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 33cf1541-ee83-4671-7f66-08d988231936
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2021 17:10:58.0229
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mVfqzO6n47XYqDPlazFF8XMzjLMvn7ICGn5ZMcxrrdidn3WzOCMkTmlHOBh3vX+2KcIm3tgXWsgey01TP9QI+9gfknl04FZt0HrgiIJISxI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5293
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10128 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 adultscore=0
 bulkscore=0 suspectscore=0 malwarescore=0 spamscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2110050103
X-Proofpoint-GUID: NDwOHYm8kU_m_O9qk6ulGo5vcr5KACZx
X-Proofpoint-ORIG-GUID: NDwOHYm8kU_m_O9qk6ulGo5vcr5KACZx
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/5/21 7:50 AM, Christian Brauner wrote:
> On Mon, Oct 04, 2021 at 02:21:24PM -0500, Mike Christie wrote:
>> The vhost layer has similar requirements as io_uring where its worker
>> threads need to access the userspace thread's memory, want to inherit the
>> parents's cgroups and namespaces, and be checked against the parent's
>> RLIMITs. Right now, the vhost layer uses the kthread API which has
>> kthread_use_mm for mem access, and those threads can use
>> cgroup_attach_task_all for v1 cgroups, but there are no helpers for the
>> other items.
>>
>> This adds a helper to clone a process so we can inherit everything we
>> want in one call. It's a more generic version of create_io_thread which
>> will be used by the vhost layer and io_uring in later patches in this set.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>> ---
>>  include/linux/sched/task.h |  6 ++++-
>>  kernel/fork.c              | 48 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
>> index e165cc67fd3c..ba0499b6627c 100644
>> --- a/include/linux/sched/task.h
>> +++ b/include/linux/sched/task.h
>> @@ -87,7 +87,11 @@ extern void exit_files(struct task_struct *);
>>  extern void exit_itimers(struct signal_struct *);
>>  
>>  extern pid_t kernel_clone(struct kernel_clone_args *kargs);
>> -struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node);
>> +struct task_struct *create_io_thread(int (*fn)(void *i), void *arg, int node);
>> +struct task_struct *kernel_worker(int (*fn)(void *), void *arg, int node,
>> +				  unsigned long clone_flags, u32 worker_flags);
>> +__printf(2, 3)
>> +void kernel_worker_start(struct task_struct *tsk, const char namefmt[], ...);
>>  struct task_struct *fork_idle(int);
>>  struct mm_struct *copy_init_mm(void);
>>  extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 98264cf1d6a6..3f3fcabffa5f 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -2540,6 +2540,54 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
>>  	return copy_process(NULL, 0, node, &args);
>>  }
>>  
>> +/**
>> + * kernel_worker - create a copy of a process to be used by the kernel
>> + * @fn: thread stack
>> + * @arg: data to be passed to fn
>> + * @node: numa node to allocate task from
>> + * @clone_flags: CLONE flags
>> + * @worker_flags: KERN_WORKER flags
>> + *
>> + * This returns a created task, or an error pointer. The returned task is
>> + * inactive, and the caller must fire it up through kernel_worker_start(). If
>> + * this is an PF_IO_WORKER all singals but KILL and STOP are blocked.
>> + */
>> +struct task_struct *kernel_worker(int (*fn)(void *), void *arg, int node,
>> +				  unsigned long clone_flags, u32 worker_flags)
>> +{
>> +	struct kernel_clone_args args = {
>> +		.flags		= ((lower_32_bits(clone_flags) | CLONE_VM |
>> +				   CLONE_UNTRACED) & ~CSIGNAL),
>> +		.exit_signal	= (lower_32_bits(clone_flags) & CSIGNAL),
>> +		.stack		= (unsigned long)fn,
>> +		.stack_size	= (unsigned long)arg,
>> +		.worker_flags	= KERN_WORKER_USER | worker_flags,
>> +	};
>> +
>> +	return copy_process(NULL, 0, node, &args);
>> +}
>> +EXPORT_SYMBOL_GPL(kernel_worker);
>> +
>> +/**
>> + * kernel_worker_start - Start a task created with kernel_worker
>> + * @tsk: task to wake up
>> + * @namefmt: printf-style format string for the thread name
>> + * @arg: arguments for @namefmt
>> + */
>> +void kernel_worker_start(struct task_struct *tsk, const char namefmt[], ...)
>> +{
>> +	char name[TASK_COMM_LEN];
>> +	va_list args;
> 
> You could think about reporting an error from this function if
> KERN_WORK_USER isn't set or only call the below when KERN_WORK_USER is
> set. Both options are fine.
> 

I'm not sure how to handle this comment, because I might have misread
an older comment or made it up in my head.

KERN_WORK_USER is only set on the kernel_clone_args, so at this point we
don't have that struct available anymore.

I didn't add a new PF_KTHREAD_WORK_USER flag to sched.h, because I thought
I had got a review comment to not add another PF flag for this. However, I
can't seem to find that comment now so I'm not sure if maybe I misread a
comment or made it up.

If it's ok I could add a PF_KTHREAD_WORK_USER, then do a:

WARN_ON(!(tsk->flags & PF_KTHREAD_WORK_USER)

so future developers get loud feedback they are doing the
wrong thing right away.