| Message ID | 20221128021005.232105-1-lizetao1@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix probe failed when modprobe modules | expand |
On Mon, Nov 28, 2022 at 10:10:01AM +0800, Li Zetao wrote: > This patchset fixes similar issue, the root cause of the > problem is that the virtqueues are not stopped on error > handling path. I've been thinking about this. Almost all drivers are affected. The reason really is that it used to be the right thing to do: On legacy pci del_vqs writes 0 into vq index and this resets the device as a side effect (we actually do this multiple times, what e.g. writes of MSI vector after the 1st reset do I have no idea). mmio ccw and modern pci don't. Given this has been with us for a while I am inlined to look for a global solution rather than tweaking each driver. Given many drivers are supposed to work on legacy too, we know del_vqs includes a reset for many of them. So I think I see a better way to do this: Add virtio_reset_device_and_del_vqs() and convert all drivers to that. When doing this, we also need to/can fix a related problem (and related to the hardening that Jason Wang was looking into): virtio_reset_device is inherently racy: vq interrupts could be in flight when we do reset. We need to prevent handlers from firing in the window between reset and freeing the irq, so we should first free irqs and only then start changing the state by e.g. device reset. Quite a lot of core work here. Jason are you still looking into hardening? > Li Zetao (4): > 9p: Fix probe failed when modprobe 9pnet_virtio > virtio-mem: Fix probe failed when modprobe virtio_mem > virtio-input: Fix probe failed when modprobe virtio_input > virtio-blk: Fix probe failed when modprobe virtio_blk > > drivers/block/virtio_blk.c | 1 + > drivers/virtio/virtio_input.c | 1 + > drivers/virtio/virtio_mem.c | 1 + > net/9p/trans_virtio.c | 1 + > 4 files changed, 4 insertions(+) > > -- > 2.25.1
On Mon, Nov 28, 2022 at 6:14 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Nov 28, 2022 at 10:10:01AM +0800, Li Zetao wrote: > > This patchset fixes similar issue, the root cause of the > > problem is that the virtqueues are not stopped on error > > handling path. > > I've been thinking about this. > Almost all drivers are affected. > > The reason really is that it used to be the right thing to do: > On legacy pci del_vqs writes 0 > into vq index into vq address actually? > and this resets the device as a side effect I think there's no guarantee for a device to do this. > (we actually do this multiple times, what e.g. writes of MSI vector > after the 1st reset do I have no idea). > > mmio ccw and modern pci don't. > > Given this has been with us for a while I am inlined to look for > a global solution rather than tweaking each driver. But do we still need patches for -stable at least? > > Given many drivers are supposed to work on legacy too, we know del_vqs > includes a reset for many of them. So I think I see a better way to do > this: > > Add virtio_reset_device_and_del_vqs() What's the difference with the current del_vqs method? Is this something like: virtio_reset_device(); config->del_vqs(); > > and convert all drivers to that. > > When doing this, we also need to/can fix a related problem (and related > to the hardening that Jason Wang was looking into): > virtio_reset_device is inherently racy: vq interrupts could > be in flight when we do reset. We need to prevent handlers from firing in > the window between reset and freeing the irq, so we should first > free irqs and only then start changing the state by e.g. > device reset. Yes. > > > Quite a lot of core work here. Jason are you still looking into > hardening? Yes, last time we've discussed a solution that depends on the first kick to enable the interrupt handler. But after some thought, it seems risky since there's no guarantee that the device work in this way. One example is the current vhost_net, it doesn't wait for the kick to process the rx packets. Any more thought on this? Thanks > > > > > Li Zetao (4): > > 9p: Fix probe failed when modprobe 9pnet_virtio > > virtio-mem: Fix probe failed when modprobe virtio_mem > > virtio-input: Fix probe failed when modprobe virtio_input > > virtio-blk: Fix probe failed when modprobe virtio_blk > > > > drivers/block/virtio_blk.c | 1 + > > drivers/virtio/virtio_input.c | 1 + > > drivers/virtio/virtio_mem.c | 1 + > > net/9p/trans_virtio.c | 1 + > > 4 files changed, 4 insertions(+) > > > > -- > > 2.25.1 >
This patchset fixes similar issue, the root cause of the problem is that the virtqueues are not stopped on error handling path. Changes since v1: - Modify the description error of the test case and fixes tag information. - Add patch to fix virtio_gpu module. v1 at: https://lore.kernel.org/all/20221128021005.232105-1-lizetao1@huawei.com/ Li Zetao (5): 9p: Fix probe failed when modprobe 9pnet_virtio virtio-mem: Fix probe failed when modprobe virtio_mem virtio-input: Fix probe failed when modprobe virtio_input virtio-blk: Fix probe failed when modprobe virtio_blk drm/virtio: Fix probe failed when modprobe virtio_gpu drivers/block/virtio_blk.c | 1 + drivers/gpu/drm/virtio/virtgpu_kms.c | 1 + drivers/virtio/virtio_input.c | 1 + drivers/virtio/virtio_mem.c | 1 + net/9p/trans_virtio.c | 1 + 5 files changed, 5 insertions(+)
On 11/29/22 9:06 AM, Li Zetao wrote: > This patchset fixes similar issue, the root cause of the > problem is that the virtqueues are not stopped on error > handling path. Not related to just this patchset, but guys, Huawei really *REALLY* need to get the email situation sorted. I'm digging whole/half patchsets out of spam every morning. This has been brought up in the past. And no, the cloud variant of the email also doesn't work properly. Talk to your IT department, get this sorted once and for all. You risk your patches being dumped on the floor because people don't see them, or only see small parts of a patchset. And it's really annoying to have to deal with as a recipient.
On Tue, Nov 29, 2022 at 11:37:09AM +0800, Jason Wang wrote: > > > > > > Quite a lot of core work here. Jason are you still looking into > > hardening? > > Yes, last time we've discussed a solution that depends on the first > kick to enable the interrupt handler. But after some thought, it seems > risky since there's no guarantee that the device work in this way. > > One example is the current vhost_net, it doesn't wait for the kick to > process the rx packets. Any more thought on this? > > Thanks Specifically virtio net is careful to call virtio_device_ready under rtnl lock so buffers are only added after DRIVER_OK. However we do not need to tie this to kick, this is what I wrote: > BTW Jason, I had the idea to disable callbacks until driver uses the > virtio core for the first time (e.g. by calling virtqueue_add* family of > APIs). Less aggressive than your ideas but I feel it will add security > to the init path at least. So not necessarily kick, we can make adding buffers allow the interrupt.
On Mon, Dec 19, 2022 at 6:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 29, 2022 at 11:37:09AM +0800, Jason Wang wrote: > > > > > > > > > Quite a lot of core work here. Jason are you still looking into > > > hardening? > > > > Yes, last time we've discussed a solution that depends on the first > > kick to enable the interrupt handler. But after some thought, it seems > > risky since there's no guarantee that the device work in this way. > > > > One example is the current vhost_net, it doesn't wait for the kick to > > process the rx packets. Any more thought on this? > > > > Thanks > > Specifically virtio net is careful to call virtio_device_ready > under rtnl lock so buffers are only added after DRIVER_OK. Right but it only got fixed this year after some code audit. > > However we do not need to tie this to kick, this is what I wrote: > > > BTW Jason, I had the idea to disable callbacks until driver uses the > > virtio core for the first time (e.g. by calling virtqueue_add* family of > > APIs). Less aggressive than your ideas but I feel it will add security > > to the init path at least. > > So not necessarily kick, we can make adding buffers allow the > interrupt. Some questions: 1) It introduces a code defined behaviour other than depending on the spec defined behavior like DRIVER_OK, this will lead extra complexity in auditing 2) there's no guarantee that the interrupt handler is ready before virtqueue_add(), or it requires barriers before virtqueue_add() to make sure the handler is commit So it looks to me the virtio_device_ready() should be still the correct way to go: 1) it depends on spec defined behaviour like DRIVER_OK, and it then can comply with possible future security requirement of drivers defined in the spec 2) choose to use a new boolean instead of reusing vq->broken 3) enable the harden in driver one by one Does it make sense? Thanks > > > > -- > MST >
On Mon, Nov 28, 2022 at 05:14:44AM -0500, Michael S. Tsirkin wrote: > On Mon, Nov 28, 2022 at 10:10:01AM +0800, Li Zetao wrote: > > This patchset fixes similar issue, the root cause of the > > problem is that the virtqueues are not stopped on error > > handling path. > > I've been thinking about this. > Almost all drivers are affected. > > The reason really is that it used to be the right thing to do: > On legacy pci del_vqs writes 0 > into vq index and this resets the device as a side effect > (we actually do this multiple times, what e.g. writes of MSI vector > after the 1st reset do I have no idea). > > mmio ccw and modern pci don't. > > Given this has been with us for a while I am inlined to look for > a global solution rather than tweaking each driver. > > Given many drivers are supposed to work on legacy too, we know del_vqs > includes a reset for many of them. So I think I see a better way to do > this: > > Add virtio_reset_device_and_del_vqs() > > and convert all drivers to that. > > When doing this, we also need to/can fix a related problem (and related > to the hardening that Jason Wang was looking into): > virtio_reset_device is inherently racy: vq interrupts could > be in flight when we do reset. We need to prevent handlers from firing in > the window between reset and freeing the irq, so we should first > free irqs and only then start changing the state by e.g. > device reset. > > > Quite a lot of core work here. Jason are you still looking into > hardening? > Li Zetao, Jason, any updates. You guys looking into this? > > > Li Zetao (4): > > 9p: Fix probe failed when modprobe 9pnet_virtio > > virtio-mem: Fix probe failed when modprobe virtio_mem > > virtio-input: Fix probe failed when modprobe virtio_input > > virtio-blk: Fix probe failed when modprobe virtio_blk > > > > drivers/block/virtio_blk.c | 1 + > > drivers/virtio/virtio_input.c | 1 + > > drivers/virtio/virtio_mem.c | 1 + > > net/9p/trans_virtio.c | 1 + > > 4 files changed, 4 insertions(+) > > > > -- > > 2.25.1
On Fri, Jan 27, 2023 at 7:12 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Nov 28, 2022 at 05:14:44AM -0500, Michael S. Tsirkin wrote: > > On Mon, Nov 28, 2022 at 10:10:01AM +0800, Li Zetao wrote: > > > This patchset fixes similar issue, the root cause of the > > > problem is that the virtqueues are not stopped on error > > > handling path. > > > > I've been thinking about this. > > Almost all drivers are affected. > > > > The reason really is that it used to be the right thing to do: > > On legacy pci del_vqs writes 0 > > into vq index and this resets the device as a side effect > > (we actually do this multiple times, what e.g. writes of MSI vector > > after the 1st reset do I have no idea). > > > > mmio ccw and modern pci don't. > > > > Given this has been with us for a while I am inlined to look for > > a global solution rather than tweaking each driver. > > > > Given many drivers are supposed to work on legacy too, we know del_vqs > > includes a reset for many of them. So I think I see a better way to do > > this: > > > > Add virtio_reset_device_and_del_vqs() > > > > and convert all drivers to that. > > > > When doing this, we also need to/can fix a related problem (and related > > to the hardening that Jason Wang was looking into): > > virtio_reset_device is inherently racy: vq interrupts could > > be in flight when we do reset. We need to prevent handlers from firing in > > the window between reset and freeing the irq, so we should first > > free irqs and only then start changing the state by e.g. > > device reset. > > > > > > Quite a lot of core work here. Jason are you still looking into > > hardening? > > > > Li Zetao, Jason, any updates. You guys looking into this? At least I will continue the work of IRQ hardening. And this work could be done on top. Thanks > > > > > > > Li Zetao (4): > > > 9p: Fix probe failed when modprobe 9pnet_virtio > > > virtio-mem: Fix probe failed when modprobe virtio_mem > > > virtio-input: Fix probe failed when modprobe virtio_input > > > virtio-blk: Fix probe failed when modprobe virtio_blk > > > > > > drivers/block/virtio_blk.c | 1 + > > > drivers/virtio/virtio_input.c | 1 + > > > drivers/virtio/virtio_mem.c | 1 + > > > net/9p/trans_virtio.c | 1 + > > > 4 files changed, 4 insertions(+) > > > > > > -- > > > 2.25.1 >