* [PATCH] vp_vdpa: harden the logic of set status @ 2022-11-11 14:55 Longpeng(Mike) 2022-11-11 15:14 ` Stefano Garzarella 0 siblings, 1 reply; 6+ messages in thread From: Longpeng(Mike) @ 2022-11-11 14:55 UTC (permalink / raw) To: stefanha, mst, jasowang, sgarzare Cc: virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong, Longpeng From: Longpeng <longpeng2@huawei.com> 1. We should not set status to 0 when invoking vp_vdpa_set_status(). 2. The driver MUST wait for a read of device_status to return 0 before reinitializing the device. Signed-off-by: Longpeng <longpeng2@huawei.com> --- drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c index d448db0c4de3..d35fac5cde11 100644 --- a/drivers/vdpa/virtio_pci/vp_vdpa.c +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c @@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status) { struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); - u8 s = vp_vdpa_get_status(vdpa); + u8 s; + + /* We should never be setting status to 0. */ + BUG_ON(status == 0); + s = vp_vdpa_get_status(vdpa); if (status & VIRTIO_CONFIG_S_DRIVER_OK && !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { vp_vdpa_request_irq(vp_vdpa); @@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa) u8 s = vp_vdpa_get_status(vdpa); vp_modern_set_status(mdev, 0); + /* After writing 0 to device_status, the driver MUST wait for a read of + * device_status to return 0 before reinitializing the device. + */ + while (vp_modern_get_status(mdev)) + msleep(1); if (s & VIRTIO_CONFIG_S_DRIVER_OK) vp_vdpa_free_irq(vp_vdpa); -- 2.23.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vp_vdpa: harden the logic of set status 2022-11-11 14:55 [PATCH] vp_vdpa: harden the logic of set status Longpeng(Mike) @ 2022-11-11 15:14 ` Stefano Garzarella 2022-11-11 15:49 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 6+ messages in thread From: Stefano Garzarella @ 2022-11-11 15:14 UTC (permalink / raw) To: Longpeng(Mike) Cc: stefanha, mst, jasowang, virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote: >From: Longpeng <longpeng2@huawei.com> > >1. We should not set status to 0 when invoking vp_vdpa_set_status(). > >2. The driver MUST wait for a read of device_status to return 0 before > reinitializing the device. > >Signed-off-by: Longpeng <longpeng2@huawei.com> >--- > drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > >diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c >index d448db0c4de3..d35fac5cde11 100644 >--- a/drivers/vdpa/virtio_pci/vp_vdpa.c >+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >@@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status) > { > struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); > struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); >- u8 s = vp_vdpa_get_status(vdpa); Is this change really needed? >+ u8 s; >+ >+ /* We should never be setting status to 0. */ >+ BUG_ON(status == 0); IMHO panicking the kernel seems excessive in this case, please use WARN_ON and maybe return earlier. > >+ s = vp_vdpa_get_status(vdpa); > if (status & VIRTIO_CONFIG_S_DRIVER_OK && > !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { > vp_vdpa_request_irq(vp_vdpa); >@@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa) > u8 s = vp_vdpa_get_status(vdpa); > > vp_modern_set_status(mdev, 0); >+ /* After writing 0 to device_status, the driver MUST wait for a read of >+ * device_status to return 0 before reinitializing the device. >+ */ >+ while (vp_modern_get_status(mdev)) >+ msleep(1); Should we set a limit after which we give up? A malfunctioning device could keep us here forever. Thanks, Stefano > > if (s & VIRTIO_CONFIG_S_DRIVER_OK) > vp_vdpa_free_irq(vp_vdpa); >-- >2.23.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vp_vdpa: harden the logic of set status 2022-11-11 15:14 ` Stefano Garzarella @ 2022-11-11 15:49 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2022-11-11 16:35 ` Stefano Garzarella 0 siblings, 1 reply; 6+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2022-11-11 15:49 UTC (permalink / raw) To: Stefano Garzarella Cc: stefanha, mst, jasowang, virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong 在 2022/11/11 23:14, Stefano Garzarella 写道: > On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote: >> From: Longpeng <longpeng2@huawei.com> >> >> 1. We should not set status to 0 when invoking vp_vdpa_set_status(). >> >> 2. The driver MUST wait for a read of device_status to return 0 before >> reinitializing the device. >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c >> b/drivers/vdpa/virtio_pci/vp_vdpa.c >> index d448db0c4de3..d35fac5cde11 100644 >> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c >> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >> @@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct vdpa_device >> *vdpa, u8 status) >> { >> struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); >> struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); >> - u8 s = vp_vdpa_get_status(vdpa); > > Is this change really needed? > No need to get the status if we try to set status to 0 (trigger BUG). >> + u8 s; >> + >> + /* We should never be setting status to 0. */ >> + BUG_ON(status == 0); > > IMHO panicking the kernel seems excessive in this case, please use > WARN_ON and maybe return earlier. > Um...I referenced the vp_reset/vp_set_status, >> >> + s = vp_vdpa_get_status(vdpa); >> if (status & VIRTIO_CONFIG_S_DRIVER_OK && >> !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { >> vp_vdpa_request_irq(vp_vdpa); >> @@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa) >> u8 s = vp_vdpa_get_status(vdpa); >> >> vp_modern_set_status(mdev, 0); >> + /* After writing 0 to device_status, the driver MUST wait for a >> read of >> + * device_status to return 0 before reinitializing the device. >> + */ >> + while (vp_modern_get_status(mdev)) >> + msleep(1); > > Should we set a limit after which we give up? A malfunctioning device > could keep us here forever. > Yes, but the malfunctioning device maybe can not work anymore, how to handle it? > Thanks, > Stefano > >> >> if (s & VIRTIO_CONFIG_S_DRIVER_OK) >> vp_vdpa_free_irq(vp_vdpa); >> -- >> 2.23.0 >> > > . ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vp_vdpa: harden the logic of set status 2022-11-11 15:49 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2022-11-11 16:35 ` Stefano Garzarella 2022-11-12 7:33 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 6+ messages in thread From: Stefano Garzarella @ 2022-11-11 16:35 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: stefanha, mst, jasowang, virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong On Fri, Nov 11, 2022 at 11:49:10PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > >在 2022/11/11 23:14, Stefano Garzarella 写道: >>On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote: >>>From: Longpeng <longpeng2@huawei.com> >>> >>>1. We should not set status to 0 when invoking vp_vdpa_set_status(). >>> >>>2. The driver MUST wait for a read of device_status to return 0 before >>> reinitializing the device. >>> >>>Signed-off-by: Longpeng <longpeng2@huawei.com> >>>--- >>>drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- >>>1 file changed, 10 insertions(+), 1 deletion(-) >>> >>>diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>index d448db0c4de3..d35fac5cde11 100644 >>>--- a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>@@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct >>>vdpa_device *vdpa, u8 status) >>>{ >>> struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); >>> struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); >>>- u8 s = vp_vdpa_get_status(vdpa); >> >>Is this change really needed? >> >No need to get the status if we try to set status to 0 (trigger BUG). > Okay, but that's the case that should never happen, so IMHO we can leave it as it is. >>>+ u8 s; >>>+ >>>+ /* We should never be setting status to 0. */ >>>+ BUG_ON(status == 0); >> >>IMHO panicking the kernel seems excessive in this case, please use >>WARN_ON and maybe return earlier. >> >Um...I referenced the vp_reset/vp_set_status, Ah I see, maybe it's an old code, because recently we always try to avoid BUG_ON(). > >>> >>>+ s = vp_vdpa_get_status(vdpa); >>> if (status & VIRTIO_CONFIG_S_DRIVER_OK && >>> !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { >>> vp_vdpa_request_irq(vp_vdpa); >>>@@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa) >>> u8 s = vp_vdpa_get_status(vdpa); >>> >>> vp_modern_set_status(mdev, 0); >>>+ /* After writing 0 to device_status, the driver MUST wait for >>>a read of >>>+ * device_status to return 0 before reinitializing the device. >>>+ */ >>>+ while (vp_modern_get_status(mdev)) >>>+ msleep(1); >> >>Should we set a limit after which we give up? A malfunctioning >>device could keep us here forever. >> >Yes, but the malfunctioning device maybe can not work anymore, how to >handle it? Maybe we should set the status to broken, but in this case we could just return an error if we couldn't reset it, how about that? Thanks, Stefano ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vp_vdpa: harden the logic of set status 2022-11-11 16:35 ` Stefano Garzarella @ 2022-11-12 7:33 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2022-11-14 4:21 ` Jason Wang 0 siblings, 1 reply; 6+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2022-11-12 7:33 UTC (permalink / raw) To: Stefano Garzarella Cc: stefanha, mst, jasowang, virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong 在 2022/11/12 0:35, Stefano Garzarella 写道: > On Fri, Nov 11, 2022 at 11:49:10PM +0800, Longpeng (Mike, Cloud > Infrastructure Service Product Dept.) wrote: >> >> >> 在 2022/11/11 23:14, Stefano Garzarella 写道: >>> On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote: >>>> From: Longpeng <longpeng2@huawei.com> >>>> >>>> 1. We should not set status to 0 when invoking vp_vdpa_set_status(). >>>> >>>> 2. The driver MUST wait for a read of device_status to return 0 before >>>> reinitializing the device. >>>> >>>> Signed-off-by: Longpeng <longpeng2@huawei.com> >>>> --- >>>> drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>> b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>> index d448db0c4de3..d35fac5cde11 100644 >>>> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>> @@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct >>>> vdpa_device *vdpa, u8 status) >>>> { >>>> struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); >>>> struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); >>>> - u8 s = vp_vdpa_get_status(vdpa); >>> >>> Is this change really needed? >>> >> No need to get the status if we try to set status to 0 (trigger BUG). >> > > Okay, but that's the case that should never happen, so IMHO we can leave > it as it is. > OK. >>>> + u8 s; >>>> + >>>> + /* We should never be setting status to 0. */ >>>> + BUG_ON(status == 0); >>> >>> IMHO panicking the kernel seems excessive in this case, please use >>> WARN_ON and maybe return earlier. >>> >> Um...I referenced the vp_reset/vp_set_status, > > Ah I see, maybe it's an old code, because recently we always try to > avoid BUG_ON(). > OK. The checkpatch.pl script also triggered a waring about it. I'll use WARN_ON in next version. >> >>>> >>>> + s = vp_vdpa_get_status(vdpa); >>>> if (status & VIRTIO_CONFIG_S_DRIVER_OK && >>>> !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { >>>> vp_vdpa_request_irq(vp_vdpa); >>>> @@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa) >>>> u8 s = vp_vdpa_get_status(vdpa); >>>> >>>> vp_modern_set_status(mdev, 0); >>>> + /* After writing 0 to device_status, the driver MUST wait for a >>>> read of >>>> + * device_status to return 0 before reinitializing the device. >>>> + */ >>>> + while (vp_modern_get_status(mdev)) >>>> + msleep(1); >>> >>> Should we set a limit after which we give up? A malfunctioning device >>> could keep us here forever. >>> >> Yes, but the malfunctioning device maybe can not work anymore, how to >> handle it? > > Maybe we should set the status to broken, but in this case we could just > return an error if we couldn't reset it, how about that? > It can work, but it seems to violate the specification. Maybe we can also wait for other guys' suggestions and then decide how to handle the exception. > Thanks, > Stefano > > . ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vp_vdpa: harden the logic of set status 2022-11-12 7:33 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2022-11-14 4:21 ` Jason Wang 0 siblings, 0 replies; 6+ messages in thread From: Jason Wang @ 2022-11-14 4:21 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.), Stefano Garzarella Cc: stefanha, mst, virtualization, arei.gonglei, yechuan, huangzhichao, linux-kernel, xiehong 在 2022/11/12 15:33, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 写道: > > > 在 2022/11/12 0:35, Stefano Garzarella 写道: >> On Fri, Nov 11, 2022 at 11:49:10PM +0800, Longpeng (Mike, Cloud >> Infrastructure Service Product Dept.) wrote: >>> >>> >>> 在 2022/11/11 23:14, Stefano Garzarella 写道: >>>> On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote: >>>>> From: Longpeng <longpeng2@huawei.com> >>>>> >>>>> 1. We should not set status to 0 when invoking vp_vdpa_set_status(). >>>>> >>>>> 2. The driver MUST wait for a read of device_status to return 0 >>>>> before >>>>> reinitializing the device. >>>>> >>>>> Signed-off-by: Longpeng <longpeng2@huawei.com> >>>>> --- >>>>> drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++- >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>>> b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>>> index d448db0c4de3..d35fac5cde11 100644 >>>>> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c >>>>> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >>>>> @@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct >>>>> vdpa_device *vdpa, u8 status) >>>>> { >>>>> struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); >>>>> struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); >>>>> - u8 s = vp_vdpa_get_status(vdpa); >>>> >>>> Is this change really needed? >>>> >>> No need to get the status if we try to set status to 0 (trigger BUG). >>> >> >> Okay, but that's the case that should never happen, so IMHO we can >> leave it as it is. >> > OK. > >>>>> + u8 s; >>>>> + >>>>> + /* We should never be setting status to 0. */ >>>>> + BUG_ON(status == 0); >>>> >>>> IMHO panicking the kernel seems excessive in this case, please use >>>> WARN_ON and maybe return earlier. >>>> >>> Um...I referenced the vp_reset/vp_set_status, >> >> Ah I see, maybe it's an old code, because recently we always try to >> avoid BUG_ON(). >> > OK. The checkpatch.pl script also triggered a waring about it. > I'll use WARN_ON in next version. > >>> >>>>> >>>>> + s = vp_vdpa_get_status(vdpa); >>>>> if (status & VIRTIO_CONFIG_S_DRIVER_OK && >>>>> !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { >>>>> vp_vdpa_request_irq(vp_vdpa); >>>>> @@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device >>>>> *vdpa) >>>>> u8 s = vp_vdpa_get_status(vdpa); >>>>> >>>>> vp_modern_set_status(mdev, 0); >>>>> + /* After writing 0 to device_status, the driver MUST wait for >>>>> a read of >>>>> + * device_status to return 0 before reinitializing the device. >>>>> + */ >>>>> + while (vp_modern_get_status(mdev)) >>>>> + msleep(1); >>>> >>>> Should we set a limit after which we give up? A malfunctioning >>>> device could keep us here forever. >>>> >>> Yes, but the malfunctioning device maybe can not work anymore, how >>> to handle it? >> >> Maybe we should set the status to broken, but in this case we could >> just return an error if we couldn't reset it, how about that? >> > It can work, but it seems to violate the specification. Maybe we can > also wait for other guys' suggestions and then decide how to handle > the exception. Need more thought but it's not an issue that is introduced in this patch, we can do optimization on top. Probably a warning plus FAILED. Then at least the device can DOS the driver which is good for hardening as well. Thanks > >> Thanks, >> Stefano >> >> . > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-14 4:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-11 14:55 [PATCH] vp_vdpa: harden the logic of set status Longpeng(Mike) 2022-11-11 15:14 ` Stefano Garzarella 2022-11-11 15:49 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2022-11-11 16:35 ` Stefano Garzarella 2022-11-12 7:33 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2022-11-14 4:21 ` Jason Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®