Re: [PATCH v3] virtio-rng: return available data with O_NONBLOCK

[Michael S. Tsirkin]

On Tue, Aug 11, 2020 at 04:42:32PM +0200, Laurent Vivier wrote:
> On 11/08/2020 16:28, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If a program opens /dev/hwrng with O_NONBLOCK and uses poll() and
> > non-blocking read() to retrieve random data, it ends up in a tight
> > loop with poll() always returning POLLIN and read() returning EAGAIN.
> > This repeats forever until some process makes a blocking read() call.
> > The reason is that virtio_read() always returns 0 in non-blocking mode,
> > even if data is available. Worse, it fetches random data from the
> > hypervisor after every non-blocking call, without ever using this data.
> > 
> > The following test program illustrates the behavior and can be used
> > for testing and experiments. The problem will only be seen if all
> > tasks use non-blocking access; otherwise the blocking reads will
> > "recharge" the random pool and cause other, non-blocking reads to
> > succeed at least sometimes.
> > 
> > /* Whether to use non-blocking mode in a task, problem occurs if CONDITION 
> > is 1 */
> > //#define CONDITION (getpid() % 2 != 0)
> > 
> > static volatile sig_atomic_t stop;
> > static void handler(int sig __attribute__((unused))) { stop = 1; }
> > 
> > static void loop(int fd, int sec)
> > {
> >     struct pollfd pfd = { .fd = fd, .events  = POLLIN, };
> >     unsigned long errors = 0, eagains = 0, bytes = 0, succ = 0;
> >     int size, rc, rd;
> > 
> >     srandom(getpid());
> >     if (CONDITION && fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK) == 
> > -1)
> >             perror("fcntl");
> >     size = MINBUFSIZ + random() % (MAXBUFSIZ - MINBUFSIZ + 1);
> > 
> >     for(;;) {
> >             char buf[size];
> > 
> >             if (stop)
> >                     break;
> >             rc = poll(&pfd, 1, sec);
> >             if (rc > 0) {
> >                     rd = read(fd, buf, sizeof(buf));
> >                     if (rd == -1 && errno == EAGAIN)
> >                             eagains++;
> >                     else if (rd == -1)
> >                             errors++;
> >                     else {
> >                             succ++;
> >                             bytes += rd;
> >                             write(1, buf, sizeof(buf));
> >                     }
> >             } else if (rc == -1) {
> >                     if (errno != EINTR)
> >                             perror("poll");
> >                     break;
> >             } else
> >                     fprintf(stderr, "poll: timeout\n");
> >     }
> >     fprintf(stderr,
> >             "pid %d %sblocking, bufsize %d, %d seconds, %lu bytes read, %lu 
> > success, %lu eagain, %lu errors\n",
> >             getpid(), CONDITION ? "non-" : "", size, sec, bytes, succ, 
> > eagains, errors);
> > }
> > 
> > int main(void)
> > {
> >     int fd;
> > 
> >     fork(); fork();
> >     fd = open("/dev/hwrng", O_RDONLY);
> >     if (fd == -1) {
> >             perror("open");
> >             return 1;
> >     };
> >     signal(SIGALRM, handler);
> >     alarm(SECONDS);
> >     loop(fd, SECONDS);
> >     close(fd);
> >     wait(NULL);
> >     return 0;
> > }
> > 
> > void loop(int fd)
> > {
> >         struct pollfd pfd0 = { .fd = fd, .events  = POLLIN, };
> >         int rc;
> >         unsigned int n;
> > 
> >         for (n = LOOPS; n > 0; n--) {
> >                 struct pollfd pfd = pfd0;
> >                 char buf[SIZE];
> > 
> >                 rc = poll(&pfd, 1, 1);
> >                 if (rc > 0) {
> >                         int rd = read(fd, buf, sizeof(buf));
> > 
> >                         if (rd == -1)
> >                                 perror("read");
> >                         else
> >                                 printf("read %d bytes\n", rd);
> >                 } else if (rc == -1)
> >                         perror("poll");
> >                 else
> >                         fprintf(stderr, "timeout\n");
> > 
> >         }
> > }
> > 
> > int main(void)
> > {
> >         int fd;
> > 
> >         fd = open("/dev/hwrng", O_RDONLY|O_NONBLOCK);
> >         if (fd == -1) {
> >                 perror("open");
> >                 return 1;
> >         };
> >         loop(fd);
> >         close(fd);
> >         return 0;
> > }
> > 
> > This can be observed in the real word e.g. with nested qemu/KVM virtual
> > machines, if both the "outer" and "inner" VMs have a virtio-rng device.
> > If the "inner" VM requests random data, qemu running in the "outer" VM
> > uses this device in a non-blocking manner like the test program above.
> > 
> > Fix it by returning available data if a previous hypervisor call has
> > completed. I tested this patch with the program above, and with rng-tools.
> > 
> > v2 -> v3: Simplified the implementation as suggested by Laurent Vivier
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  drivers/char/hw_random/virtio-rng.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/char/hw_random/virtio-rng.c 
> > b/drivers/char/hw_random/virtio-rng.c
> > index a90001e02bf7..8eaeceecb41e 100644
> > --- a/drivers/char/hw_random/virtio-rng.c
> > +++ b/drivers/char/hw_random/virtio-rng.c
> > @@ -65,7 +65,7 @@ static int virtio_read(struct hwrng *rng, void *buf, 
> > size_t size, bool wait)
> >             register_buffer(vi, buf, size);
> >     }
> >  
> > -   if (!wait)
> > +   if (!wait && !completion_done(&vi->have_data))
> >             return 0;
> >  
> >     ret = wait_for_completion_killable(&vi->have_data);
> > @@ -74,7 +74,7 @@ static int virtio_read(struct hwrng *rng, void *buf, 
> > size_t size, bool wait)
> >  
> >     vi->busy = false;
> >  
> > -   return vi->data_avail;
> > +   return min_t(size_t, size, vi->data_avail);
> >  }
> >  
> >  static void virtio_cleanup(struct hwrng *rng)
> > 
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Laurent didn't we agree the real fix is private buffers in the driver,
and copying out from there?

-- 
MST