Re: [PATCH v1] Revert "i2c: virtio: Avoid hang by using interruptible completion wait"

[Andi Shyti]

Viresh, can I have a comment from you here, please?

Thanks,
Andi

On Tue, Sep 22, 2026 at 01:17:06AM -0400, Michael S. Tsirkin wrote:
> On Tue, Sep 22, 2026 at 01:02:51AM -0400, Yuho Choi wrote:
> > This reverts commit a663b3c47ab10f66130818cf94eb59c971541c3f.
> > 
> > When a transfer is interrupted by a signal, virtio_i2c_complete_reqs()
> > stops waiting for the remaining requests and virtio_i2c_xfer() frees the
> > reqs array while the virtqueue descriptors are still in flight on the
> > device.  The backend can then write into freed memory, and
> > virtio_i2c_msg_done() calls complete() on already-freed requests.
> > 
> > Commit 84e1d0bf1d71 ("i2c: virtio: disable timeout handling") removed the
> > exact same failure mode caused by timeouts, concluding there was no simple
> > fix because the buffers must be held until the device returns them.  A hang
> > due to an unresponsive backend is preferable to guest memory corruption.
> > Restore the unconditional wait until request lifetime can be decoupled
> > safely.
> > 
> > Fixes: a663b3c47ab1 ("i2c: virtio: Avoid hang by using interruptible completion wait")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Yuho Choi <oss.patchbox@xxxxxxxxx>
> 
> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> 
> > ---
> > A proper interruptible wait requires refcounting requests and bounce
> > buffering to hold memory until the backend returns descriptors (similar
> > to virtio_rtc/virtio_pmem). Since that is a larger rework unsuitable for
> > stable, revert to uninterruptible wait first.
> 
> another way is ring or device reset, if that is acceptable.
> 
> >  drivers/i2c/busses/i2c-virtio.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> > index 5da6fef92bec3..581e55b5c65ba 100644
> > --- a/drivers/i2c/busses/i2c-virtio.c
> > +++ b/drivers/i2c/busses/i2c-virtio.c
> > @@ -116,16 +116,15 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> >  	for (i = 0; i < num; i++) {
> >  		struct virtio_i2c_req *req = &reqs[i];
> >  
> > -		if (!failed) {
> > -			if (wait_for_completion_interruptible(&req->completion))
> > -				failed = true;
> > -			else if (req->in_hdr.status != VIRTIO_I2C_MSG_OK)
> > -				failed = true;
> > -			else
> > -				j++;
> > -		}
> > +		wait_for_completion(&req->completion);
> > +
> > +		if (!failed && req->in_hdr.status != VIRTIO_I2C_MSG_OK)
> > +			failed = true;
> >  
> >  		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
> > +
> > +		if (!failed)
> > +			j++;
> >  	}
> >  
> >  	return j;
> > 
> > base-commit: f0100363d8c374bd8e9ea7c9ba02744f0b802ca4
> > -- 
> > 2.43.0
>