From: Jason Wang <jasowang@redhat.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: "Michael S. Tsirkin" <mst@redhat.com>, virtualization <virtualization@lists.linux-foundation.org>, linux-kernel <linux-kernel@vger.kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Peter Zijlstra <peterz@infradead.org>, "Paul E. McKenney" <paulmck@kernel.org>, Marc Zyngier <maz@kernel.org>, Halil Pasic <pasic@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, eperezma <eperezma@redhat.com>, Cindy Lu <lulu@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, Vineeth Vijayan <vneethv@linux.ibm.com>, Peter Oberparleiter <oberpar@linux.ibm.com>, linux-s390@vger.kernel.org, conghui.chen@intel.com, Viresh Kumar <viresh.kumar@linaro.org>, netdev <netdev@vger.kernel.org>, pankaj.gupta.linux@gmail.com, sudeep.holla@arm.com, Bjorn Andersson <bjorn.andersson@linaro.org>, Mathieu Poirier <mathieu.poirier@linaro.org> Subject: Re: [PATCH V6 8/9] virtio: harden vring IRQ Date: Fri, 17 Jun 2022 11:14:50 +0800 [thread overview] Message-ID: <CACGkMEsdhCx8mmGn+axjM-+Psep4jVN2zzbBQhjW3y6gvHXxXQ@mail.gmail.com> (raw) In-Reply-To: <YqojyHuocSoZ0v/Y@e120937-lin> On Thu, Jun 16, 2022 at 2:24 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > > On Wed, Jun 15, 2022 at 09:41:18AM +0800, Jason Wang wrote: > > On Wed, Jun 15, 2022 at 12:46 AM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > Hi Jason, > > > > > > > On Tue, Jun 14, 2022 at 03:40:21PM +0800, Jason Wang wrote: > > > > On Mon, Jun 13, 2022 at 5:28 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > [snip] > > > > > > > > > > arm_scmi > > > > > > > > It looks to me the singleton device could be used by SCMI immediately after > > > > > > > > /* Ensure initialized scmi_vdev is visible */ > > > > smp_store_mb(scmi_vdev, vdev); > > > > > > > > So we probably need to do virtio_device_ready() before that. It has an > > > > optional rx queue but the filling is done after the above assignment, > > > > so it's safe. And the callback looks safe is a callback is triggered > > > > after virtio_device_ready() buy before the above assignment. > > > > > > > > > > I wanted to give it a go at this series testing it on the context of > > > SCMI but it does not apply > > > > > > - not on a v5.18: > > > > > > 17:33 $ git rebase -i v5.18 > > > 17:33 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx > > > Applying: virtio: use virtio_device_ready() in virtio_device_restore() > > > Applying: virtio: use virtio_reset_device() when possible > > > Applying: virtio: introduce config op to synchronize vring callbacks > > > Applying: virtio-pci: implement synchronize_cbs() > > > Applying: virtio-mmio: implement synchronize_cbs() > > > error: patch failed: drivers/virtio/virtio_mmio.c:345 > > > error: drivers/virtio/virtio_mmio.c: patch does not apply > > > Patch failed at 0005 virtio-mmio: implement synchronize_cbs() > > > > > > - neither on a v5.19-rc2: > > > > > > 17:33 $ git rebase -i v5.19-rc2 > > > 17:35 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx > > > Applying: virtio: use virtio_device_ready() in virtio_device_restore() > > > error: patch failed: drivers/virtio/virtio.c:526 > > > error: drivers/virtio/virtio.c: patch does not apply > > > Patch failed at 0001 virtio: use virtio_device_ready() in > > > virtio_device_restore() > > > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > > When you have resolved this problem, run "git am --continue". > > > > > > ... what I should take as base ? > > > > It should have already been included in rc2, so there's no need to > > apply patch manually. > > > > I tested this series as included in v5.19-rc2 (WITHOUT adding a virtio_device_ready > in SCMI virtio as you mentioned above ... if I got it right) and I have NOT seen any > issue around SCMI virtio using my usual test setup (using both SCMI vqueues). > > No anomalies even when using SCMI virtio in atomic/polling mode. > > Adding a virtio_device_ready() at the end of the SCMI virtio probe() > works fine either, it does not make any difference in my setup. > (both using QEMU and kvmtool with this latter NOT supporting > virtio_V1...not sure if it makes a difference but I thought was worth > mentioning) Thanks a lot for the testing. We want to prevent malicious hypervisors from attacking us. So more questions: Assuming we do: virtio_device_ready(); /* Ensure initialized scmi_vdev is visible */ smp_store_mb(scmi_vdev, vdev); This means we allow the callbacks (scmi_vio_complete) to be called before smp_store_mb(). We need to make sure the callbacks are robust. And this looks fine since we have the check of scmi_vio_channel_acquire() and if the notification is called before smp_store_mb(), the acquire will fail. If we put virtio_device_ready() after smp_store_mb() like: /* Ensure initialized scmi_vdev is visible */ smp_store_mb(scmi_vdev, vdev); virtio_device_ready(); If I understand correctly, there will be a race since the SCMI may try to use the device before virtio_device_ready(), this violates the virtio spec somehow. Thanks > > Thanks, > Cristian >
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: "Michael S. Tsirkin" <mst@redhat.com>, Peter Zijlstra <peterz@infradead.org>, Viresh Kumar <viresh.kumar@linaro.org>, linux-kernel <linux-kernel@vger.kernel.org>, Vineeth Vijayan <vneethv@linux.ibm.com>, Cindy Lu <lulu@redhat.com>, Marc Zyngier <maz@kernel.org>, Halil Pasic <pasic@linux.ibm.com>, eperezma <eperezma@redhat.com>, "Paul E. McKenney" <paulmck@kernel.org>, linux-s390@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>, virtualization <virtualization@lists.linux-foundation.org>, conghui.chen@intel.com, pankaj.gupta.linux@gmail.com, Mathieu Poirier <mathieu.poirier@linaro.org>, netdev <netdev@vger.kernel.org>, Cornelia Huck <cohuck@redhat.com>, Peter Oberparleiter <oberpar@linux.ibm.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, sudeep.holla@arm.com Subject: Re: [PATCH V6 8/9] virtio: harden vring IRQ Date: Fri, 17 Jun 2022 11:14:50 +0800 [thread overview] Message-ID: <CACGkMEsdhCx8mmGn+axjM-+Psep4jVN2zzbBQhjW3y6gvHXxXQ@mail.gmail.com> (raw) In-Reply-To: <YqojyHuocSoZ0v/Y@e120937-lin> On Thu, Jun 16, 2022 at 2:24 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > > On Wed, Jun 15, 2022 at 09:41:18AM +0800, Jason Wang wrote: > > On Wed, Jun 15, 2022 at 12:46 AM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > Hi Jason, > > > > > > > On Tue, Jun 14, 2022 at 03:40:21PM +0800, Jason Wang wrote: > > > > On Mon, Jun 13, 2022 at 5:28 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > [snip] > > > > > > > > > > arm_scmi > > > > > > > > It looks to me the singleton device could be used by SCMI immediately after > > > > > > > > /* Ensure initialized scmi_vdev is visible */ > > > > smp_store_mb(scmi_vdev, vdev); > > > > > > > > So we probably need to do virtio_device_ready() before that. It has an > > > > optional rx queue but the filling is done after the above assignment, > > > > so it's safe. And the callback looks safe is a callback is triggered > > > > after virtio_device_ready() buy before the above assignment. > > > > > > > > > > I wanted to give it a go at this series testing it on the context of > > > SCMI but it does not apply > > > > > > - not on a v5.18: > > > > > > 17:33 $ git rebase -i v5.18 > > > 17:33 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx > > > Applying: virtio: use virtio_device_ready() in virtio_device_restore() > > > Applying: virtio: use virtio_reset_device() when possible > > > Applying: virtio: introduce config op to synchronize vring callbacks > > > Applying: virtio-pci: implement synchronize_cbs() > > > Applying: virtio-mmio: implement synchronize_cbs() > > > error: patch failed: drivers/virtio/virtio_mmio.c:345 > > > error: drivers/virtio/virtio_mmio.c: patch does not apply > > > Patch failed at 0005 virtio-mmio: implement synchronize_cbs() > > > > > > - neither on a v5.19-rc2: > > > > > > 17:33 $ git rebase -i v5.19-rc2 > > > 17:35 $ git am ./v6_20220527_jasowang_rework_on_the_irq_hardening_of_virtio.mbx > > > Applying: virtio: use virtio_device_ready() in virtio_device_restore() > > > error: patch failed: drivers/virtio/virtio.c:526 > > > error: drivers/virtio/virtio.c: patch does not apply > > > Patch failed at 0001 virtio: use virtio_device_ready() in > > > virtio_device_restore() > > > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > > When you have resolved this problem, run "git am --continue". > > > > > > ... what I should take as base ? > > > > It should have already been included in rc2, so there's no need to > > apply patch manually. > > > > I tested this series as included in v5.19-rc2 (WITHOUT adding a virtio_device_ready > in SCMI virtio as you mentioned above ... if I got it right) and I have NOT seen any > issue around SCMI virtio using my usual test setup (using both SCMI vqueues). > > No anomalies even when using SCMI virtio in atomic/polling mode. > > Adding a virtio_device_ready() at the end of the SCMI virtio probe() > works fine either, it does not make any difference in my setup. > (both using QEMU and kvmtool with this latter NOT supporting > virtio_V1...not sure if it makes a difference but I thought was worth > mentioning) Thanks a lot for the testing. We want to prevent malicious hypervisors from attacking us. So more questions: Assuming we do: virtio_device_ready(); /* Ensure initialized scmi_vdev is visible */ smp_store_mb(scmi_vdev, vdev); This means we allow the callbacks (scmi_vio_complete) to be called before smp_store_mb(). We need to make sure the callbacks are robust. And this looks fine since we have the check of scmi_vio_channel_acquire() and if the notification is called before smp_store_mb(), the acquire will fail. If we put virtio_device_ready() after smp_store_mb() like: /* Ensure initialized scmi_vdev is visible */ smp_store_mb(scmi_vdev, vdev); virtio_device_ready(); If I understand correctly, there will be a race since the SCMI may try to use the device before virtio_device_ready(), this violates the virtio spec somehow. Thanks > > Thanks, > Cristian > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-06-17 3:15 UTC|newest] Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-27 6:01 [PATCH V6 0/9] rework on the IRQ hardening of virtio Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 6:01 ` [PATCH V6 1/9] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:29 ` Xuan Zhuo 2022-05-27 7:29 ` Xuan Zhuo 2022-05-27 6:01 ` [PATCH V6 2/9] virtio: use virtio_reset_device() when possible Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:30 ` Xuan Zhuo 2022-05-27 7:30 ` Xuan Zhuo 2022-05-27 8:52 ` Eugenio Perez Martin 2022-05-27 10:34 ` Stefano Garzarella 2022-05-27 10:34 ` Stefano Garzarella 2022-05-27 6:01 ` [PATCH V6 3/9] virtio: introduce config op to synchronize vring callbacks Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:30 ` Xuan Zhuo 2022-05-27 7:30 ` Xuan Zhuo 2022-05-27 10:36 ` Stefano Garzarella 2022-05-27 10:36 ` Stefano Garzarella 2022-05-27 6:01 ` [PATCH V6 4/9] virtio-pci: implement synchronize_cbs() Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:31 ` Xuan Zhuo 2022-05-27 7:31 ` Xuan Zhuo 2022-05-27 6:01 ` [PATCH V6 5/9] virtio-mmio: " Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:32 ` Xuan Zhuo 2022-05-27 7:32 ` Xuan Zhuo 2022-05-27 6:01 ` [PATCH V6 6/9] virtio-ccw: " Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-30 15:12 ` Cornelia Huck 2022-05-30 15:12 ` Cornelia Huck 2022-05-27 6:01 ` [PATCH V6 7/9] virtio: allow to unbreak virtqueue Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:33 ` Xuan Zhuo 2022-05-27 7:33 ` Xuan Zhuo 2022-05-30 15:15 ` Cornelia Huck 2022-05-30 15:15 ` Cornelia Huck 2022-05-27 6:01 ` [PATCH V6 8/9] virtio: harden vring IRQ Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:49 ` Xuan Zhuo 2022-05-27 7:49 ` Xuan Zhuo 2022-05-30 15:18 ` Cornelia Huck 2022-05-30 15:18 ` Cornelia Huck 2022-06-11 5:12 ` Michael S. Tsirkin 2022-06-11 5:12 ` Michael S. Tsirkin 2022-06-13 5:26 ` Jason Wang 2022-06-13 5:26 ` Jason Wang 2022-06-13 7:23 ` Michael S. Tsirkin 2022-06-13 7:23 ` Michael S. Tsirkin 2022-06-13 8:07 ` Jason Wang 2022-06-13 8:07 ` Jason Wang 2022-06-13 8:19 ` Michael S. Tsirkin 2022-06-13 8:19 ` Michael S. Tsirkin 2022-06-13 8:51 ` Jason Wang 2022-06-13 8:51 ` Jason Wang 2022-06-13 8:59 ` Michael S. Tsirkin 2022-06-13 8:59 ` Michael S. Tsirkin 2022-06-13 9:08 ` Jason Wang 2022-06-13 9:08 ` Jason Wang 2022-06-13 9:14 ` Jason Wang 2022-06-13 9:14 ` Jason Wang 2022-06-13 9:27 ` Michael S. Tsirkin 2022-06-13 9:27 ` Michael S. Tsirkin 2022-06-14 7:40 ` Jason Wang 2022-06-14 7:40 ` Jason Wang 2022-06-14 13:50 ` Michael S. Tsirkin 2022-06-14 13:50 ` Michael S. Tsirkin 2022-06-15 1:32 ` Jason Wang 2022-06-15 1:32 ` Jason Wang 2022-06-14 15:49 ` Michael S. Tsirkin 2022-06-14 15:49 ` Michael S. Tsirkin 2022-06-15 1:38 ` Jason Wang 2022-06-15 1:38 ` Jason Wang 2022-06-16 17:11 ` Michael S. Tsirkin 2022-06-16 17:11 ` Michael S. Tsirkin 2022-06-17 1:24 ` Jason Wang 2022-06-17 1:24 ` Jason Wang 2022-06-17 5:36 ` Michael S. Tsirkin 2022-06-17 5:36 ` Michael S. Tsirkin 2022-06-17 7:26 ` Jason Wang 2022-06-17 7:26 ` Jason Wang 2022-06-17 14:33 ` Peter Zijlstra 2022-06-17 14:33 ` Peter Zijlstra 2022-06-14 16:46 ` Cristian Marussi 2022-06-15 1:41 ` Jason Wang 2022-06-15 1:41 ` Jason Wang 2022-06-15 18:24 ` Cristian Marussi 2022-06-17 3:14 ` Jason Wang [this message] 2022-06-17 3:14 ` Jason Wang 2022-06-13 9:26 ` Michael S. Tsirkin 2022-06-13 9:26 ` Michael S. Tsirkin 2022-06-14 7:19 ` Jason Wang 2022-06-14 7:19 ` Jason Wang 2022-07-05 11:06 ` chenxiang (M) 2022-07-05 12:56 ` Jason Wang 2022-07-05 12:56 ` Jason Wang 2022-05-27 6:01 ` [PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value Jason Wang 2022-05-27 6:01 ` Jason Wang 2022-05-27 7:49 ` Xuan Zhuo 2022-05-27 7:49 ` Xuan Zhuo 2022-05-27 10:35 ` Stefano Garzarella 2022-05-27 10:35 ` Stefano Garzarella 2022-05-27 10:50 ` Michael S. Tsirkin 2022-05-27 10:50 ` Michael S. Tsirkin 2022-05-30 3:48 ` Jason Wang 2022-05-30 3:48 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACGkMEsdhCx8mmGn+axjM-+Psep4jVN2zzbBQhjW3y6gvHXxXQ@mail.gmail.com \ --to=jasowang@redhat.com \ --cc=bjorn.andersson@linaro.org \ --cc=cohuck@redhat.com \ --cc=conghui.chen@intel.com \ --cc=cristian.marussi@arm.com \ --cc=eperezma@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=lulu@redhat.com \ --cc=mathieu.poirier@linaro.org \ --cc=maz@kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=oberpar@linux.ibm.com \ --cc=pankaj.gupta.linux@gmail.com \ --cc=pasic@linux.ibm.com \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=sgarzare@redhat.com \ --cc=sudeep.holla@arm.com \ --cc=tglx@linutronix.de \ --cc=viresh.kumar@linaro.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=vneethv@linux.ibm.com \ --cc=xuanzhuo@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.