This patch factors out the split core detaching logic that could be
reused by in order feature into a dedicated function.

Acked-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 0f07a6637acb..96d7f165ec88 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
 	vq->split.desc_state[head].indir_desc = NULL;
 }
 
-static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
-			     void **ctx)
+static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
+					  unsigned int head,
+					  void **ctx)
 {
 	struct vring_desc_extra *extra;
 	unsigned int i;
@@ -824,8 +825,6 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 	}
 
 	vring_unmap_one_split(vq, &extra[i]);
-	vq->split.desc_extra[i].next = vq->free_head;
-	vq->free_head = head;
 
 	/* Plus final descriptor */
 	vq->vq.num_free++;
@@ -834,6 +833,17 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 		detach_indirect_split(vq, head);
 	else if (ctx)
 		*ctx = vq->split.desc_state[head].indir_desc;
+
+	return i;
+}
+
+static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
+			     void **ctx)
+{
+	unsigned int i = detach_buf_split_in_order(vq, head, ctx);
+
+	vq->split.desc_extra[i].next = vq->free_head;
+	vq->free_head = head;
 }
 
 static bool virtqueue_poll_split(const struct vring_virtqueue *vq,
-- 
2.31.1

On Mon, Oct 20, 2025 at 03:10:02PM +0800, Jason Wang wrote:
> This patch factors out the split core detaching logic that could be
> reused by in order feature into a dedicated function.
> 
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 0f07a6637acb..96d7f165ec88 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
>  	vq->split.desc_state[head].indir_desc = NULL;
>  }
>  
> -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> -			     void **ctx)
> +static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
> +					  unsigned int head,
> +					  void **ctx)


Well not really _inorder, right? This is a common function.
You want to call it __detach_buf_split or something maybe.

Additionally the very first line in there is:

        __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);

and the byte swap is not needed for inorder.
you could just do __cpu_to_virtio16(true, VRING_DESC_F_NEXT)




>  {
>  	struct vring_desc_extra *extra;
>  	unsigned int i;
> @@ -824,8 +825,6 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>  	}
>  
>  	vring_unmap_one_split(vq, &extra[i]);
> -	vq->split.desc_extra[i].next = vq->free_head;
> -	vq->free_head = head;
>  
>  	/* Plus final descriptor */
>  	vq->vq.num_free++;
> @@ -834,6 +833,17 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>  		detach_indirect_split(vq, head);
>  	else if (ctx)
>  		*ctx = vq->split.desc_state[head].indir_desc;
> +
> +	return i;
> +}
> +
> +static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> +			     void **ctx)
> +{
> +	unsigned int i = detach_buf_split_in_order(vq, head, ctx);
> +
> +	vq->split.desc_extra[i].next = vq->free_head;
> +	vq->free_head = head;
>  }
>  
>  static bool virtqueue_poll_split(const struct vring_virtqueue *vq,
> -- 
> 2.31.1

On Mon, Oct 20, 2025 at 11:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 20, 2025 at 03:10:02PM +0800, Jason Wang wrote:
> > This patch factors out the split core detaching logic that could be
> > reused by in order feature into a dedicated function.
> >
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 0f07a6637acb..96d7f165ec88 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
> >       vq->split.desc_state[head].indir_desc = NULL;
> >  }
> >
> > -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > -                          void **ctx)
> > +static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
> > +                                       unsigned int head,
> > +                                       void **ctx)
>
>
> Well not really _inorder, right? This is a common function.

Yes, but inorder is a subset for ooo so I use this name.

> You want to call it __detach_buf_split or something maybe.
>
> Additionally the very first line in there is:
>
>         __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>
> and the byte swap is not needed for inorder.

I don't see why?

> you could just do __cpu_to_virtio16(true, VRING_DESC_F_NEXT)

Probably you mean a leftover for hardening? E.g should we check
desc_extra.flag instead of desc.flag here?

while (vq->split.vring.desc[i].flags & nextflag) {
                vring_unmap_one_split(vq, &extra[i]);
        i = vq->split.desc_extra[i].next;
                vq->vq.num_free++;
        }

Thanks

On Tue, Oct 21, 2025 at 11:36:12AM +0800, Jason Wang wrote:
> On Mon, Oct 20, 2025 at 11:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 20, 2025 at 03:10:02PM +0800, Jason Wang wrote:
> > > This patch factors out the split core detaching logic that could be
> > > reused by in order feature into a dedicated function.
> > >
> > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 0f07a6637acb..96d7f165ec88 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
> > >       vq->split.desc_state[head].indir_desc = NULL;
> > >  }
> > >
> > > -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > -                          void **ctx)
> > > +static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
> > > +                                       unsigned int head,
> > > +                                       void **ctx)
> >
> >
> > Well not really _inorder, right? This is a common function.
> 
> Yes, but inorder is a subset for ooo so I use this name.

Can't say it is consistent. I suggest for example:
	_in_order -> specific to in order
	_ooo -> specific to ooo
	no suffix - common

or some other scheme where it's clear which is which.



> > You want to call it __detach_buf_split or something maybe.
> >
> > Additionally the very first line in there is:
> >
> >         __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
> >
> > and the byte swap is not needed for inorder.
> 
> I don't see why?

To be more precise we do need a swap we do not need it
conditional.


No, I mean inorder is a modern only feature. So we do not
need a branch in the inorder path,
you can use __cpu_to_virtio16 with true flag,
not cpu_to_virtio16.

> > you could just do __cpu_to_virtio16(true, VRING_DESC_F_NEXT)
> 
> Probably you mean a leftover for hardening? E.g should we check
> desc_extra.flag instead of desc.flag here?
> 
> while (vq->split.vring.desc[i].flags & nextflag) {
>                 vring_unmap_one_split(vq, &extra[i]);
>         i = vq->split.desc_extra[i].next;
>                 vq->vq.num_free++;
>         }
> 
> Thanks

If it is not exploitable we do not care.

-- 
MST

On Tue, Oct 21, 2025 at 4:27 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 21, 2025 at 11:36:12AM +0800, Jason Wang wrote:
> > On Mon, Oct 20, 2025 at 11:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Oct 20, 2025 at 03:10:02PM +0800, Jason Wang wrote:
> > > > This patch factors out the split core detaching logic that could be
> > > > reused by in order feature into a dedicated function.
> > > >
> > > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
> > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 0f07a6637acb..96d7f165ec88 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
> > > >       vq->split.desc_state[head].indir_desc = NULL;
> > > >  }
> > > >
> > > > -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > -                          void **ctx)
> > > > +static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
> > > > +                                       unsigned int head,
> > > > +                                       void **ctx)
> > >
> > >
> > > Well not really _inorder, right? This is a common function.
> >
> > Yes, but inorder is a subset for ooo so I use this name.
>
> Can't say it is consistent. I suggest for example:
>         _in_order -> specific to in order
>         _ooo -> specific to ooo
>         no suffix - common
>
> or some other scheme where it's clear which is which.

Will do that.

>
>
>
> > > You want to call it __detach_buf_split or something maybe.
> > >
> > > Additionally the very first line in there is:
> > >
> > >         __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
> > >
> > > and the byte swap is not needed for inorder.
> >
> > I don't see why?
>
> To be more precise we do need a swap we do not need it
> conditional.
>
>
> No, I mean inorder is a modern only feature. So we do not
> need a branch in the inorder path,
> you can use __cpu_to_virtio16 with true flag,
> not cpu_to_virtio16.

The problem is that the core logic will be reused by the ooo as well.
I'm not sure it's worthwhile to introduce a new flag parameter for the
logic like:

detach_buf_split_in_order()
{
        __virtio16 nextflag = __cpu_to_virtio16(true, VRING_DESC_F_NEXT);
        detach_buf_split(..., nextflag);
}

?

>
> > > you could just do __cpu_to_virtio16(true, VRING_DESC_F_NEXT)
> >
> > Probably you mean a leftover for hardening? E.g should we check
> > desc_extra.flag instead of desc.flag here?
> >
> > while (vq->split.vring.desc[i].flags & nextflag) {
> >                 vring_unmap_one_split(vq, &extra[i]);
> >         i = vq->split.desc_extra[i].next;
> >                 vq->vq.num_free++;
> >         }
> >
> > Thanks
>
> If it is not exploitable we do not care.

It looks like it can be triggered by the device as the descriptor ring
is writable. Will post a fix.

Thanks

>
> --
> MST
>
>

On Wed, Oct 22, 2025 at 12:00:53PM +0800, Jason Wang wrote:
> On Tue, Oct 21, 2025 at 4:27 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 21, 2025 at 11:36:12AM +0800, Jason Wang wrote:
> > > On Mon, Oct 20, 2025 at 11:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Oct 20, 2025 at 03:10:02PM +0800, Jason Wang wrote:
> > > > > This patch factors out the split core detaching logic that could be
> > > > > reused by in order feature into a dedicated function.
> > > > >
> > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 18 ++++++++++++++----
> > > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 0f07a6637acb..96d7f165ec88 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -802,8 +802,9 @@ static void detach_indirect_split(struct vring_virtqueue *vq,
> > > > >       vq->split.desc_state[head].indir_desc = NULL;
> > > > >  }
> > > > >
> > > > > -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > > -                          void **ctx)
> > > > > +static unsigned detach_buf_split_in_order(struct vring_virtqueue *vq,
> > > > > +                                       unsigned int head,
> > > > > +                                       void **ctx)
> > > >
> > > >
> > > > Well not really _inorder, right? This is a common function.
> > >
> > > Yes, but inorder is a subset for ooo so I use this name.
> >
> > Can't say it is consistent. I suggest for example:
> >         _in_order -> specific to in order
> >         _ooo -> specific to ooo
> >         no suffix - common
> >
> > or some other scheme where it's clear which is which.
> 
> Will do that.
> 
> >
> >
> >
> > > > You want to call it __detach_buf_split or something maybe.
> > > >
> > > > Additionally the very first line in there is:
> > > >
> > > >         __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
> > > >
> > > > and the byte swap is not needed for inorder.
> > >
> > > I don't see why?
> >
> > To be more precise we do need a swap we do not need it
> > conditional.
> >
> >
> > No, I mean inorder is a modern only feature. So we do not
> > need a branch in the inorder path,
> > you can use __cpu_to_virtio16 with true flag,
> > not cpu_to_virtio16.
> 
> The problem is that the core logic will be reused by the ooo as well.
> I'm not sure it's worthwhile to introduce a new flag parameter for the
> logic like:
> 
> detach_buf_split_in_order()
> {
>         __virtio16 nextflag = __cpu_to_virtio16(true, VRING_DESC_F_NEXT);
>         detach_buf_split(..., nextflag);
> }
> 
> ?

If it's common code then no.


> >
> > > > you could just do __cpu_to_virtio16(true, VRING_DESC_F_NEXT)
> > >
> > > Probably you mean a leftover for hardening? E.g should we check
> > > desc_extra.flag instead of desc.flag here?
> > >
> > > while (vq->split.vring.desc[i].flags & nextflag) {
> > >                 vring_unmap_one_split(vq, &extra[i]);
> > >         i = vq->split.desc_extra[i].next;
> > >                 vq->vq.num_free++;
> > >         }
> > >
> > > Thanks
> >
> > If it is not exploitable we do not care.
> 
> It looks like it can be triggered by the device as the descriptor ring
> is writable. Will post a fix.
> 
> Thanks

question is if the guest is exploitable as a result.

> >
> > --
> > MST
> >
> >