Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7217566f-9c40-ae9d-6fd6-2ef93f13f853@oracle.com>
Date:   Tue, 28 Jun 2022 08:48:11 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH] vfio: remove useless judgement
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     lizhe.67@bytedance.com, cohuck@redhat.com, jgg@ziepe.ca,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        lizefan.x@bytedance.com
References: <20220627035109.73745-1-lizhe.67@bytedance.com>
 <20220627160640.7edca0dd.alex.williamson@redhat.com>
From:   Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20220627160640.7edca0dd.alex.williamson@redhat.com>
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?UVZEa2JIOEVybE9mdFloOTJrY1RiNmlZdlAwVjhiNVNMdnQ4N0M2QXgzQWdQ?=
 =?utf-8?B?ckFMZGRXVlc2T2VHUFFTR1ZYQU1SeTZZbGpQZWZMT0VKbklUSTQrM25xekV3?=
 =?utf-8?B?amtCeXRzSGZmL09RbnBZNWE2dnF4UkdXRlgrVnYyOUNEb0N4NXZYU0haOGw4?=
 =?utf-8?B?T0g2N3R0b2dVaVFLaTV1K0ZabVNOSk15UERGU0pydWJjak9FWWMyWnh1SWFo?=
 =?utf-8?B?c1VVRW50M2NjWW91eFFQMmVmZ01HVDVJL3dEaFY0R29UdFFyOVRXUEVIT0FH?=
 =?utf-8?B?MFVRUFcyUnpJa3pFNS9SN2NtSmFncGQ1NEdZcVhrT05weUphRUpzUWVkbjFH?=
 =?utf-8?B?VmQxOEt6UGRMaFFJSW9yNTdpVmQzUVRJVSsweUtkU0pSYloyWjRDVWpISE0z?=
 =?utf-8?B?cTVYdVdzTjhTREs3c0dBYzFGSVZ1NVFqVG82djFwRzEwYitUQ29pRmlwSmh3?=
 =?utf-8?B?VzNTa0NSL3pLWkMvZk9SS3JLUnZHRmJHT2lqVUFQb0t0Y1MrZGRwbWFWdEY5?=
 =?utf-8?B?VSs3NXhWRUtXNFE1K0l0cVZDdmg1OWNRRjZxVzZEL3JKVWNmZ2liTzFLUlNx?=
 =?utf-8?B?NVV6OXdtWGZaa2YxZ3UrdE1oUjFqWDJYcXNYWnVBNTVvQjNwUm03czVHTVNi?=
 =?utf-8?B?TE9ROUNvNzNOT0xnZUFNQTNxZ0FTdHZ0UDdLZzV2MVBJYVZiQk1ad1pyM3ZG?=
 =?utf-8?B?YTFFTDErSXFrbUg4dFFqMENBZXF0M0hFQUx0LzFXOStlbzdXV0RNenpsRkxT?=
 =?utf-8?B?Y0V1RThZK0hrcnJtWXVkRjFJdW5jK1pUVnhjWWZmV0RiaUJGVUZjVmx0T3Y3?=
 =?utf-8?B?Q2ZaMFJJd250UlN0dE9hQ25oZVc3VTN2Qktwc2N5TExOZTVLaUJqN0dQV0FS?=
 =?utf-8?B?K3BTbHVTVmNsS2tZa1hGdXUxTlQ0RDBia0tYend3K0h3c1FjOTR0SGVuSDBF?=
 =?utf-8?B?TW8wZThObWU3SDZZMisvVzhaeVNNTXpYMitaZGVCZjNJbmRsRFhTUmtnU0hr?=
 =?utf-8?B?N2hzY2VVVGxOSFRiL3NjL0ZYNWpYN2lsZ1JmM0NhYUhuQ2ZLZkZtSlNsYU5B?=
 =?utf-8?B?YlFqTEZLYkRjbzhOT05nczZVTC9YSzEzMDQ3djFQbm5CdTE2cnUrdU10MUxB?=
 =?utf-8?B?V3Ezb0xrMWMxQzNaZ0ViNzBCQksyUk9qQy9xb3BxTjhicnZMa1Q1dDVGMFpD?=
 =?utf-8?B?L1B0MkRrWitBMWJVby9JWm9Td0JqS0VSaUhMNHlhMWJFdmZZZGxRbDBuMUJt?=
 =?utf-8?B?TVpmaFdiU0RDMzM5UTBwdE9XbFFiL1NxRG56YXJTcHZGMUt3NTRzeWF1ank3?=
 =?utf-8?B?UTN3cG5VbjZZN0RYY0NxU0JKMUhtenBiRHp4VlFJOFNCK3lPZnVTTGRYSlB3?=
 =?utf-8?B?MjY1V3ZUUUhvb2FTTnB5Z1pvZ0hjaEhnQVh5T280VlRod0xDRjBQTUpOdnBJ?=
 =?utf-8?B?ZW9KQ1VYUzhzRGxqc1lGTE9QcWxsckZaU1duVHBFdUovcmZMRlovUkNCZmJ4?=
 =?utf-8?B?OTd4QnpkVUhHUk1JTUtSRjVIQjkvbTVZY3M1N1B1ay9ObGdlaEZHRk5Hci9W?=
 =?utf-8?B?MFR0NXpiK3h2S2hQTXRUM1lYRFdQY3RCYWQ4K3hKRXN2eDdRc0tjV0xYK2JX?=
 =?utf-8?B?bWVaV1Z5L1RXU0lIQkVvZHZ0cXhXRWdFWWhkVW5mWWNKOHJOZW5uS2o2cEhS?=
 =?utf-8?B?SUdnUUo2Z1NhTGU1R09FTnpLK3FuWkE5M3h3UmxDcUhYcWhwSHEzMS9ydldh?=
 =?utf-8?B?UlZnOFVEZ0J1eVN1ZWZRTDNDNXlwR1lBbllwSmErVUNFYkpwMjVPM0luU29q?=
 =?utf-8?B?cHRRbVN1WFFrcWEzR3NId0tHbEszSlRaSjV1NG42WW1xTnNjelJlRVVkR3JD?=
 =?utf-8?B?cWI0OTJwUjQ5SlFIdHlRSlBvSCtlOGNJY01sbmwvaHVJOTVTNkVsakdGcUhF?=
 =?utf-8?B?c2dOUGdrRS9zdk05NXlJNzdoYjc0MytWNncwbkJEYThEUGtHalFVeStneGFM?=
 =?utf-8?B?SkVwK2dkem55NTFtblJwZ1NDZ0lNZWtFWUpQeGozSzZ5SkdZMVlLbUpSSGRj?=
 =?utf-8?B?N1cvL3pIaVo2NFVYOWxDWEg3bnhHYVdXSWtBWWNodHcvVzAxWVBxUEMvQ2JJ?=
 =?utf-8?B?TDNjb0ZWZFQ4QlJkV2R1YVBvQWFFY2VMQXlZcURzRzVKLzA0bWYxZDh6K3Ni?=
 =?utf-8?B?N0E9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68e64b07-7481-4472-aa2c-08da59047686
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3240.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jun 2022 12:48:13.0874
 (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: psBKxnXwrwv1Z16KUlsgTYgHfzcb9TZ9i7o9utYwSbCMXoUoPVda2mAwajg1lW4DP5zJD2oAcUO+dewgLOLmTTdKZg17bmMHVUysGC6xKDs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB3904
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-06-28_07:2022-06-28,2022-06-28 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0 bulkscore=0
 phishscore=0 mlxlogscore=999 adultscore=0 mlxscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2206280053
X-Proofpoint-GUID: 6TsPf8Vv3XYj6x-0rIqVMAWJjVwJi-7z
X-Proofpoint-ORIG-GUID: 6TsPf8Vv3XYj6x-0rIqVMAWJjVwJi-7z
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

For cpr, old qemu directly exec's new qemu, so task does not change.

To support fork+exec, the ownership test needs to be deleted or modified.

Pinned page accounting is another issue, as the parent counts pins in its
mm->locked_vm.  If the child unmaps, it cannot simply decrement its own
mm->locked_vm counter.  As you and I have discussed, the count is also 
wrong in the direct exec model, because exec clears mm->locked_vm.  I am 
thinking vfio could count pins in struct user locked_vm to handle both 
models.  The user struct and its count would persist across direct exec,
and be shared by parent and child for fork+exec.  However, that does change
the RLIMIT_MEMLOCK value that applications must set, because the limit must
accommodate vfio plus other sub-systems that count in user->locked_vm, which
includes io_uring, skbuff, xdp, and perf.  Plus, the limit must accommodate all
processes of that user, not just a single process.

Folks like fork+exec because it allows recovery if the new qemu process fails to
initialize. One can fall back to the original process, if the above issues are fixed.

- Steve

On 6/27/2022 6:06 PM, Alex Williamson wrote:
> 
> Hey Steve, how did you get around this for cpr or is this a gap?
> Thanks,
> 
> Alex
> 
> On Mon, 27 Jun 2022 11:51:09 +0800
> lizhe.67@bytedance.com wrote:
> 
>> From: Li Zhe <lizhe.67@bytedance.com>
>>
>> In function vfio_dma_do_unmap(), we currently prevent process to unmap
>> vfio dma region whose mm_struct is different from the vfio_dma->task.
>> In our virtual machine scenario which is using kvm and qemu, this
>> judgement stops us from liveupgrading our qemu, which uses fork() &&
>> exec() to load the new binary but the new process cannot do the
>> VFIO_IOMMU_UNMAP_DMA action during vm exit because of this judgement.
>>
>> This judgement is added in commit 8f0d5bb95f76 ("vfio iommu type1: Add
>> task structure to vfio_dma") for the security reason. But it seems that
>> no other task who has no family relationship with old and new process
>> can get the same vfio_dma struct here for the reason of resource
>> isolation. So this patch delete it.
>>
>> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
>> Reviewed-by: Jason Gunthorpe <jgg@ziepe.ca>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index c13b9290e357..a8ff00dad834 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1377,12 +1377,6 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  
>>  		if (!iommu->v2 && iova > dma->iova)
>>  			break;
>> -		/*
>> -		 * Task with same address space who mapped this iova range is
>> -		 * allowed to unmap the iova range.
>> -		 */
>> -		if (dma->task->mm != current->mm)
>> -			break;
>>  
>>  		if (invalidate_vaddr) {
>>  			if (dma->vaddr_invalid) {
>