* [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
@ 2022-04-28 15:12 Stefano Garzarella
2022-04-29 2:46 ` Jason Wang
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2022-04-28 15:12 UTC (permalink / raw)
To: virtualization
Cc: Jason Wang, linux-kernel, Michael S. Tsirkin, Stefan Hajnoczi,
Max Gurtovoy, Stefano Garzarella
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@redhat.com>
---
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);
+ status = VIRTIO_BLK_S_IOERR;
+ break;
+ }
+
+ break;
+
default:
dev_warn(&vdpasim->vdpa.dev,
"Unsupported request type %d\n", type);
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
2022-04-28 15:12 [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH Stefano Garzarella
@ 2022-04-29 2:46 ` Jason Wang
2022-04-29 7:14 ` Stefano Garzarella
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2022-04-29 2:46 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, linux-kernel, Michael S. Tsirkin,
Stefan Hajnoczi, Max Gurtovoy
On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella <sgarzare@redhat.com> 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@redhat.com>
> ---
> 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.
Thanks
> + status = VIRTIO_BLK_S_IOERR;
> + break;
> + }
> +
> + break;
> +
> default:
> dev_warn(&vdpasim->vdpa.dev,
> "Unsupported request type %d\n", type);
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
2022-04-29 2:46 ` Jason Wang
@ 2022-04-29 7:14 ` Stefano Garzarella
2022-05-05 8:26 ` Jason Wang
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2022-04-29 7:14 UTC (permalink / raw)
To: Jason Wang
Cc: virtualization, linux-kernel, Michael S. Tsirkin,
Stefan Hajnoczi, Max Gurtovoy
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@redhat.com> 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@redhat.com>
>> ---
>> 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.
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?
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
2022-04-29 7:14 ` Stefano Garzarella
@ 2022-05-05 8:26 ` Jason Wang
2022-05-05 8:40 ` Stefano Garzarella
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2022-05-05 8:26 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, linux-kernel, Michael S. Tsirkin,
Stefan Hajnoczi, Max Gurtovoy
On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella <sgarzare@redhat.com> 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@redhat.com> 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@redhat.com>
> >> ---
> >> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
2022-05-05 8:26 ` Jason Wang
@ 2022-05-05 8:40 ` Stefano Garzarella
2022-05-07 5:07 ` Jason Wang
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2022-05-05 8:40 UTC (permalink / raw)
To: Jason Wang
Cc: virtualization, linux-kernel, Michael S. Tsirkin,
Stefan Hajnoczi, Max Gurtovoy
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@redhat.com> 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@redhat.com> 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@redhat.com>
>> >> ---
>> >> 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.
>
>>
>> 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 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.
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.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
2022-05-05 8:40 ` Stefano Garzarella
@ 2022-05-07 5:07 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2022-05-07 5:07 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, linux-kernel, Michael S. Tsirkin,
Stefan Hajnoczi, Max Gurtovoy
On Thu, May 5, 2022 at 4:40 PM Stefano Garzarella <sgarzare@redhat.com> 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@redhat.com> 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@redhat.com> 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@redhat.com>
> >> >> ---
> >> >> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-07 5:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28 15:12 [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH Stefano Garzarella
2022-04-29 2:46 ` Jason Wang
2022-04-29 7:14 ` Stefano Garzarella
2022-05-05 8:26 ` Jason Wang
2022-05-05 8:40 ` Stefano Garzarella
2022-05-07 5:07 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).