[PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

Fix several loose ends relative to reverting support for vaddr removal
and update.  Mark feature and ioctl flags as deprecated, restore local
variable scope in pin pages, remove remaining support in the mapping
code.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

This applies on top of Steve's patch[1] to fully remove and deprecate
this feature in the short term, following the same methodology we used
for the v1 migration interface removal.  The intention would be to pick
Steve's patch and this follow-on for v6.2 given that existing support
exposes vulnerabilities and no known upstream userspaces make use of
this feature.

[1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-steven.sistare@oracle.com/

 drivers/vfio/vfio_iommu_type1.c |   23 ++++-------------------
 include/uapi/linux/vfio.h       |    7 ++++---
 2 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 02c6ea3bed69..731d8d4b6524 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -790,7 +790,6 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	unsigned long remote_vaddr;
 	struct vfio_dma *dma;
 	bool do_accounting;
-	dma_addr_t iova;
 
 	if (!iommu || !pages)
 		return -EINVAL;
@@ -815,6 +814,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	do_accounting = list_empty(&iommu->domain_list);
 
 	for (i = 0; i < npage; i++) {
+		dma_addr_t iova;
 		unsigned long phys_pfn;
 		struct vfio_pfn *vpfn;
 
@@ -1467,7 +1467,6 @@ static bool vfio_iommu_iova_dma_valid(struct vfio_iommu *iommu,
 static int vfio_dma_do_map(struct vfio_iommu *iommu,
 			   struct vfio_iommu_type1_dma_map *map)
 {
-	bool set_vaddr = map->flags & VFIO_DMA_MAP_FLAG_VADDR;
 	dma_addr_t iova = map->iova;
 	unsigned long vaddr = map->vaddr;
 	size_t size = map->size;
@@ -1485,16 +1484,13 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	if (map->flags & VFIO_DMA_MAP_FLAG_READ)
 		prot |= IOMMU_READ;
 
-	if ((prot && set_vaddr) || (!prot && !set_vaddr))
-		return -EINVAL;
-
 	mutex_lock(&iommu->lock);
 
 	pgsize = (size_t)1 << __ffs(iommu->pgsize_bitmap);
 
 	WARN_ON((pgsize - 1) & PAGE_MASK);
 
-	if (!size || (size | iova | vaddr) & (pgsize - 1)) {
+	if (!prot || !size || (size | iova | vaddr) & (pgsize - 1)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -1505,17 +1501,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		goto out_unlock;
 	}
 
-	dma = vfio_find_dma(iommu, iova, size);
-	if (set_vaddr) {
-		if (!dma) {
-			ret = -ENOENT;
-		} else if (dma->iova != iova || dma->size != size) {
-			ret = -EINVAL;
-		} else {
-			dma->vaddr = vaddr;
-		}
-		goto out_unlock;
-	} else if (dma) {
+	if (vfio_find_dma(iommu, iova, size)) {
 		ret = -EEXIST;
 		goto out_unlock;
 	}
@@ -2727,8 +2713,7 @@ static int vfio_iommu_type1_map_dma(struct vfio_iommu *iommu,
 {
 	struct vfio_iommu_type1_dma_map map;
 	unsigned long minsz;
-	uint32_t mask = VFIO_DMA_MAP_FLAG_READ | VFIO_DMA_MAP_FLAG_WRITE |
-			VFIO_DMA_MAP_FLAG_VADDR;
+	uint32_t mask = VFIO_DMA_MAP_FLAG_READ | VFIO_DMA_MAP_FLAG_WRITE;
 
 	minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 04d944c8941d..800ca94aafb3 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -49,8 +49,8 @@
 /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
 #define VFIO_UNMAP_ALL			9
 
-/* Obsolete, not supported by any IOMMU. */
-#define VFIO_UPDATE_VADDR		10
+/* Probe for reverted vaddr removal and update support */
+#define VFIO_UPDATE_VADDR_DEPRECATED	10
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
@@ -1348,7 +1348,7 @@ struct vfio_iommu_type1_dma_map {
 	__u32	flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
-#define VFIO_DMA_MAP_FLAG_VADDR (1 << 2)
+#define VFIO_DMA_MAP_FLAG_VADDR_DEPRECATED (1 << 2) /* prior vaddr remapping */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
@@ -1390,6 +1390,7 @@ struct vfio_iommu_type1_dma_unmap {
 	__u32	flags;
 #define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
 #define VFIO_DMA_UNMAP_FLAG_ALL		     (1 << 1)
+#define VFIO_DMA_UNMAP_FLAG_VADDR_DEPRECATED (1 << 2) /* prior vaddr removal */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
 	__u8    data[];

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Jason Gunthorpe]

On Wed, Dec 07, 2022 at 02:45:18PM -0700, Alex Williamson wrote:
> Fix several loose ends relative to reverting support for vaddr removal
> and update.  Mark feature and ioctl flags as deprecated, restore local
> variable scope in pin pages, remove remaining support in the mapping
> code.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Tian, Kevin]

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Thursday, December 8, 2022 5:45 AM
> 
> Fix several loose ends relative to reverting support for vaddr removal
> and update.  Mark feature and ioctl flags as deprecated, restore local
> variable scope in pin pages, remove remaining support in the mapping
> code.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> This applies on top of Steve's patch[1] to fully remove and deprecate
> this feature in the short term, following the same methodology we used
> for the v1 migration interface removal.  The intention would be to pick
> Steve's patch and this follow-on for v6.2 given that existing support
> exposes vulnerabilities and no known upstream userspaces make use of
> this feature.
> 
> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
> steven.sistare@oracle.com/
> 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

btw given the exposure and no known upstream usage should this be
also pushed to stable kernels?

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Thu, 8 Dec 2022 07:56:30 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Thursday, December 8, 2022 5:45 AM
> > 
> > Fix several loose ends relative to reverting support for vaddr removal
> > and update.  Mark feature and ioctl flags as deprecated, restore local
> > variable scope in pin pages, remove remaining support in the mapping
> > code.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > This applies on top of Steve's patch[1] to fully remove and deprecate
> > this feature in the short term, following the same methodology we used
> > for the v1 migration interface removal.  The intention would be to pick
> > Steve's patch and this follow-on for v6.2 given that existing support
> > exposes vulnerabilities and no known upstream userspaces make use of
> > this feature.
> > 
> > [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
> > steven.sistare@oracle.com/
> >   
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw given the exposure and no known upstream usage should this be
> also pushed to stable kernels?

I'll add to both:

Cc: stable@vger.kernel.org # v5.12+

Thanks,
Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Steven Sistare]

On 12/8/2022 11:40 AM, Alex Williamson wrote:
> On Thu, 8 Dec 2022 07:56:30 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
>>> From: Alex Williamson <alex.williamson@redhat.com>
>>> Sent: Thursday, December 8, 2022 5:45 AM
>>>
>>> Fix several loose ends relative to reverting support for vaddr removal
>>> and update.  Mark feature and ioctl flags as deprecated, restore local
>>> variable scope in pin pages, remove remaining support in the mapping
>>> code.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>> This applies on top of Steve's patch[1] to fully remove and deprecate
>>> this feature in the short term, following the same methodology we used
>>> for the v1 migration interface removal.  The intention would be to pick
>>> Steve's patch and this follow-on for v6.2 given that existing support
>>> exposes vulnerabilities and no known upstream userspaces make use of
>>> this feature.
>>>
>>> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
>>> steven.sistare@oracle.com/
>>>   
>>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>
>> btw given the exposure and no known upstream usage should this be
>> also pushed to stable kernels?
> 
> I'll add to both:
> 
> Cc: stable@vger.kernel.org # v5.12+

We maintain and use a version of qemu that contains the live update patches,
and requires these kernel interfaces. Other companies are also experimenting 
with it.  Please do not remove it from stable.

- Steve

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Fri, 9 Dec 2022 13:40:29 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/8/2022 11:40 AM, Alex Williamson wrote:
> > On Thu, 8 Dec 2022 07:56:30 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> >>> From: Alex Williamson <alex.williamson@redhat.com>
> >>> Sent: Thursday, December 8, 2022 5:45 AM
> >>>
> >>> Fix several loose ends relative to reverting support for vaddr removal
> >>> and update.  Mark feature and ioctl flags as deprecated, restore local
> >>> variable scope in pin pages, remove remaining support in the mapping
> >>> code.
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> ---
> >>>
> >>> This applies on top of Steve's patch[1] to fully remove and deprecate
> >>> this feature in the short term, following the same methodology we used
> >>> for the v1 migration interface removal.  The intention would be to pick
> >>> Steve's patch and this follow-on for v6.2 given that existing support
> >>> exposes vulnerabilities and no known upstream userspaces make use of
> >>> this feature.
> >>>
> >>> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
> >>> steven.sistare@oracle.com/
> >>>     
> >>
> >> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >>
> >> btw given the exposure and no known upstream usage should this be
> >> also pushed to stable kernels?  
> > 
> > I'll add to both:
> > 
> > Cc: stable@vger.kernel.org # v5.12+  
> 
> We maintain and use a version of qemu that contains the live update patches,
> and requires these kernel interfaces. Other companies are also experimenting 
> with it.  Please do not remove it from stable.

The interface has been determined to have vulnerabilities and the
proposal to resolve those vulnerabilities is to implement a new API.
If we think it's worthwhile to remove the existing, vulnerable interface
in the current kernel, what makes it safe to keep it for stable kernels?

Existing users that could choose not to accept the revert in their
downstream kernel and allowing users evaluating the interface more time
before they know it's been removed upstream, are not terribly
compelling reasons to keep it in upstream stable kernels.  Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Steven Sistare]

On 12/9/2022 2:42 PM, Alex Williamson wrote:
> On Fri, 9 Dec 2022 13:40:29 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
> 
>> On 12/8/2022 11:40 AM, Alex Williamson wrote:
>>> On Thu, 8 Dec 2022 07:56:30 +0000
>>> "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>>   
>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>> Sent: Thursday, December 8, 2022 5:45 AM
>>>>>
>>>>> Fix several loose ends relative to reverting support for vaddr removal
>>>>> and update.  Mark feature and ioctl flags as deprecated, restore local
>>>>> variable scope in pin pages, remove remaining support in the mapping
>>>>> code.
>>>>>
>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>> ---
>>>>>
>>>>> This applies on top of Steve's patch[1] to fully remove and deprecate
>>>>> this feature in the short term, following the same methodology we used
>>>>> for the v1 migration interface removal.  The intention would be to pick
>>>>> Steve's patch and this follow-on for v6.2 given that existing support
>>>>> exposes vulnerabilities and no known upstream userspaces make use of
>>>>> this feature.
>>>>>
>>>>> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
>>>>> steven.sistare@oracle.com/
>>>>>     
>>>>
>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>>
>>>> btw given the exposure and no known upstream usage should this be
>>>> also pushed to stable kernels?  
>>>
>>> I'll add to both:
>>>
>>> Cc: stable@vger.kernel.org # v5.12+  
>>
>> We maintain and use a version of qemu that contains the live update patches,
>> and requires these kernel interfaces. Other companies are also experimenting 
>> with it.  Please do not remove it from stable.
> 
> The interface has been determined to have vulnerabilities and the
> proposal to resolve those vulnerabilities is to implement a new API.
> If we think it's worthwhile to remove the existing, vulnerable interface
> in the current kernel, what makes it safe to keep it for stable kernels?

I do not think it's worth while, but I have stopped fighting for 6.2.

> Existing users that could choose not to accept the revert in their
> downstream kernel and allowing users evaluating the interface more time
> before they know it's been removed upstream, are not terribly
> compelling reasons to keep it in upstream stable kernels.  Thanks,

The compelling reason is that stable is supposed to be stable and maintain
existing interfaces, and now I will need to re-merge the interfaces at
regular intervals when we update UEK from stable. Oracle is a current user 
of these interfaces in our business. Do we count?

- Steve

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Fri, 9 Dec 2022 14:52:49 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/9/2022 2:42 PM, Alex Williamson wrote:
> > On Fri, 9 Dec 2022 13:40:29 -0500
> > Steven Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> On 12/8/2022 11:40 AM, Alex Williamson wrote:  
> >>> On Thu, 8 Dec 2022 07:56:30 +0000
> >>> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >>>     
> >>>>> From: Alex Williamson <alex.williamson@redhat.com>
> >>>>> Sent: Thursday, December 8, 2022 5:45 AM
> >>>>>
> >>>>> Fix several loose ends relative to reverting support for vaddr removal
> >>>>> and update.  Mark feature and ioctl flags as deprecated, restore local
> >>>>> variable scope in pin pages, remove remaining support in the mapping
> >>>>> code.
> >>>>>
> >>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>> ---
> >>>>>
> >>>>> This applies on top of Steve's patch[1] to fully remove and deprecate
> >>>>> this feature in the short term, following the same methodology we used
> >>>>> for the v1 migration interface removal.  The intention would be to pick
> >>>>> Steve's patch and this follow-on for v6.2 given that existing support
> >>>>> exposes vulnerabilities and no known upstream userspaces make use of
> >>>>> this feature.
> >>>>>
> >>>>> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
> >>>>> steven.sistare@oracle.com/
> >>>>>       
> >>>>
> >>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >>>>
> >>>> btw given the exposure and no known upstream usage should this be
> >>>> also pushed to stable kernels?    
> >>>
> >>> I'll add to both:
> >>>
> >>> Cc: stable@vger.kernel.org # v5.12+    
> >>
> >> We maintain and use a version of qemu that contains the live update patches,
> >> and requires these kernel interfaces. Other companies are also experimenting 
> >> with it.  Please do not remove it from stable.  
> > 
> > The interface has been determined to have vulnerabilities and the
> > proposal to resolve those vulnerabilities is to implement a new API.
> > If we think it's worthwhile to remove the existing, vulnerable interface
> > in the current kernel, what makes it safe to keep it for stable kernels?  
> 
> I do not think it's worth while, but I have stopped fighting for 6.2.
> 
> > Existing users that could choose not to accept the revert in their
> > downstream kernel and allowing users evaluating the interface more time
> > before they know it's been removed upstream, are not terribly
> > compelling reasons to keep it in upstream stable kernels.  Thanks,  
> 
> The compelling reason is that stable is supposed to be stable and maintain
> existing interfaces, and now I will need to re-merge the interfaces at
> regular intervals when we update UEK from stable. Oracle is a current user 
> of these interfaces in our business. Do we count?

These are the rules for stable from
Documentation/process/stable-kernel-rules.rst:

 - It must be obviously correct and tested.

(check)

 - It cannot be bigger than 100 lines, with context.

(We're pushing this a bit, but we could certainly disable w/o removing
the interface in far fewer lines.  We're close enough that I think a
direct backport is preferable)

 - It must fix only one thing.

(check)

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

(This is a point where you could present an objection)

 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.

(This as well)

 - Serious issues as reported by a user of a distribution kernel may also
   be considered if they fix a notable performance or interactivity issue.
   As these fixes are not as obvious and have a higher risk of a subtle
   regression they should only be submitted by a distribution kernel
   maintainer and include an addendum linking to a bugzilla entry if it
   exists and additional information on the user-visible impact.

(N/A, but note the mention of a user visible impact)

 - New device IDs and quirks are also accepted.

(N/A)

 - No "theoretical race condition" issues, unless an explanation of how the
   race can be exploited is also provided.

(AIUI, the vulnerabilities here may not have exploits, but they are real)

 - It cannot contain any "trivial" fixes in it (spelling changes,
   whitespace cleanups, etc).

(N/A)

 - It must follow the
   :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
   rules.

(Of course)

 - It or an equivalent fix must already exist in Linus' tree (upstream).

This last bullet is really the crux of what brings us to this point, if
you're not willing to defend the vulnerabilities to maintain the
interface in the mainline kernel, why should the upstream community
maintain them in the stable kernels?

The question is not about who is using the interface, it's the fact that
the resolution to the existing vulnerabilities is to remove the
interface and nobody is making a case around the validity or
exploit-ability of those vulnerabilities to carry along the interface
in the interim.

If the revert does go into mainline, but were to skip stable, that only
delays your re-merging burden briefly, while continuing to expose stable
kernels to the vulnerabilities, and risks further users adopting an
interface that no longer exists.  Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Steven Sistare]

On 12/9/2022 4:01 PM, Alex Williamson wrote:
> On Fri, 9 Dec 2022 14:52:49 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
>> On 12/9/2022 2:42 PM, Alex Williamson wrote:
>>> On Fri, 9 Dec 2022 13:40:29 -0500
>>> Steven Sistare <steven.sistare@oracle.com> wrote:
>>>   
>>>> On 12/8/2022 11:40 AM, Alex Williamson wrote:  
>>>>> On Thu, 8 Dec 2022 07:56:30 +0000
>>>>> "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>>>>     
>>>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>>>> Sent: Thursday, December 8, 2022 5:45 AM
>>>>>>>
>>>>>>> Fix several loose ends relative to reverting support for vaddr removal
>>>>>>> and update.  Mark feature and ioctl flags as deprecated, restore local
>>>>>>> variable scope in pin pages, remove remaining support in the mapping
>>>>>>> code.
>>>>>>>
>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>>> ---
>>>>>>>
>>>>>>> This applies on top of Steve's patch[1] to fully remove and deprecate
>>>>>>> this feature in the short term, following the same methodology we used
>>>>>>> for the v1 migration interface removal.  The intention would be to pick
>>>>>>> Steve's patch and this follow-on for v6.2 given that existing support
>>>>>>> exposes vulnerabilities and no known upstream userspaces make use of
>>>>>>> this feature.
>>>>>>>
>>>>>>> [1]https://lore.kernel.org/all/1670363753-249738-2-git-send-email-
>>>>>>> steven.sistare@oracle.com/
>>>>>>>       
>>>>>>
>>>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>>>>
>>>>>> btw given the exposure and no known upstream usage should this be
>>>>>> also pushed to stable kernels?    
>>>>>
>>>>> I'll add to both:
>>>>>
>>>>> Cc: stable@vger.kernel.org # v5.12+    
>>>>
>>>> We maintain and use a version of qemu that contains the live update patches,
>>>> and requires these kernel interfaces. Other companies are also experimenting 
>>>> with it.  Please do not remove it from stable.  
>>>
>>> The interface has been determined to have vulnerabilities and the
>>> proposal to resolve those vulnerabilities is to implement a new API.
>>> If we think it's worthwhile to remove the existing, vulnerable interface
>>> in the current kernel, what makes it safe to keep it for stable kernels?  
>>
>> I do not think it's worth while, but I have stopped fighting for 6.2.
>>
>>> Existing users that could choose not to accept the revert in their
>>> downstream kernel and allowing users evaluating the interface more time
>>> before they know it's been removed upstream, are not terribly
>>> compelling reasons to keep it in upstream stable kernels.  Thanks,  
>>
>> The compelling reason is that stable is supposed to be stable and maintain
>> existing interfaces, and now I will need to re-merge the interfaces at
>> regular intervals when we update UEK from stable. Oracle is a current user 
>> of these interfaces in our business. Do we count?
> 
> These are the rules for stable from
> Documentation/process/stable-kernel-rules.rst:
> 
>  - It must be obviously correct and tested.
> 
> (check)
> 
>  - It cannot be bigger than 100 lines, with context.
> 
> (We're pushing this a bit, but we could certainly disable w/o removing
> the interface in far fewer lines.  We're close enough that I think a
> direct backport is preferable)
> 
>  - It must fix only one thing.
> 
> (check)
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> (This is a point where you could present an objection)
> 
>  - It must fix a problem that causes a build error (but not for things
>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>    security issue, or some "oh, that's not good" issue.  In short, something
>    critical.
> 
> (This as well)
> 
>  - Serious issues as reported by a user of a distribution kernel may also
>    be considered if they fix a notable performance or interactivity issue.
>    As these fixes are not as obvious and have a higher risk of a subtle
>    regression they should only be submitted by a distribution kernel
>    maintainer and include an addendum linking to a bugzilla entry if it
>    exists and additional information on the user-visible impact.
> 
> (N/A, but note the mention of a user visible impact)
> 
>  - New device IDs and quirks are also accepted.
> 
> (N/A)
> 
>  - No "theoretical race condition" issues, unless an explanation of how the
>    race can be exploited is also provided.
> 
> (AIUI, the vulnerabilities here may not have exploits, but they are real)
> 
>  - It cannot contain any "trivial" fixes in it (spelling changes,
>    whitespace cleanups, etc).
> 
> (N/A)
> 
>  - It must follow the
>    :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
>    rules.
> 
> (Of course)
> 
>  - It or an equivalent fix must already exist in Linus' tree (upstream).
> 
> This last bullet is really the crux of what brings us to this point, if
> you're not willing to defend the vulnerabilities to maintain the
> interface in the mainline kernel, why should the upstream community
> maintain them in the stable kernels?
> 
> The question is not about who is using the interface, it's the fact that
> the resolution to the existing vulnerabilities is to remove the
> interface and nobody is making a case around the validity or
> exploit-ability of those vulnerabilities to carry along the interface
> in the interim.
> 
> If the revert does go into mainline, but were to skip stable, that only
> delays your re-merging burden briefly, while continuing to expose stable
> kernels to the vulnerabilities, and risks further users adopting an
> interface that no longer exists.  Thanks,

Thank you for your thoughtful response.  Rather than debate the degree of
of vulnerability, I propose an alternate solution.  The technical crux of
the matter is support for mediated devices.  So, let's exclude them when
these legacy interfaces are used, and allow them for native iommufd.  The
fix is small and simple: if there is no iommu capable domain in the container, 
then return false for the VFIO_UPDATE_VADDR extension. And to prevent locked_mm 
underflow, add to the new mm's locked_vm in VFIO_DMA_MAP_FLAG_VADDR.

Two small patches, which I can submit on monday, for 6.x and stable.

I can also submit a patch for iommufd to use these interfaces with no mdev
in vfio compat mode.

I am still committed to new interfaces for native iommufd, and am making good
progress with Jason's patch.

- Steve

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Jason Gunthorpe]

On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:

> Thank you for your thoughtful response.  Rather than debate the degree of
> of vulnerability, I propose an alternate solution.  The technical crux of
> the matter is support for mediated devices.  

I'm not sure I'm convinced about that. It is easy to make problematic
situations with mdevs, but that doesn't mean other cases don't exist
too eg what happens if userspace suspends and then immediately does
something to trigger a domain attachment? Doesn't it still deadlock
the kernel?

Honestly, I'm not sure I see the big deal, just don't backport these
reverts to your disto kernel.

Jason

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Steven Sistare]

On 12/12/2022 8:17 AM, Jason Gunthorpe wrote:
> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> 
>> Thank you for your thoughtful response.  Rather than debate the degree of
>> of vulnerability, I propose an alternate solution.  The technical crux of
>> the matter is support for mediated devices.  
> 
> I'm not sure I'm convinced about that. It is easy to make problematic
> situations with mdevs, but that doesn't mean other cases don't exist
> too eg what happens if userspace suspends and then immediately does
> something to trigger a domain attachment? Doesn't it still deadlock
> the kernel?

No deadlock.  Any ioctl's that need vaddr return EINVAL if the vaddr has been
suspended.  ioctl's that do not need it succeed.  The vaddr is not needed when
all pages have been pinned, because iova can be translated via the iommu.

> Honestly, I'm not sure I see the big deal, just don't backport these
> reverts to your disto kernel.

It must be done every time the kernel is refreshed, and is disruptive and 
error prone.  All exceptions to the normal process are. And it derails my qemu 
patch review until native iommufd support is pushed into qemu.  If I can avoid 
those problems with a few simple fixes, then that is a win.

- Steve

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Mon, 12 Dec 2022 09:17:54 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> 
> > Thank you for your thoughtful response.  Rather than debate the degree of
> > of vulnerability, I propose an alternate solution.  The technical crux of
> > the matter is support for mediated devices.    
> 
> I'm not sure I'm convinced about that. It is easy to make problematic
> situations with mdevs, but that doesn't mean other cases don't exist
> too eg what happens if userspace suspends and then immediately does
> something to trigger a domain attachment? Doesn't it still deadlock
> the kernel?

The opportunity for that to deadlock isn't obvious to me, a replay
would be stalled waiting for invalid vaddrs, but this is essentially
the user deadlocking themselves.  There's also code there to handle the
process getting killed while waiting, making it interruptible.  Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Steven Sistare]

On 12/12/2022 10:58 AM, Alex Williamson wrote:
> On Mon, 12 Dec 2022 09:17:54 -0400
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
>> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
>>
>>> Thank you for your thoughtful response.  Rather than debate the degree of
>>> of vulnerability, I propose an alternate solution.  The technical crux of
>>> the matter is support for mediated devices.    
>>
>> I'm not sure I'm convinced about that. It is easy to make problematic
>> situations with mdevs, but that doesn't mean other cases don't exist
>> too eg what happens if userspace suspends and then immediately does
>> something to trigger a domain attachment? Doesn't it still deadlock
>> the kernel?
> 
> The opportunity for that to deadlock isn't obvious to me, a replay
> would be stalled waiting for invalid vaddrs, but this is essentially
> the user deadlocking themselves.  There's also code there to handle the
> process getting killed while waiting, making it interruptible.  Thanks,

I will submit new patches tomorrow to exclude mdevs.  Almost done.

- Steve

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Mon, 12 Dec 2022 15:59:11 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/12/2022 10:58 AM, Alex Williamson wrote:
> > On Mon, 12 Dec 2022 09:17:54 -0400
> > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >   
> >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> >>  
> >>> Thank you for your thoughtful response.  Rather than debate the degree of
> >>> of vulnerability, I propose an alternate solution.  The technical crux of
> >>> the matter is support for mediated devices.      
> >>
> >> I'm not sure I'm convinced about that. It is easy to make problematic
> >> situations with mdevs, but that doesn't mean other cases don't exist
> >> too eg what happens if userspace suspends and then immediately does
> >> something to trigger a domain attachment? Doesn't it still deadlock
> >> the kernel?  
> > 
> > The opportunity for that to deadlock isn't obvious to me, a replay
> > would be stalled waiting for invalid vaddrs, but this is essentially
> > the user deadlocking themselves.  There's also code there to handle the
> > process getting killed while waiting, making it interruptible.  Thanks,  
> 
> I will submit new patches tomorrow to exclude mdevs.  Almost done.

I've dropped the removal commits from my next branch in the interim.
Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Jason Gunthorpe]

On Mon, Dec 12, 2022 at 02:26:51PM -0700, Alex Williamson wrote:
> On Mon, 12 Dec 2022 15:59:11 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
> 
> > On 12/12/2022 10:58 AM, Alex Williamson wrote:
> > > On Mon, 12 Dec 2022 09:17:54 -0400
> > > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >   
> > >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> > >>  
> > >>> Thank you for your thoughtful response.  Rather than debate the degree of
> > >>> of vulnerability, I propose an alternate solution.  The technical crux of
> > >>> the matter is support for mediated devices.      
> > >>
> > >> I'm not sure I'm convinced about that. It is easy to make problematic
> > >> situations with mdevs, but that doesn't mean other cases don't exist
> > >> too eg what happens if userspace suspends and then immediately does
> > >> something to trigger a domain attachment? Doesn't it still deadlock
> > >> the kernel?  
> > > 
> > > The opportunity for that to deadlock isn't obvious to me, a replay
> > > would be stalled waiting for invalid vaddrs, but this is essentially
> > > the user deadlocking themselves.  There's also code there to handle the
> > > process getting killed while waiting, making it interruptible.  Thanks,  
> > 
> > I will submit new patches tomorrow to exclude mdevs.  Almost done.
> 
> I've dropped the removal commits from my next branch in the interim.

Woah, please don't do that - I already built and sent pull requests
assuming this, there are conflicts.

Why would we not revert everything from 6.2 - that is what we agreed
to do?

Jason

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Mon, 12 Dec 2022 19:08:57 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Mon, Dec 12, 2022 at 02:26:51PM -0700, Alex Williamson wrote:
> > On Mon, 12 Dec 2022 15:59:11 -0500
> > Steven Sistare <steven.sistare@oracle.com> wrote:
> >   
> > > On 12/12/2022 10:58 AM, Alex Williamson wrote:  
> > > > On Mon, 12 Dec 2022 09:17:54 -0400
> > > > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >     
> > > >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> > > >>    
> > > >>> Thank you for your thoughtful response.  Rather than debate the degree of
> > > >>> of vulnerability, I propose an alternate solution.  The technical crux of
> > > >>> the matter is support for mediated devices.        
> > > >>
> > > >> I'm not sure I'm convinced about that. It is easy to make problematic
> > > >> situations with mdevs, but that doesn't mean other cases don't exist
> > > >> too eg what happens if userspace suspends and then immediately does
> > > >> something to trigger a domain attachment? Doesn't it still deadlock
> > > >> the kernel?    
> > > > 
> > > > The opportunity for that to deadlock isn't obvious to me, a replay
> > > > would be stalled waiting for invalid vaddrs, but this is essentially
> > > > the user deadlocking themselves.  There's also code there to handle the
> > > > process getting killed while waiting, making it interruptible.  Thanks,    
> > > 
> > > I will submit new patches tomorrow to exclude mdevs.  Almost done.  
> > 
> > I've dropped the removal commits from my next branch in the interim.  
> 
> Woah, please don't do that - I already built and sent pull requests
> assuming this, there are conflicts.

I've done merges both ways with your iommufd pull request and don't see
any conflicts relative to these changes.  Kconfig, Makefile, and
vfio_main.c related to virq integration and group extraction are the
only conflicts.  Besides, it's already pushed and I don't have any
references to the old head, so someone would need to provide it if we
wanted to keep the old hashes.
 
> Why would we not revert everything from 6.2 - that is what we agreed
> to do?

The decision to revert was based on the current interface being buggy,
abandoned, and re-implemented.  It doesn't seem that there's much future
for the current interface, but Steve has stepped up to restrict the
current implementation to non-mdev devices, which resolves your concern
regarding unlimited user blocking of kernel threads afaict, and we'll
see what he does with locked memory.  If it looks ok, then I think it
reduces our urgency to remove it, and in particular, I think negates
our need to remove it from stable when we eventually do so anyway.
Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Jason Gunthorpe]

On Mon, Dec 12, 2022 at 04:29:48PM -0700, Alex Williamson wrote:
> On Mon, 12 Dec 2022 19:08:57 -0400
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Mon, Dec 12, 2022 at 02:26:51PM -0700, Alex Williamson wrote:
> > > On Mon, 12 Dec 2022 15:59:11 -0500
> > > Steven Sistare <steven.sistare@oracle.com> wrote:
> > >   
> > > > On 12/12/2022 10:58 AM, Alex Williamson wrote:  
> > > > > On Mon, 12 Dec 2022 09:17:54 -0400
> > > > > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > >     
> > > > >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> > > > >>    
> > > > >>> Thank you for your thoughtful response.  Rather than debate the degree of
> > > > >>> of vulnerability, I propose an alternate solution.  The technical crux of
> > > > >>> the matter is support for mediated devices.        
> > > > >>
> > > > >> I'm not sure I'm convinced about that. It is easy to make problematic
> > > > >> situations with mdevs, but that doesn't mean other cases don't exist
> > > > >> too eg what happens if userspace suspends and then immediately does
> > > > >> something to trigger a domain attachment? Doesn't it still deadlock
> > > > >> the kernel?    
> > > > > 
> > > > > The opportunity for that to deadlock isn't obvious to me, a replay
> > > > > would be stalled waiting for invalid vaddrs, but this is essentially
> > > > > the user deadlocking themselves.  There's also code there to handle the
> > > > > process getting killed while waiting, making it interruptible.  Thanks,    
> > > > 
> > > > I will submit new patches tomorrow to exclude mdevs.  Almost done.  
> > > 
> > > I've dropped the removal commits from my next branch in the interim.  
> > 
> > Woah, please don't do that - I already built and sent pull requests
> > assuming this, there are conflicts.
> 
> I've done merges both ways with your iommufd pull request and don't see
> any conflicts relative to these changes.  Kconfig, Makefile, and
> vfio_main.c related to virq integration and group extraction are the
> only conflicts. 

I got an extra hunk in the header file

> Besides, it's already pushed and I don't have any references to the
> old head, so someone would need to provide it if we wanted to keep
> the old hashes.

https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/tag/?h=for-linus-iommufd-merged
https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/commit/?h=for-linus-iommufd-merged&id=e9a1f0f32d86c05f01878a0448384a46a453abc7

> > Why would we not revert everything from 6.2 - that is what we agreed
> > to do?
> 
> The decision to revert was based on the current interface being buggy,
> abandoned, and re-implemented.  It doesn't seem that there's much future
> for the current interface, but Steve has stepped up to restrict the
> current implementation to non-mdev devices, which resolves your concern
> regarding unlimited user blocking of kernel threads afaict, and we'll
> see what he does with locked memory.  

Except nobody has seen this yet, and it can't go into 6.2 at this
point (see Linus's rather harsh remarks on late work for v6.2)

So we are punting on this for another kernel cycle. I don't like any
of this.

Regardless of what happens I need this all settled and the vfio tree
unchanging by Thursday to finalize the above tag..

Jason

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Alex Williamson]

On Mon, 12 Dec 2022 19:35:40 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Mon, Dec 12, 2022 at 04:29:48PM -0700, Alex Williamson wrote:
> > On Mon, 12 Dec 2022 19:08:57 -0400
> > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >   
> > > On Mon, Dec 12, 2022 at 02:26:51PM -0700, Alex Williamson wrote:  
> > > > On Mon, 12 Dec 2022 15:59:11 -0500
> > > > Steven Sistare <steven.sistare@oracle.com> wrote:
> > > >     
> > > > > On 12/12/2022 10:58 AM, Alex Williamson wrote:    
> > > > > > On Mon, 12 Dec 2022 09:17:54 -0400
> > > > > > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > > >       
> > > > > >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> > > > > >>      
> > > > > >>> Thank you for your thoughtful response.  Rather than debate the degree of
> > > > > >>> of vulnerability, I propose an alternate solution.  The technical crux of
> > > > > >>> the matter is support for mediated devices.          
> > > > > >>
> > > > > >> I'm not sure I'm convinced about that. It is easy to make problematic
> > > > > >> situations with mdevs, but that doesn't mean other cases don't exist
> > > > > >> too eg what happens if userspace suspends and then immediately does
> > > > > >> something to trigger a domain attachment? Doesn't it still deadlock
> > > > > >> the kernel?      
> > > > > > 
> > > > > > The opportunity for that to deadlock isn't obvious to me, a replay
> > > > > > would be stalled waiting for invalid vaddrs, but this is essentially
> > > > > > the user deadlocking themselves.  There's also code there to handle the
> > > > > > process getting killed while waiting, making it interruptible.  Thanks,      
> > > > > 
> > > > > I will submit new patches tomorrow to exclude mdevs.  Almost done.    
> > > > 
> > > > I've dropped the removal commits from my next branch in the interim.    
> > > 
> > > Woah, please don't do that - I already built and sent pull requests
> > > assuming this, there are conflicts.  
> > 
> > I've done merges both ways with your iommufd pull request and don't see
> > any conflicts relative to these changes.  Kconfig, Makefile, and
> > vfio_main.c related to virq integration and group extraction are the
> > only conflicts.   
> 
> I got an extra hunk in the header file
> 
> > Besides, it's already pushed and I don't have any references to the
> > old head, so someone would need to provide it if we wanted to keep
> > the old hashes.  
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/tag/?h=for-linus-iommufd-merged
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/commit/?h=for-linus-iommufd-merged&id=e9a1f0f32d86c05f01878a0448384a46a453abc7

Ok, I do still have that reference around.  Thanks.

> > > Why would we not revert everything from 6.2 - that is what we agreed
> > > to do?  
> > 
> > The decision to revert was based on the current interface being buggy,
> > abandoned, and re-implemented.  It doesn't seem that there's much future
> > for the current interface, but Steve has stepped up to restrict the
> > current implementation to non-mdev devices, which resolves your concern
> > regarding unlimited user blocking of kernel threads afaict, and we'll
> > see what he does with locked memory.    
> 
> Except nobody has seen this yet, and it can't go into 6.2 at this
> point (see Linus's rather harsh remarks on late work for v6.2)

We already outlined earlier in this thread the criteria that prompted
us to tag the revert for stable, which was Steve's primary objection in
the short term.  I can't in good faith push forward with a revert,
including stable, if Steve is working on a proposal to resolve the
issues prompting us to accelerate the code removal.  Depending on the
scope of Steve's proposal, I think we might be able to still consider
this a fix for v6.2.  Thanks,

Alex

Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

[Jason Gunthorpe]

On Mon, Dec 12, 2022 at 05:04:24PM -0700, Alex Williamson wrote:
> > > The decision to revert was based on the current interface being buggy,
> > > abandoned, and re-implemented.  It doesn't seem that there's much future
> > > for the current interface, but Steve has stepped up to restrict the
> > > current implementation to non-mdev devices, which resolves your concern
> > > regarding unlimited user blocking of kernel threads afaict, and we'll
> > > see what he does with locked memory.    
> > 
> > Except nobody has seen this yet, and it can't go into 6.2 at this
> > point (see Linus's rather harsh remarks on late work for v6.2)
> 
> We already outlined earlier in this thread the criteria that prompted
> us to tag the revert for stable, which was Steve's primary objection in
> the short term.

I still don't understand this, everyone running a distro deals with
the stuff. Even if you do blindly pull from a -stable branch instead
of cherry picking you only have to do the revert-revert once. Git is
good at this stuff.

Plus I have a doubt after all the backporting required to get vfio to
the required state that -stable patches are even going to work
anyhow..

> I can't in good faith push forward with a revert, including stable,
> if Steve is working on a proposal to resolve the issues prompting us
> to accelerate the code removal.  Depending on the scope of Steve's
> proposal, I think we might be able to still consider this a fix for
> v6.2.  Thanks,

Well, IMHO, you are better to send it for v6.2-rc1 than try to squeeze
it into this merge window and risk Linus's wrath

Jason