Message-ID: <a171238e-d731-1c22-af72-0f7faf7f4bea@oracle.com>
Date:   Fri, 17 Dec 2021 16:08:14 -0600
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.4.0
Subject: Re: [PATCH V6 01/10] Use copy_process in vhost layer
Content-Language: en-US
To:     "Eric W. Biederman" <ebiederm@xmission.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,
        christian.brauner@ubuntu.com, axboe@kernel.dk,
        linux-kernel@vger.kernel.org
References: <20211129194707.5863-1-michael.christie@oracle.com>
 <87tuf79gni.fsf@email.froward.int.ebiederm.org>
From:   michael.christie@oracle.com
In-Reply-To: <87tuf79gni.fsf@email.froward.int.ebiederm.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S1ZuRWRodWx2QXFCOWc2SDVjWERLblBKVkkxSmhaaUFhK1ptdW9GS3EvckQr?=
 =?utf-8?B?SDM3bHNRem9jYUZTYlBjbHErVnZQMldxVW9LTEpjd1pVZG1oS1hXM0RVUFRp?=
 =?utf-8?B?MjhKR1FBZERqSWRGNmdPTVVKa2ZLOU5WMlhVQUliRjdvdlY4dDcrQVZmaE13?=
 =?utf-8?B?NElLRlhnQ0hZQ2ZEQ3J1WDRiaWF0SnZyd1ZVVWVLR0R6OVlmNlZqY2Vzb1JI?=
 =?utf-8?B?c05aWjIzazZqL0x2YVVmTXhLckVuWjNNMXlNRWtzTnVoaVVJa1daU0Q2ZHdY?=
 =?utf-8?B?WEpuV0VGakQvdElWYjF2dEtTWGFtVUIraEZRWFlHRjI4cEcrOEhXQTA4ZmVs?=
 =?utf-8?B?Y0FhK2tncmFaT3RRMGU1d0ZXbkRsZEZNc0JzUWpldjZPbFh1dWs2YjdFcmF0?=
 =?utf-8?B?blhEcGU4NU5tL3J5bHpNRHJXa25JRDByNzd1ZUYzZTNPVGpYNzZOUGUwR2th?=
 =?utf-8?B?UDRESFY3eTZDa0dPc05XQ1VtbTQzSGNWaVJwaTl4akM4U1ZxcGc2clRwbjl4?=
 =?utf-8?B?VkQzVjRKUi9FWDFzRnI5U1VvL0l2NU5WUWtuMHBnTVNuNUV3MjhLci9wYjhk?=
 =?utf-8?B?UEVsenZvR01LNkwwLzlraXVNbzVJYnJOQ2c1OXZzL05ZT1hEVng2a1FyMEg3?=
 =?utf-8?B?blhobFhKd1NZUVVWQ1loUzhFcWluV0ExNHJudHU5SGxwTEtaNTFrTHUraFEv?=
 =?utf-8?B?SzVvL005NjNDNFppaTZQdjMrV2lqeHU3MTdpUWdJNURrNkRxRXF4a1BMSTA0?=
 =?utf-8?B?YzkzS0FYamFaeTY5YVo5OU9pUmtDN2lJSE9FZ0E0VmgwNW5XY1M3eEpYY05F?=
 =?utf-8?B?c0UrcWg1b2Z0dUN1Y0xSQzNFeUNHRVcwb1ErQUNFNGVORHBHdy9xKzU0MjlY?=
 =?utf-8?B?S2t6S0tNNzQ5S3p6azh1cUhxR0ExOHh3Vld2UkpycXB3Z0drS3JGcTcyNHBk?=
 =?utf-8?B?NWFOeVkyWDBMK0VUeS9oUmFlMEpYZm1TRDZ4RUZRWkpGV2lJWlB2bDNlR1da?=
 =?utf-8?B?Szdsanl6bEVMRjZGOE53UkV2WVdyUis4ZTF0bll6QkR6NGJjSUFwQWtURDk5?=
 =?utf-8?B?UnBHV2VNeG4xUE14T3RNSU9GT3dxSVFmNW01bStHT0diZzA2enhtM3dSMU9r?=
 =?utf-8?B?dVY0M1B6UzB0Z3hlVzhvdDNjai9wMlpGVmF6TFp4SDV6VlZqY3dPUFgvUlAx?=
 =?utf-8?B?SytzVG1tU2J1bnFlTEo0aDlaY090YUpSdWRtVjNTV3lxdElTeGhJOTFnSllL?=
 =?utf-8?B?eS9QaXl6VjBXOGMzcldrVXVrZ3BjNEFGd1lBcUMwaklLaU5RdllJcEF6ZHJS?=
 =?utf-8?B?MWdSODFDeHZuajZUV1NoY2N0V05HczFIRis3dys3Q2Z0ZkloVWRobGxNMW51?=
 =?utf-8?B?NnlPUTJGck1MZ0lOTmN5NnZodE8wZXZlT1NWMWh3ckNzbkM0Q3d1MGpRKy9w?=
 =?utf-8?B?MHdJcVI5U1hYR04yN0p3U0N0SVJYN3RZRVdmaDQwaVB6MExZWFQwTGRhSVB6?=
 =?utf-8?B?Z1F4VnBucnZ5WlNYbXRpUFY3YjRBR1BQc0phZ05GR0RPOWgvYUxmNFpnNXJ2?=
 =?utf-8?B?cGlzNzBwcGl1V3NSUitxbGtNZVVFcVg2OWYzKytQTlJMeCt3SEZSbklHRzM1?=
 =?utf-8?B?bHhHci9MaW9Ra3JMM1R5S0MreEZvcDZtNlliZjZWa3ZNOVZNeHFtVGQwcnNG?=
 =?utf-8?B?UzRwQUFwd0NIYVpkVGtLbXJHL0tuTEZsMVJnd3N0cmVHdlBUYjlCRTVQaFBz?=
 =?utf-8?B?d0cySEFBekVJWHl6TlhQTThXOEp4U0o5WlljSVFQODdOWkpwUHUrWUNac0tw?=
 =?utf-8?B?bkhNUDJIempXcUR3dXhScEdGVHl1K2JLcXhRdkhWZDFYUDBnVmY5SmlNQzFL?=
 =?utf-8?B?Q1E3czZtNFJhcjFTRDRtZmQxb0p3QVl3cldWQ0o0OG5XYk44WDdBNkhkTlB1?=
 =?utf-8?B?anAvRFpTbkxLL1JYQXhTenNvb01VM1JrKzRnR1JobGZUR1YyS1EzMW94SW5O?=
 =?utf-8?B?Z3BMM3luZjFUay8wSXd6NXNYckZxUkJKMlBXSWhqWUhsbEhudkFSelhRL1Nt?=
 =?utf-8?B?Q0hoUnRiMU84TDhpY3EyZzRIc3JQVlJEaWFjUmlYSFBTalhlYitITkdheUI1?=
 =?utf-8?B?Z2FKSk5Dc0FVbDVUODIybUJiam9JdkVqeFRVaUlPUjAzQXZ6dnlVN3NoM2pw?=
 =?utf-8?B?akFoMVA5allGc1Z0RENLTEMrL1loR1JjSFFtK0t4T01aMmg1d1IzU2ZRSUNn?=
 =?utf-8?Q?spx28tucBJO+QS4kwjBxunzZDgeyUqG5M+qPJwEZFI=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bb06a791-7df4-4268-cf3e-08d9c1a9ba0e
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Dec 2021 22:08:16.7181
 (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: W2ksqaI0hYBOkDFd+AEXJcruNrsco43Ggvcp20IEVD5wT7Thv9unQ/D/3MZkYvwDyNB9WBXlil2D/ZBtDUvQR5iWwq7Dp1RDK0aXC+gBqIY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3547
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10201 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 malwarescore=0 bulkscore=0 spamscore=0 mlxlogscore=778 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2112170123
X-Proofpoint-ORIG-GUID: -SDVxUYVhg1fTyOyKBHyqlUcwiiFWB1X
X-Proofpoint-GUID: -SDVxUYVhg1fTyOyKBHyqlUcwiiFWB1X
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 12/17/21 1:26 PM, Eric W. Biederman wrote:
> Mike Christie <michael.christie@oracle.com> writes:
> 
>> The following patches made over Linus's tree, allow the vhost layer to do
>> a copy_process on the thread that does the VHOST_SET_OWNER ioctl like how
>> io_uring does a copy_process against its userspace app. This allows the
>> vhost layer's worker threads to inherit cgroups, namespaces, address
>> space, etc and this worker thread will also be accounted for against that
>> owner/parent process's RLIMIT_NPROC limit.
>>
>> If you are not familiar with qemu and vhost here is more detailed
>> problem description:
>>
>> Qemu will create vhost devices in the kernel which perform network, SCSI,
>> etc IO and management operations from worker threads created by the
>> kthread API. Because the kthread API does a copy_process on the kthreadd
>> thread, the vhost layer has to use kthread_use_mm to access the Qemu
>> thread's memory and cgroup_attach_task_all to add itself to the Qemu
>> thread's cgroups.
>>
>> The problem with this approach is that we then have to add new functions/
>> args/functionality for every thing we want to inherit. I started doing
>> that here:
>>
>> https://urldefense.com/v3/__https://lkml.org/lkml/2021/6/23/1233__;!!ACWV5N9M2RV99hQ!eIaEe9V8mCgGU6vyvaWTKGi3Zlnz0rgk5Y-0nsBXRbsuVZsM8lYfHr8I8GRuoLYPYrOB$ 
>>
>> for the RLIMIT_NPROC check, but it seems it might be easier to just
>> inherit everything from the beginning, becuase I'd need to do something
>> like that patch several times.
> 
> I read through the code and I don't see why you want to make these
> almost threads of a process not actually threads of that process
> (like the io_uring threads are).
> 
> As a separate process there are many things that will continue to be
> disjoint.  If the thread changes cgroups for example your new process
> won't follow.
> 
> If you want them to be actually processes with an lifetime independent
> of the creating process I expect you want to reparent them to the local
> init process.  Just so they don't confuse the process tree.  Plus init
> processes know how to handle unexpected children.
> 
> What are the semantics you are aiming for?
> 

Hi Eric,

Right now, for vhost we need the thread being created:

1. added to the caller's cgroup.
2. to share the mm struct with the caller.
3. to be accounted for under the caller's nproc rlimit value.

For 1 and 2, we have cgroup_attach_task_all and get_task_mm
already.

This patchset started with me just trying to handle #3 by modifying kthreads
like here:

https://lkml.org/lkml/2021/6/23/1234

So we can use kthreads and the existing helpers and add:

A. a ucounts version of the above patches in the link

or

B. a helper that does something like copy_process's use of
is_ucounts_overlimit and vhost can call that.

instead of this patchset.


Before we even get to the next section below, do you consider items 1 - 3
something we need an API based on copy_process for?

Do you think I should just do A or B above, or do you have another idea? If
so can we get agreement on that from everyone?

I thought my patches in that link were a little hacky in how they passed
around the user/creds info. I thought maybe it shouldn't be passed around like
that, so switched to the copy_process based approach which did everything for
me. And I thought io_uring needed something similar as us so I made it generic.

I don't have a preference. You and Christian are the experts, so I'll leave it
to you guys.


> I can see sense in generalizing some of the pieces of create_io_thread
> but I think generalizing create_io_thread itself is premature.  The code
> lives in kernel/fork.c because it is a very special thing that we want
> to keep our eyes on.
> 
> Some of your generalization makes it much more difficult to tell what
> your code is going to use because you remove hard codes that are there
> to simplify the analysis of the situation.
> 
> My gut says adding a new create_vhost_worker and putting that in
> kernel/fork.c is a lot safer and will allow much better code analysis.
> 
> If there a really are commonalities between creating a userspace process
> that runs completely in the kernel and creating an additional userspace
> thread we refactor the code and simplify things.
> 
> I am especially nervous about generalizing the io_uring code as it's
> signal handling just barely works, and any generalization will cause it
> to break.  So you are in the process of generalizing code that simply
> can not handle the general case.  That scares me