Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] virtio-rng: data race on data_avail leads to deadlock --
commit 2904ec0356e25704559279a6336e7fed895c58d2 Author: Thorsten Blum <[email protected]> hwrng: virtio - prevent use-after-free after a failed restore remove_common() frees the driver data during freeze, but does not clear vdev->priv, which points to freed memory. Clear vdev->priv on the error cleanup path and before freeing the driver data in remove_common(). Also reset the device and return early from remove_common() if vdev->priv is already NULL. > diff --git a/drivers/char/hw_random/virtio-rng.c > b/drivers/char/hw_random/virtio-rng.c > index f9982217247dd..45d87c0614a5e 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c [ ... ] > @@ -198,6 +199,11 @@ static void remove_common(struct virtio_device *vdev) > { > struct virtrng_info *vi = vdev->priv; > > + if (!vi) { > + virtio_reset_device(vdev); > + return; > + } > + > vi->hwrng_removed = true; > vi->data_avail = 0; [Severity: High] Does this code race with a reader and cause a deadlock? In copy_data() in drivers/char/hw_random/virtio-rng.c, vi->data_avail is read, memcpy() is called, and then size is subtracted: drivers/char/hw_random/virtio-rng.c:copy_data() { ... memcpy(buf, vi->data + idx, size); vi->data_idx += size; vi->data_avail -= size; if (vi->data_avail == 0) request_entropy(vi); return size; } If remove_common() sets vi->data_avail to 0 before the subtraction, will this cause an unsigned integer underflow, leaving vi->data_avail as a huge value? Since vi->data_avail != 0, copy_data() would skip calling request_entropy(). virtio_read() would then consume the single completion from remove_common(). Seeing a huge data_avail instead of 0, virtio_read() would continue looping and eventually block forever in wait_for_completion_killable() because no pending entropy request will trigger a new completion: drivers/char/hw_random/virtio-rng.c:virtio_read() { ... while (size != 0) { /* data_avail is 0 but a request is pending */ ret = wait_for_completion_killable(&vi->have_data); ... } Could this cause hwrng_unregister() in remove_common() to block forever waiting for the reader to drop its reference, deadlocking device removal? > vi->data_idx = 0; > complete(&vi->have_data); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
