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;
Date:   Tue, 9 Mar 2021 19:45:03 -0400
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     Peter Xu <peterx@redhat.com>,
        "Zengtao (B)" <prime.zeng@hisilicon.com>,
        Cornelia Huck <cohuck@redhat.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Giovanni Cabiddu <giovanni.cabiddu@intel.com>,
        Michel Lespinasse <walken@google.com>,
        Jann Horn <jannh@google.com>,
        Max Gurtovoy <mgurtovoy@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Linuxarm <linuxarm@huawei.com>
Subject: Re: [PATCH] vfio/pci: make the vfio_pci_mmap_fault reentrant
Message-ID: <20210309234503.GN2356281@nvidia.com>
References: <1615201890-887-1-git-send-email-prime.zeng@hisilicon.com>
 <20210308132106.49da42e2@omen.home.shazbot.org>
 <20210308225626.GN397383@xz-x1>
 <6b98461600f74f2385b9096203fa3611@hisilicon.com>
 <20210309124609.GG2356281@nvidia.com>
 <20210309082951.75f0eb01@x1.home.shazbot.org>
 <20210309164004.GJ2356281@nvidia.com>
 <20210309184739.GD763132@xz-x1>
 <20210309122607.0b68fb9b@omen.home.shazbot.org>
 <20210309125639.70724531@omen.home.shazbot.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210309125639.70724531@omen.home.shazbot.org>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from mlx.ziepe.ca (142.162.115.133) by BL1PR13CA0439.namprd13.prod.outlook.com (2603:10b6:208:2c3::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3933.16 via Frontend Transport; Tue, 9 Mar 2021 23:45:04 +0000
Received: from jgg by mlx with local (Exim 4.94)        (envelope-from <jgg@nvidia.com>)        id 1lJm2B-00AXG4-Jg; Tue, 09 Mar 2021 19:45:03 -0400
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 6bbcf0c2-8796-40b0-45ae-08d8e3555d4a
X-MS-TrafficTypeDiagnostic: DM6PR12MB3210:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <DM6PR12MB321049F10E3D0D36F432E160C2929@DM6PR12MB3210.namprd12.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:8273;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: Utnua6lSLp744MGbeySqle5X5h2vLlXWF5bMXCneog7cn+NrJnuPYYasyl448JQgtHBfHQ221OmjfiJSmJjImfoudXtjKc8f+cx7/iahp3U64kjhCs5Ndq5dHDxsuv6FTPcEdA950L2JL0Qs2MFknslBplHHhzX4Jhhna7iwIzFMXg6hRmQ8RORv6YeAa89MhKyETX2NpTLtD1boyEJo9NTeWo9995BQbFzeFhT/Hx8+TPhpkIMk0B9hPYucdD+x1mruUBGFjYfElTAXuhcQWB1f6ls4W33RqR4KEhGG9b0V9Gxk55Zm5wlMw/LHo2QDVM+wuwMSv6COg+ebaTZNdQBFO2tiVrX3FXfmNjz7DvfyluzQxANMZJQSHr/uHI2N425SDIDaYpl3WYUxKxRQavQaPGH3/Rbm2vPXekEGrXEaWmqdRVDFxi6UXQ1LV0DRQDgSF8BlVSefsaymhF5/tOPJiXLJ7YxBB6Z0xfCRMgI9jRq91wNGqFbZkf/U8NGXW976cJAQZH5Ce8wvGqQVsw==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR12MB3834.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(396003)(346002)(366004)(376002)(39860400002)(136003)(1076003)(8936002)(66556008)(6916009)(86362001)(26005)(33656002)(9786002)(8676002)(5660300002)(316002)(54906003)(2906002)(478600001)(7416002)(426003)(66946007)(186003)(66476007)(9746002)(36756003)(83380400001)(4326008)(2616005);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?r5xDpSS+gjUSMGOLjoWzyvt+y1CcP+GNR2pfFJ7pbODEoWhal/ErQy5SNiKr?=
 =?us-ascii?Q?RZcvcQp6DdF1WokDW8KY0MEWTzr78+NBX13S/5hTWA4hHiqvpZujQkoM2yKe?=
 =?us-ascii?Q?U9lp1gfnYIzf7tpM40ZAuk3uKP9Y3JLvv0kqoiAQVAVTrNBVPEVVAHhO9cjd?=
 =?us-ascii?Q?Dq/5YHkE9aFRaxBD1vDAbyPaiGzB3B/1rWbydn55ncvNUFv1tHZsy81Udt6Q?=
 =?us-ascii?Q?y6TI023eBhldJNoFZDbeuZWCjLiV8I7Xx/VuekR08f7e41YbPOA96rCG+4JS?=
 =?us-ascii?Q?AehmxB6426iVLqV6hSfU9usiJXMud/Tzz4VmW2/kWzAuEAwia/hT2XIY4KfD?=
 =?us-ascii?Q?IoE79urqPP2GJ2dhBp6LLocXFLHtW4t/uubCNoc61xgd4S3e7wrSenBb/SRw?=
 =?us-ascii?Q?FAiRjyETkkm9p9MYSo9aYvmPWR712e0zsBsfnj+b/9XPKTC4Qg+ZTXRJz/rc?=
 =?us-ascii?Q?T4AhD3B8F/LjxaUzFczcBT9/ly3fcyNExafBY0jIYNk74WsRIKX1lvinOGQt?=
 =?us-ascii?Q?0BmL3615UsVzAVXczfXcslNnpajw06XtnTi+VnGw9arXcP3XRpYK/0rPJB8r?=
 =?us-ascii?Q?L5t8N9cp4g607zS7ceLk6yOKWYb3iXUql00wlLuNdcQgH64RCWDONvKiHUz3?=
 =?us-ascii?Q?2MVGnbyRvkGfzhy7S4VFd8oK3OU+9mtnkDMd/AMxN4py7k3iF4m/YlNpBg76?=
 =?us-ascii?Q?zXowd1Im2ZvYyvax+rZny7145XwWwzr95Qk4HOG2ja9W7t1D4w5aUxrDQzmU?=
 =?us-ascii?Q?if5J0/9vLsTyi4X5rCwoNociUcE4+1287csvFDOMDLuCbKwyMkST6+xASsBt?=
 =?us-ascii?Q?bow3XyNKFzKfSciyrJ05X1hDkmrgwgRukKiPDvBUkzwNNN6xrZarhlXSxbMq?=
 =?us-ascii?Q?RD84l0D5cPo13eJX2Ae4yN3lzgIxyNuIn3YUJ9KqMEN0BtqwFM59dTbfVCJE?=
 =?us-ascii?Q?z5+Jbm5ms2WGi6t5TbpLePag61teJ2bqOmf3rb5C4DtnY71tyYcpVXW2zpbo?=
 =?us-ascii?Q?7ZBoaf0s2ZsukeTZUWAL06Dmj9AjuSXc2M7klATio9RMO3Ag9RR1pVfkVoRg?=
 =?us-ascii?Q?7nZugrrgBlwJE72NYL8f90oVdPYsc1LdEda+ksPjtcRd5wa4Od9EOB0Nfr9P?=
 =?us-ascii?Q?jBzy1usYMTxg85yo7SLapLKYcZCHKIboZkt0BDL4/XEg3ZE9F1cBFT4z0iJt?=
 =?us-ascii?Q?cbDwBmLLmK2/i+wZ5txAEL/f+2qLSWfIDNTmzL50LzbYmU2KwO4Ija16vggL?=
 =?us-ascii?Q?Fx+XUteoxVEL8ouwETr9nh5CRvnExlgfcI4Swy8zaUKdtUnEVKFxsGoJGjQ6?=
 =?us-ascii?Q?iyd84vshzLWOvz4AuLsSKE0BUNBpveXsU9l3fclbqKUrUQ=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bbcf0c2-8796-40b0-45ae-08d8e3555d4a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3834.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2021 23:45:05.8424
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1NEN8V08DXHOgMRJG2OG9fBriVX4b2rNXjmzwxWLdg1Zy6VDDc0KVLuqz1kPZIOx
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3210
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Mar 09, 2021 at 12:56:39PM -0700, Alex Williamson wrote:

> And I think this is what we end up with for the current code base:

Yeah, that looks Ok
 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 65e7e6b44578..2f247ab18c66 100644
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1568,19 +1568,24 @@ void vfio_pci_memory_unlock_and_restore(struct vfio_pci_device *vdev, u16 cmd)
>  }
>  
>  /* Caller holds vma_lock */
> -static int __vfio_pci_add_vma(struct vfio_pci_device *vdev,
> -			      struct vm_area_struct *vma)
> +struct vfio_pci_mmap_vma *__vfio_pci_add_vma(struct vfio_pci_device *vdev,
> +					     struct vm_area_struct *vma)
>  {
>  	struct vfio_pci_mmap_vma *mmap_vma;
>  
> +	list_for_each_entry(mmap_vma, &vdev->vma_list, vma_next) {
> +		if (mmap_vma->vma == vma)
> +			return ERR_PTR(-EEXIST);
> +	}
> +
>  	mmap_vma = kmalloc(sizeof(*mmap_vma), GFP_KERNEL);
>  	if (!mmap_vma)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	mmap_vma->vma = vma;
>  	list_add(&mmap_vma->vma_next, &vdev->vma_list);
>  
> -	return 0;
> +	return mmap_vma;
>  }
>  
>  /*
> @@ -1612,30 +1617,39 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct vfio_pci_device *vdev = vma->vm_private_data;
> -	vm_fault_t ret = VM_FAULT_NOPAGE;
> +	struct vfio_pci_mmap_vma *mmap_vma;
> +	unsigned long vaddr, pfn;
> +	vm_fault_t ret;
>  
>  	mutex_lock(&vdev->vma_lock);
>  	down_read(&vdev->memory_lock);
>  
>  	if (!__vfio_pci_memory_enabled(vdev)) {
>  		ret = VM_FAULT_SIGBUS;
> -		mutex_unlock(&vdev->vma_lock);
>  		goto up_out;
>  	}
>  
> -	if (__vfio_pci_add_vma(vdev, vma)) {
> -		ret = VM_FAULT_OOM;
> -		mutex_unlock(&vdev->vma_lock);
> +	mmap_vma = __vfio_pci_add_vma(vdev, vma);
> +	if (IS_ERR(mmap_vma)) {
> +		/* A concurrent fault might have already inserted the page */
> +		ret = (PTR_ERR(mmap_vma) == -EEXIST) ? VM_FAULT_NOPAGE :
> +						       VM_FAULT_OOM;

I think -EEIXST should not be an error, lets just go down to the
vmf_insert_pfn() and let the MM resolve the race naturally.

I suspect returning VM_FAULT_NOPAGE will be averse to the userspace if
it hits this race??

Also the _prot does look needed at least due to the SME, but possibly
also to ensure NC gets set..

Jason