Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH

[Jason Wang]

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.

>
> 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.

Thanks

>
> Thanks,
> Stefano
>