[PATCH] i2c: virtio: Reset virtqueue before freeing interrupted requests

[PATCH] i2c: virtio: Reset virtqueue before freeing interrupted requests

[Guangshuo Li]

virtio_i2c_complete_reqs() waits for each queued request to complete
before releasing the per-message DMA-safe buffer. The request array is
then freed by virtio_i2c_xfer().

After wait_for_completion_interruptible() was introduced, the wait can
return before the current request has completed. In that case, the
request and the remaining requests may still be owned by the virtqueue
and may still be completed by the device later.

The error path nevertheless continues to release each message buffer and
virtio_i2c_xfer() frees the request array. A later device completion can
then write to freed request memory or call complete() on a freed
completion object.

Reset the virtqueue when the wait is interrupted so that outstanding
buffers are recovered before the request array and message buffers are
freed.

Fixes: a663b3c47ab1 ("i2c: virtio: Avoid hang by using interruptible completion wait")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/i2c/busses/i2c-virtio.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 5da6fef92bec..6ef09eda3ed0 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -117,12 +117,19 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
 		struct virtio_i2c_req *req = &reqs[i];
 
 		if (!failed) {
-			if (wait_for_completion_interruptible(&req->completion))
+			if (wait_for_completion_interruptible(&req->completion)) {
+				/*
+				 * The remaining buffers may still be owned by
+				 * the device. Reset the virtqueue before freeing
+				 * the request array and message buffers.
+				 */
+				virtqueue_reset(vq, NULL, NULL);
 				failed = true;
-			else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK)
+			} else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
 				failed = true;
-			else
+			} else {
 				j++;
+			}
 		}
 
 		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
-- 
2.43.0

Re: [PATCH] i2c: virtio: Reset virtqueue before freeing interrupted requests

[Viresh Kumar]

On 05-07-26, 23:59, Guangshuo Li wrote:
> virtio_i2c_complete_reqs() waits for each queued request to complete
> before releasing the per-message DMA-safe buffer. The request array is
> then freed by virtio_i2c_xfer().
> 
> After wait_for_completion_interruptible() was introduced, the wait can
> return before the current request has completed. In that case, the
> request and the remaining requests may still be owned by the virtqueue
> and may still be completed by the device later.
> 
> The error path nevertheless continues to release each message buffer and
> virtio_i2c_xfer() frees the request array. A later device completion can
> then write to freed request memory or call complete() on a freed
> completion object.
> 
> Reset the virtqueue when the wait is interrupted so that outstanding
> buffers are recovered before the request array and message buffers are
> freed.
> 
> Fixes: a663b3c47ab1 ("i2c: virtio: Avoid hang by using interruptible completion wait")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/i2c/busses/i2c-virtio.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index 5da6fef92bec..6ef09eda3ed0 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -117,12 +117,19 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>  		struct virtio_i2c_req *req = &reqs[i];
>  
>  		if (!failed) {
> -			if (wait_for_completion_interruptible(&req->completion))
> +			if (wait_for_completion_interruptible(&req->completion)) {
> +				/*
> +				 * The remaining buffers may still be owned by
> +				 * the device. Reset the virtqueue before freeing
> +				 * the request array and message buffers.
> +				 */
> +				virtqueue_reset(vq, NULL, NULL);
>  				failed = true;
> -			else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK)
> +			} else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
>  				failed = true;
> -			else
> +			} else {
>  				j++;
> +			}
>  		}
>  
>  		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);

+ Gavin, who was also trying to solve this path recently.

https://lore.kernel.org/all/20260610155834.79207-1-gavin.li@samsara.com/

-- 
viresh