| Message ID | 20190131151914.164903-3-sgarzare@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: add DISCARD and WRITE ZEROES features | expand |
* Stefano Garzarella (sgarzare@redhat.com) wrote: > In order to avoid migration issues, we enable DISCARD and > WRITE ZEROES features only for machine type >= 4.0 > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > hw/block/virtio-blk.c | 2 ++ > hw/core/machine.c | 1 + > include/hw/virtio/virtio-blk.h | 1 + > 3 files changed, 4 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 8a6754d9a2..542ec52536 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > IOThread *), > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > + true), I think that's OK, but do you really want a DEFINE_PROP_BOOL and a bool discard_wzeroes? I think DEFINE_PROP_BIT is mostly used for a flag word where each property is one more bit in the field. Dave > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 2629515363..ce98857af0 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = { > { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" }, > { "tpm-crb", "ppi", "false" }, > { "tpm-tis", "ppi", "false" }, > + { "virtio-blk-device", "discard-wzeroes", "false" }, > }; > const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1); > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index 5117431d96..c336afb4cd 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -40,6 +40,7 @@ struct VirtIOBlkConf > uint32_t request_merging; > uint16_t num_queues; > uint16_t queue_size; > + uint32_t discard_wzeroes; > }; > > struct VirtIOBlockDataPlane; > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote: > * Stefano Garzarella (sgarzare@redhat.com) wrote: > > In order to avoid migration issues, we enable DISCARD and > > WRITE ZEROES features only for machine type >= 4.0 > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > --- > > hw/block/virtio-blk.c | 2 ++ > > hw/core/machine.c | 1 + > > include/hw/virtio/virtio-blk.h | 1 + > > 3 files changed, 4 insertions(+) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 8a6754d9a2..542ec52536 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > IOThread *), > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > + true), > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and > a bool discard_wzeroes? > I think DEFINE_PROP_BIT is mostly used for a flag word where each > property is one more bit in the field. > > Dave > Hi Dave, I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but looking in the virtio-blk.c, I found that also other boolean like "config-wce", "scsi", and "request-merging" was defined with DEFINE_PROP_BIT, so I followed this trand. But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it! Thanks, Stefano
* Stefano Garzarella (sgarzare@redhat.com) wrote: > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote: > > * Stefano Garzarella (sgarzare@redhat.com) wrote: > > > In order to avoid migration issues, we enable DISCARD and > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > hw/block/virtio-blk.c | 2 ++ > > > hw/core/machine.c | 1 + > > > include/hw/virtio/virtio-blk.h | 1 + > > > 3 files changed, 4 insertions(+) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index 8a6754d9a2..542ec52536 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > IOThread *), > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > + true), > > > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and > > a bool discard_wzeroes? > > I think DEFINE_PROP_BIT is mostly used for a flag word where each > > property is one more bit in the field. > > > > Dave > > > > Hi Dave, > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but > looking in the virtio-blk.c, I found that also other boolean like > "config-wce", "scsi", and "request-merging" was defined with > DEFINE_PROP_BIT, so I followed this trand. Oh odd, I don't see why it's done like that - unless that's visible to the guest somehow directly. The _BIT version is more commonly used when you have a flag word that's got multiple bits defined in it. > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it! Thanks! Dave > Thanks, > Stefano -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote: > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote: > > * Stefano Garzarella (sgarzare@redhat.com) wrote: > > > In order to avoid migration issues, we enable DISCARD and > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > hw/block/virtio-blk.c | 2 ++ > > > hw/core/machine.c | 1 + > > > include/hw/virtio/virtio-blk.h | 1 + > > > 3 files changed, 4 insertions(+) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index 8a6754d9a2..542ec52536 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > IOThread *), > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > + true), > > > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and > > a bool discard_wzeroes? > > I think DEFINE_PROP_BIT is mostly used for a flag word where each > > property is one more bit in the field. > > > > Dave > > > > Hi Dave, > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but > looking in the virtio-blk.c, I found that also other boolean like > "config-wce", "scsi", and "request-merging" was defined with > DEFINE_PROP_BIT, so I followed this trand. > > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it! > > Thanks, > Stefano I wonder why doesn't virtio-blk set bits directly in host_features? For example this is how virtio-net does it. This would remove the need for virtio_add_feature calls.
On Thu, Jan 31, 2019 at 11:43:07AM -0500, Michael S. Tsirkin wrote: > On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote: > > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote: > > > * Stefano Garzarella (sgarzare@redhat.com) wrote: > > > > In order to avoid migration issues, we enable DISCARD and > > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > --- > > > > hw/block/virtio-blk.c | 2 ++ > > > > hw/core/machine.c | 1 + > > > > include/hw/virtio/virtio-blk.h | 1 + > > > > 3 files changed, 4 insertions(+) > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > index 8a6754d9a2..542ec52536 100644 > > > > --- a/hw/block/virtio-blk.c > > > > +++ b/hw/block/virtio-blk.c > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > > IOThread *), > > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > > + true), > > > > > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and > > > a bool discard_wzeroes? > > > I think DEFINE_PROP_BIT is mostly used for a flag word where each > > > property is one more bit in the field. > > > > > > Dave > > > > > > > Hi Dave, > > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but > > looking in the virtio-blk.c, I found that also other boolean like > > "config-wce", "scsi", and "request-merging" was defined with > > DEFINE_PROP_BIT, so I followed this trand. > > > > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it! > > > > Thanks, > > Stefano > > I wonder why doesn't virtio-blk set bits directly in host_features? > For example this is how virtio-net does it. > This would remove the need for virtio_add_feature calls. Maybe this should be the best approach! What do you think if I send a trivial patch to add host_features variable like for virtio-net and change the "config_wce" and "scsi" definition? Then I will change also this patch to set directly the bits. Thanks, Stefano
On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > In order to avoid migration issues, we enable DISCARD and > WRITE ZEROES features only for machine type >= 4.0 Please use two separate properties that correspond to the VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature bits.
On Fri, Feb 01, 2019 at 12:29:28PM +0800, Stefan Hajnoczi wrote: > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > In order to avoid migration issues, we enable DISCARD and > > WRITE ZEROES features only for machine type >= 4.0 > > Please use two separate properties that correspond to the > VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature > bits. Okay. As Michael suggested, what do you think if I use a similar approach of virtio-net, adding an host_features variable and setting a corresponding feature bit? Thanks, Stefano
On Fri, Feb 01, 2019 at 10:09:08AM +0100, Stefano Garzarella wrote: > On Fri, Feb 01, 2019 at 12:29:28PM +0800, Stefan Hajnoczi wrote: > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > > In order to avoid migration issues, we enable DISCARD and > > > WRITE ZEROES features only for machine type >= 4.0 > > > > Please use two separate properties that correspond to the > > VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature > > bits. > > Okay. > As Michael suggested, what do you think if I use a similar approach of > virtio-net, adding an host_features variable and setting a corresponding > feature bit? Yes, that's a clean way of supporting feature bit options. Stefan
On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > In order to avoid migration issues, we enable DISCARD and > WRITE ZEROES features only for machine type >= 4.0 > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > hw/block/virtio-blk.c | 2 ++ > hw/core/machine.c | 1 + > include/hw/virtio/virtio-blk.h | 1 + > 3 files changed, 4 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 8a6754d9a2..542ec52536 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > IOThread *), > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > + true), > DEFINE_PROP_END_OF_LIST(), > }; > Thinking about it, are there security implications for discard? Should we make it default false? > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 2629515363..ce98857af0 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = { > { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" }, > { "tpm-crb", "ppi", "false" }, > { "tpm-tis", "ppi", "false" }, > + { "virtio-blk-device", "discard-wzeroes", "false" }, > }; > const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1); > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index 5117431d96..c336afb4cd 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -40,6 +40,7 @@ struct VirtIOBlkConf > uint32_t request_merging; > uint16_t num_queues; > uint16_t queue_size; > + uint32_t discard_wzeroes; > }; > > struct VirtIOBlockDataPlane; > -- > 2.20.1
On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > In order to avoid migration issues, we enable DISCARD and > > WRITE ZEROES features only for machine type >= 4.0 > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > --- > > hw/block/virtio-blk.c | 2 ++ > > hw/core/machine.c | 1 + > > include/hw/virtio/virtio-blk.h | 1 + > > 3 files changed, 4 insertions(+) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 8a6754d9a2..542ec52536 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > IOThread *), > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > + true), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > Thinking about it, are there security implications for discard? > Should we make it default false? Hi Michael, I'm not completely sure but if the guest can write on a specific sector, discard or write_zeroes operations should not have a security implication. Do I miss something? Thanks, Stefano
On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote: > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > > In order to avoid migration issues, we enable DISCARD and > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > hw/block/virtio-blk.c | 2 ++ > > > hw/core/machine.c | 1 + > > > include/hw/virtio/virtio-blk.h | 1 + > > > 3 files changed, 4 insertions(+) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index 8a6754d9a2..542ec52536 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > IOThread *), > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > + true), > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > > > > > Thinking about it, are there security implications for discard? > > Should we make it default false? > > Hi Michael, > > I'm not completely sure but if the guest can write on a specific sector, > discard or write_zeroes operations should not have a security implication. > > Do I miss something? Recently page cache attacks have been discussed in the Linux community: https://arxiv.org/pdf/1901.01161.pdf I guess the scenario Michael is thinking about involves either -drive cache.direct=off (including cache=writeback or cache=writethrough) or maybe a timing side-channel in the storage appliance. The discard operation may allow a guest to evict the cache, an important primitive for page cache attacks. Most of the time disk images are not shared between guests at all. Therefore the discard operation cannot be used to learn information about other guests. Let's focus on shared disk images: shared disk images are either read-only (then discard isn't allowed anyway) or they are shared writable (but this already implies a trust relationship between the guests). My opinion is that discard is safe because virtualization use cases are quite different from the attacks possible with shared library files between userspace processes. I'm curious if anyone has figured out a realistic scenario where it does matter though... Stefan
On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote: > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote: > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > > > In order to avoid migration issues, we enable DISCARD and > > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > --- > > > > hw/block/virtio-blk.c | 2 ++ > > > > hw/core/machine.c | 1 + > > > > include/hw/virtio/virtio-blk.h | 1 + > > > > 3 files changed, 4 insertions(+) > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > index 8a6754d9a2..542ec52536 100644 > > > > --- a/hw/block/virtio-blk.c > > > > +++ b/hw/block/virtio-blk.c > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > > IOThread *), > > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > > + true), > > > > DEFINE_PROP_END_OF_LIST(), > > > > }; > > > > > > > > > > Thinking about it, are there security implications for discard? > > > Should we make it default false? > > > > Hi Michael, > > > > I'm not completely sure but if the guest can write on a specific sector, > > discard or write_zeroes operations should not have a security implication. > > > > Do I miss something? > > Recently page cache attacks have been discussed in the Linux community: > https://arxiv.org/pdf/1901.01161.pdf > > I guess the scenario Michael is thinking about involves either -drive > cache.direct=off (including cache=writeback or cache=writethrough) or > maybe a timing side-channel in the storage appliance. > > The discard operation may allow a guest to evict the cache, an important > primitive for page cache attacks. > > Most of the time disk images are not shared between guests at all. > Therefore the discard operation cannot be used to learn information > about other guests. > > Let's focus on shared disk images: shared disk images are either > read-only (then discard isn't allowed anyway) or they are shared > writable (but this already implies a trust relationship between the > guests). > > My opinion is that discard is safe because virtualization use cases are > quite different from the attacks possible with shared library files > between userspace processes. I'm curious if anyone has figured out a > realistic scenario where it does matter though... Many thanks for the explanation! I'll wait to send the v3 in order to understand if Michael agrees to leave discard feature enabled to default. Thanks, Stefano
On Mon, Feb 04, 2019 at 11:16:14AM +0100, Stefano Garzarella wrote: > On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote: > > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote: > > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > > > > In order to avoid migration issues, we enable DISCARD and > > > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > --- > > > > > hw/block/virtio-blk.c | 2 ++ > > > > > hw/core/machine.c | 1 + > > > > > include/hw/virtio/virtio-blk.h | 1 + > > > > > 3 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > index 8a6754d9a2..542ec52536 100644 > > > > > --- a/hw/block/virtio-blk.c > > > > > +++ b/hw/block/virtio-blk.c > > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > > > IOThread *), > > > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > > > + true), > > > > > DEFINE_PROP_END_OF_LIST(), > > > > > }; > > > > > > > > > > > > > Thinking about it, are there security implications for discard? > > > > Should we make it default false? > > > > > > Hi Michael, > > > > > > I'm not completely sure but if the guest can write on a specific sector, > > > discard or write_zeroes operations should not have a security implication. > > > > > > Do I miss something? > > > > Recently page cache attacks have been discussed in the Linux community: > > https://arxiv.org/pdf/1901.01161.pdf > > > > I guess the scenario Michael is thinking about involves either -drive > > cache.direct=off (including cache=writeback or cache=writethrough) or > > maybe a timing side-channel in the storage appliance. > > > > The discard operation may allow a guest to evict the cache, an important > > primitive for page cache attacks. > > > > Most of the time disk images are not shared between guests at all. > > Therefore the discard operation cannot be used to learn information > > about other guests. > > > > Let's focus on shared disk images: shared disk images are either > > read-only (then discard isn't allowed anyway) or they are shared > > writable (but this already implies a trust relationship between the > > guests). > > > > My opinion is that discard is safe because virtualization use cases are > > quite different from the attacks possible with shared library files > > between userspace processes. I'm curious if anyone has figured out a > > realistic scenario where it does matter though... > > Many thanks for the explanation! > > I'll wait to send the v3 in order to understand if Michael agrees to > leave discard feature enabled to default. > > Thanks, > Stefano OK. Maybe mention the above in the commit log.
On Mon, Feb 04, 2019 at 08:37:28AM -0500, Michael S. Tsirkin wrote: > On Mon, Feb 04, 2019 at 11:16:14AM +0100, Stefano Garzarella wrote: > > On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote: > > > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote: > > > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote: > > > > > > In order to avoid migration issues, we enable DISCARD and > > > > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > --- > > > > > > hw/block/virtio-blk.c | 2 ++ > > > > > > hw/core/machine.c | 1 + > > > > > > include/hw/virtio/virtio-blk.h | 1 + > > > > > > 3 files changed, 4 insertions(+) > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > index 8a6754d9a2..542ec52536 100644 > > > > > > --- a/hw/block/virtio-blk.c > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > > > > IOThread *), > > > > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > > > > + true), > > > > > > DEFINE_PROP_END_OF_LIST(), > > > > > > }; > > > > > > > > > > > > > > > > Thinking about it, are there security implications for discard? > > > > > Should we make it default false? > > > > > > > > Hi Michael, > > > > > > > > I'm not completely sure but if the guest can write on a specific sector, > > > > discard or write_zeroes operations should not have a security implication. > > > > > > > > Do I miss something? > > > > > > Recently page cache attacks have been discussed in the Linux community: > > > https://arxiv.org/pdf/1901.01161.pdf > > > > > > I guess the scenario Michael is thinking about involves either -drive > > > cache.direct=off (including cache=writeback or cache=writethrough) or > > > maybe a timing side-channel in the storage appliance. > > > > > > The discard operation may allow a guest to evict the cache, an important > > > primitive for page cache attacks. > > > > > > Most of the time disk images are not shared between guests at all. > > > Therefore the discard operation cannot be used to learn information > > > about other guests. > > > > > > Let's focus on shared disk images: shared disk images are either > > > read-only (then discard isn't allowed anyway) or they are shared > > > writable (but this already implies a trust relationship between the > > > guests). > > > > > > My opinion is that discard is safe because virtualization use cases are > > > quite different from the attacks possible with shared library files > > > between userspace processes. I'm curious if anyone has figured out a > > > realistic scenario where it does matter though... > > > > Many thanks for the explanation! > > > > I'll wait to send the v3 in order to understand if Michael agrees to > > leave discard feature enabled to default. > > > > Thanks, > > Stefano > > OK. Maybe mention the above in the commit log. > Ok, I'll do it! Thanks, Stefano
On Thu, Jan 31, 2019 at 06:37:13PM +0100, Stefano Garzarella wrote: > On Thu, Jan 31, 2019 at 11:43:07AM -0500, Michael S. Tsirkin wrote: > > On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote: > > > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote: > > > > * Stefano Garzarella (sgarzare@redhat.com) wrote: > > > > > In order to avoid migration issues, we enable DISCARD and > > > > > WRITE ZEROES features only for machine type >= 4.0 > > > > > > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > --- > > > > > hw/block/virtio-blk.c | 2 ++ > > > > > hw/core/machine.c | 1 + > > > > > include/hw/virtio/virtio-blk.h | 1 + > > > > > 3 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > index 8a6754d9a2..542ec52536 100644 > > > > > --- a/hw/block/virtio-blk.c > > > > > +++ b/hw/block/virtio-blk.c > > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { > > > > > DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > > > IOThread *), > > > > > + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, > > > > > + true), > > > > > > > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and > > > > a bool discard_wzeroes? > > > > I think DEFINE_PROP_BIT is mostly used for a flag word where each > > > > property is one more bit in the field. > > > > > > > > Dave > > > > > > > > > > Hi Dave, > > > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but > > > looking in the virtio-blk.c, I found that also other boolean like > > > "config-wce", "scsi", and "request-merging" was defined with > > > DEFINE_PROP_BIT, so I followed this trand. > > > > > > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it! > > > > > > Thanks, > > > Stefano > > > > I wonder why doesn't virtio-blk set bits directly in host_features? > > For example this is how virtio-net does it. > > This would remove the need for virtio_add_feature calls. > > Maybe this should be the best approach! > > What do you think if I send a trivial patch to add host_features > variable like for virtio-net and change the "config_wce" and "scsi" definition? > Then I will change also this patch to set directly the bits. > > Thanks, > Stefano Sounds good to me.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 8a6754d9a2..542ec52536 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, IOThread *), + DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0, + true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/core/machine.c b/hw/core/machine.c index 2629515363..ce98857af0 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = { { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" }, { "tpm-crb", "ppi", "false" }, { "tpm-tis", "ppi", "false" }, + { "virtio-blk-device", "discard-wzeroes", "false" }, }; const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1); diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 5117431d96..c336afb4cd 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -40,6 +40,7 @@ struct VirtIOBlkConf uint32_t request_merging; uint16_t num_queues; uint16_t queue_size; + uint32_t discard_wzeroes; }; struct VirtIOBlockDataPlane;
In order to avoid migration issues, we enable DISCARD and WRITE ZEROES features only for machine type >= 4.0 Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- hw/block/virtio-blk.c | 2 ++ hw/core/machine.c | 1 + include/hw/virtio/virtio-blk.h | 1 + 3 files changed, 4 insertions(+)