Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH

[Jason Wang]

On Thu, May 5, 2022 at 4:40 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> On Thu, May 05, 2022 at 04:26:24PM +0800, Jason Wang wrote:
> >On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> >>
> >> On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote:
> >> >On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> >> >>
> >> >> The simulator behaves like a ramdisk, so we don't have to do
> >> >> anything when a VIRTIO_BLK_T_FLUSH request is received, but it
> >> >> could be useful to test driver behavior.
> >> >>
> >> >> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver
> >> >> that we support the flush command.
> >> >>
> >> >> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> >> >> ---
> >> >>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12 ++++++++++++
> >> >>  1 file changed, 12 insertions(+)
> >> >>
> >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> index 42d401d43911..a6dd1233797c 100644
> >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> @@ -25,6 +25,7 @@
> >> >>  #define DRV_LICENSE  "GPL v2"
> >> >>
> >> >>  #define VDPASIM_BLK_FEATURES   (VDPASIM_FEATURES | \
> >> >> +                                (1ULL << VIRTIO_BLK_F_FLUSH)    | \
> >> >>                                  (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
> >> >>                                  (1ULL << VIRTIO_BLK_F_SEG_MAX)  | \
> >> >>                                  (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
> >> >> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
> >> >>                 pushed += bytes;
> >> >>                 break;
> >> >>
> >> >> +       case VIRTIO_BLK_T_FLUSH:
> >> >> +               if (sector != 0) {
> >> >> +                       dev_err(&vdpasim->vdpa.dev,
> >> >> +                               "A driver MUST set sector to 0 for a VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n",
> >> >> +                               sector);
> >> >
> >> >If this is something that could be triggered by userspace/guest, then
> >> >we should avoid this.
> >>
> >> It can only be triggered by an erratic driver.
> >
> >Right, so guest can try to DOS the host via this.
>
> Yes, but I don't expect the simulator to be used in the real world, but
> only for testing and development, so the user should have full control
> of the guest.

Right, but from kernel POV it's better to avoid any guest triggerable behaviour.

>
> >
> >>
> >> I was using the simulator to test a virtio-blk driver that I'm writing
> >> in userspace and I forgot to set `sector` to zero, so I thought it would
> >> be useful.
> >>
> >> Do you mean to remove the error message?
> >
> >Some like dev_warn_once() might be better here.
>
> We also have other checks we do for each request (in and out header
> length, etc.) where we use dev_err(), should we change those too?

I think so.

>
> I don't know, from a developer's point of view I'd prefer to have them
> all printed, but actually if we have a totally wrong driver in the
> guest, we risk to hang our host to print an infinite number of messages.

Or we can use pr_debug() or tracepoints. Then the log is enabled conditally.

>
> Maybe we should change all the errors in the data path to
> dev_warn_once() and keep returning VIRTIO_BLK_S_IOERR to the guest which
> will surely get angry and print something.
>
> If you agree, I'll send a patch to change all the printing and then
> repost this with your suggestion as well.

Yes.

Thanks

>
> Thanks,
> Stefano
>