hwrng core uses two buffers that can be mixed in the virtio-rng queue. This series fixes the problem by adding an internal buffer in virtio-rng. Once the internal buffer is added, we can fix two other problems: - to be able to release the driver without waiting the device releases the buffer - actually returns some data when wait=0 as we can have some already available data It also tries to improve the performance by always having a buffer in the queue of the device. Laurent Vivier (4): hwrng: virtio - add an internal buffer hwrng: virtio - don't wait on cleanup hwrng: virtio - don't waste entropy hwrng: virtio - always add a pending request drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 21 deletions(-) -- 2.31.1
hwrng core uses two buffers that can be mixed in the
virtio-rng queue.
If the buffer is provided with wait=0 it is enqueued in the
virtio-rng queue but unused by the caller.
On the next call, core provides another buffer but the
first one is filled instead and the new one queued.
And the caller reads the data from the new one that is not
updated, and the data in the first one are lost.
To avoid this mix, virtio-rng needs to use its own unique
internal buffer at a cost of a data copy to the caller buffer.
Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index a90001e02bf7..208c547dcac1 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
struct virtrng_info {
struct hwrng hwrng;
struct virtqueue *vq;
- struct completion have_data;
char name[25];
- unsigned int data_avail;
int index;
bool busy;
bool hwrng_register_done;
bool hwrng_removed;
+ /* data transfer */
+ struct completion have_data;
+ unsigned int data_avail;
+ /* minimal size returned by rng_buffer_size() */
+#if SMP_CACHE_BYTES < 32
+ u8 data[32];
+#else
+ u8 data[SMP_CACHE_BYTES];
+#endif
};
static void random_recv_done(struct virtqueue *vq)
@@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
}
/* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
+static void register_buffer(struct virtrng_info *vi)
{
struct scatterlist sg;
- sg_init_one(&sg, buf, size);
+ sg_init_one(&sg, vi->data, sizeof(vi->data));
/* There should always be room for one buffer. */
- virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
+ virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
virtqueue_kick(vi->vq);
}
@@ -55,6 +62,8 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
{
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+ unsigned int chunk;
+ size_t read;
if (vi->hwrng_removed)
return -ENODEV;
@@ -62,19 +71,33 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
if (!vi->busy) {
vi->busy = true;
reinit_completion(&vi->have_data);
- register_buffer(vi, buf, size);
+ register_buffer(vi);
}
if (!wait)
return 0;
- ret = wait_for_completion_killable(&vi->have_data);
- if (ret < 0)
- return ret;
+ read = 0;
+ while (size != 0) {
+ ret = wait_for_completion_killable(&vi->have_data);
+ if (ret < 0)
+ return ret;
+
+ chunk = min_t(unsigned int, size, vi->data_avail);
+ memcpy(buf + read, vi->data, chunk);
+ read += chunk;
+ size -= chunk;
+ vi->data_avail = 0;
+
+ if (size != 0) {
+ reinit_completion(&vi->have_data);
+ register_buffer(vi);
+ }
+ }
vi->busy = false;
- return vi->data_avail;
+ return read;
}
static void virtio_cleanup(struct hwrng *rng)
--
2.31.1
When virtio-rng device was dropped by the hwrng core we were forced to wait the buffer to come back from the device to not have remaining ongoing operation that could spoil the buffer. But now, as the buffer is internal to the virtio-rng we can release the waiting loop immediately, the buffer will be retrieve and use when the virtio-rng driver will be selected again. This avoids to hang on an rng_current write command if the virtio-rng device is blocked by a lack of entropy. This allows to select another entropy source if the current one is empty. Signed-off-by: Laurent Vivier <lvivier at redhat.com> --- drivers/char/hw_random/virtio-rng.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 208c547dcac1..173aeea835bb 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -82,6 +82,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) ret = wait_for_completion_killable(&vi->have_data); if (ret < 0) return ret; + /* if vi->data_avail is 0, we have been interrupted + * by a cleanup, but buffer stays in the queue + */ + if (vi->data_avail == 0) + return read; chunk = min_t(unsigned int, size, vi->data_avail); memcpy(buf + read, vi->data, chunk); @@ -105,7 +110,7 @@ static void virtio_cleanup(struct hwrng *rng) struct virtrng_info *vi = (struct virtrng_info *)rng->priv; if (vi->busy) - wait_for_completion(&vi->have_data); + complete(&vi->have_data); } static int probe_common(struct virtio_device *vdev) -- 2.31.1
if we don't use all the entropy available in the buffer, keep it
and use it later.
Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
drivers/char/hw_random/virtio-rng.c | 52 +++++++++++++++++++----------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index 173aeea835bb..8ba97cf4ca8f 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -26,6 +26,7 @@ struct virtrng_info {
/* data transfer */
struct completion have_data;
unsigned int data_avail;
+ unsigned int data_idx;
/* minimal size returned by rng_buffer_size() */
#if SMP_CACHE_BYTES < 32
u8 data[32];
@@ -42,6 +43,9 @@ static void random_recv_done(struct virtqueue *vq)
if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
return;
+ vi->data_idx = 0;
+ vi->busy = false;
+
complete(&vi->have_data);
}
@@ -58,6 +62,16 @@ static void register_buffer(struct virtrng_info *vi)
virtqueue_kick(vi->vq);
}
+static unsigned int copy_data(struct virtrng_info *vi, void *buf,
+ unsigned int size)
+{
+ size = min_t(unsigned int, size, vi->data_avail);
+ memcpy(buf, vi->data + vi->data_idx, size);
+ vi->data_idx += size;
+ vi->data_avail -= size;
+ return size;
+}
+
static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
{
int ret;
@@ -68,17 +82,29 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
if (vi->hwrng_removed)
return -ENODEV;
- if (!vi->busy) {
- vi->busy = true;
- reinit_completion(&vi->have_data);
- register_buffer(vi);
+ read = 0;
+
+ /* copy available data */
+ if (vi->data_avail) {
+ chunk = copy_data(vi, buf, size);
+ size -= chunk;
+ read += chunk;
}
if (!wait)
- return 0;
+ return read;
- read = 0;
+ /* We have already copied available entropy,
+ * so either size is 0 or data_avail is 0
+ */
while (size != 0) {
+ /* data_avail is 0 */
+ if (!vi->busy) {
+ /* no pending request, ask for more */
+ vi->busy = true;
+ reinit_completion(&vi->have_data);
+ register_buffer(vi);
+ }
ret = wait_for_completion_killable(&vi->have_data);
if (ret < 0)
return ret;
@@ -88,20 +114,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
if (vi->data_avail == 0)
return read;
- chunk = min_t(unsigned int, size, vi->data_avail);
- memcpy(buf + read, vi->data, chunk);
- read += chunk;
+ chunk = copy_data(vi, buf + read, size);
size -= chunk;
- vi->data_avail = 0;
-
- if (size != 0) {
- reinit_completion(&vi->have_data);
- register_buffer(vi);
- }
+ read += chunk;
}
- vi->busy = false;
-
return read;
}
@@ -161,6 +178,7 @@ static void remove_common(struct virtio_device *vdev)
vi->hwrng_removed = true;
vi->data_avail = 0;
+ vi->data_idx = 0;
complete(&vi->have_data);
vdev->config->reset(vdev);
vi->busy = false;
--
2.31.1
Laurent Vivier
2021-Sep-22 17:09 UTC
[PATCH 4/4] hwrng: virtio - always add a pending request
If we ensure we have already some data available by enqueuing
again the buffer once data are exhausted, we can return what we
have without waiting for the device answer.
Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
drivers/char/hw_random/virtio-rng.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index 8ba97cf4ca8f..0b3c9b643495 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -20,7 +20,6 @@ struct virtrng_info {
struct virtqueue *vq;
char name[25];
int index;
- bool busy;
bool hwrng_register_done;
bool hwrng_removed;
/* data transfer */
@@ -44,16 +43,16 @@ static void random_recv_done(struct virtqueue *vq)
return;
vi->data_idx = 0;
- vi->busy = false;
complete(&vi->have_data);
}
-/* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi)
+static void request_entropy(struct virtrng_info *vi)
{
struct scatterlist sg;
+ reinit_completion(&vi->have_data);
+
sg_init_one(&sg, vi->data, sizeof(vi->data));
/* There should always be room for one buffer. */
@@ -69,6 +68,8 @@ static unsigned int copy_data(struct virtrng_info *vi, void
*buf,
memcpy(buf, vi->data + vi->data_idx, size);
vi->data_idx += size;
vi->data_avail -= size;
+ if (vi->data_avail == 0)
+ request_entropy(vi);
return size;
}
@@ -98,13 +99,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
* so either size is 0 or data_avail is 0
*/
while (size != 0) {
- /* data_avail is 0 */
- if (!vi->busy) {
- /* no pending request, ask for more */
- vi->busy = true;
- reinit_completion(&vi->have_data);
- register_buffer(vi);
- }
+ /* data_avail is 0 but a request is pending */
ret = wait_for_completion_killable(&vi->have_data);
if (ret < 0)
return ret;
@@ -126,8 +121,7 @@ static void virtio_cleanup(struct hwrng *rng)
{
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- if (vi->busy)
- complete(&vi->have_data);
+ complete(&vi->have_data);
}
static int probe_common(struct virtio_device *vdev)
@@ -163,6 +157,9 @@ static int probe_common(struct virtio_device *vdev)
goto err_find;
}
+ /* we always have a pending entropy request */
+ request_entropy(vi);
+
return 0;
err_find:
@@ -181,7 +178,6 @@ static void remove_common(struct virtio_device *vdev)
vi->data_idx = 0;
complete(&vi->have_data);
vdev->config->reset(vdev);
- vi->busy = false;
if (vi->hwrng_register_done)
hwrng_unregister(&vi->hwrng);
vdev->config->del_vqs(vdev);
--
2.31.1
Alexander Potapenko
2021-Sep-22 17:50 UTC
[PATCH 0/4] hwrng: virtio - add an internal buffer
On Wed, Sep 22, 2021 at 7:09 PM Laurent Vivier <lvivier at redhat.com> wrote:> > hwrng core uses two buffers that can be mixed in the virtio-rng queue. > > This series fixes the problem by adding an internal buffer in virtio-rng. > > Once the internal buffer is added, we can fix two other problems: > > - to be able to release the driver without waiting the device releases the > buffer > > - actually returns some data when wait=0 as we can have some already > available data > > It also tries to improve the performance by always having a buffer in > the queue of the device.Tested-by: Alexander Potapenko <glider at google.com> for the series With these four patches applied, KMSAN stops reporting boot-time errors in _mix_pool_bytes reported here: https://www.spinics.net/lists/linux-virtualization/msg46310.html> Laurent Vivier (4): > hwrng: virtio - add an internal buffer > hwrng: virtio - don't wait on cleanup > hwrng: virtio - don't waste entropy > hwrng: virtio - always add a pending request > > drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++-------- > 1 file changed, 63 insertions(+), 21 deletions(-) > > -- > 2.31.1 > >-- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Stra?e, 33 80636 M?nchen Gesch?ftsf?hrer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
On 22/09/2021 19:08, Laurent Vivier wrote:> hwrng core uses two buffers that can be mixed in the virtio-rng queue. > > This series fixes the problem by adding an internal buffer in virtio-rng. > > Once the internal buffer is added, we can fix two other problems: > > - to be able to release the driver without waiting the device releases the > buffer > > - actually returns some data when wait=0 as we can have some already > available data > > It also tries to improve the performance by always having a buffer in > the queue of the device. > > Laurent Vivier (4): > hwrng: virtio - add an internal buffer > hwrng: virtio - don't wait on cleanup > hwrng: virtio - don't waste entropy > hwrng: virtio - always add a pending request > > drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++-------- > 1 file changed, 63 insertions(+), 21 deletions(-) >Any comment? Do we need a v2? Thanks, Laurent