| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 27 Dec 2022 17:09:12 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
maxime.coquelin@...hat.com, alvaro.karsz@...id-run.com,
eperezma@...hat.com
Subject: Re: [PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
在 2022/12/27 15:19, Michael S. Tsirkin 写道:
> On Tue, Dec 27, 2022 at 11:47:34AM +0800, Jason Wang wrote:
>> On Tue, Dec 27, 2022 at 7:34 AM Michael S. Tsirkin <mst@...hat.com> wrote:
>>> On Mon, Dec 26, 2022 at 03:49:07PM +0800, Jason Wang wrote:
>>>> This patch introduces a per virtqueue waitqueue to allow driver to
>>>> sleep and wait for more used. Two new helpers are introduced to allow
>>>> driver to sleep and wake up.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@...hat.com>
>>>> ---
>>>> Changes since V1:
>>>> - check virtqueue_is_broken() as well
>>>> - use more_used() instead of virtqueue_get_buf() to allow caller to
>>>> get buffers afterwards
>>>> ---
>>>> drivers/virtio/virtio_ring.c | 29 +++++++++++++++++++++++++++++
>>>> include/linux/virtio.h | 3 +++
>>>> 2 files changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>> index 5cfb2fa8abee..9c83eb945493 100644
>>>> --- a/drivers/virtio/virtio_ring.c
>>>> +++ b/drivers/virtio/virtio_ring.c
>>>> @@ -13,6 +13,7 @@
>>>> #include <linux/dma-mapping.h>
>>>> #include <linux/kmsan.h>
>>>> #include <linux/spinlock.h>
>>>> +#include <linux/wait.h>
>>>> #include <xen/xen.h>
>>>>
>>>> #ifdef DEBUG
>>>> @@ -60,6 +61,7 @@
>>>> "%s:"fmt, (_vq)->vq.name, ##args); \
>>>> /* Pairs with READ_ONCE() in virtqueue_is_broken(). */ \
>>>> WRITE_ONCE((_vq)->broken, true); \
>>>> + wake_up_interruptible(&(_vq)->wq); \
>>>> } while (0)
>>>> #define START_USE(vq)
>>>> #define END_USE(vq)
>>>> @@ -203,6 +205,9 @@ struct vring_virtqueue {
>>>> /* DMA, allocation, and size information */
>>>> bool we_own_ring;
>>>>
>>>> + /* Wait for buffer to be used */
>>>> + wait_queue_head_t wq;
>>>> +
>>>> #ifdef DEBUG
>>>> /* They're supposed to lock for us. */
>>>> unsigned int in_use;
>>>> @@ -2024,6 +2029,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
>>>> if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>>>> vq->weak_barriers = false;
>>>>
>>>> + init_waitqueue_head(&vq->wq);
>>>> +
>>>> err = vring_alloc_state_extra_packed(&vring_packed);
>>>> if (err)
>>>> goto err_state_extra;
>>>> @@ -2517,6 +2524,8 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
>>>> if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>>>> vq->weak_barriers = false;
>>>>
>>>> + init_waitqueue_head(&vq->wq);
>>>> +
>>>> err = vring_alloc_state_extra_split(vring_split);
>>>> if (err) {
>>>> kfree(vq);
>>>> @@ -2654,6 +2663,8 @@ static void vring_free(struct virtqueue *_vq)
>>>> {
>>>> struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>> + wake_up_interruptible(&vq->wq);
>>>> +
>>>> if (vq->we_own_ring) {
>>>> if (vq->packed_ring) {
>>>> vring_free_queue(vq->vq.vdev,
>>>> @@ -2863,4 +2874,22 @@ const struct vring *virtqueue_get_vring(struct virtqueue *vq)
>>>> }
>>>> EXPORT_SYMBOL_GPL(virtqueue_get_vring);
>>>>
>>>> +int virtqueue_wait_for_used(struct virtqueue *_vq)
>>>> +{
>>>> + struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +
>>>> + /* TODO: Tweak the timeout. */
>>>> + return wait_event_interruptible_timeout(vq->wq,
>>>> + virtqueue_is_broken(_vq) || more_used(vq), HZ);
>>> There's no good timeout. Let's not even go there, if device goes
>>> bad it should set the need reset bit.
>> The problem is that we can't depend on the device. If it takes too
>> long for the device to respond to cvq, there's a high possibility that
>> the device is buggy or even malicious. We can have a higher timeout
>> here and it should be still better than waiting forever (the cvq
>> commands need to be serialized so it needs to hold a lock anyway
>> (RTNL) ).
>>
>> Thanks
> With a TODO item like this I'd expect this to be an RFC.
> Here's why:
>
> Making driver more robust from device failures is a laudable goal but it's really
> hard to be 100% foolproof here. E.g. device can just block pci reads and
> it would be very hard to recover.
Yes.
> So I'm going to only merge patches
> like this if they at least theoretically have very little chance
> of breaking existing users.
AFAIK, this is not theoretical, consider:
1) DPU may implement virtio-net CVQ with codes running in CPU
2) VDUSE may want to support CVQ in the future
>
> And note that in most setups, CVQ is only used at startup and then left mostly alone.
>
> Finally, note that lots of guests need virtio to do anything useful at all.
> So just failing commands is not enough to recover - you need to try
> harder maybe by attempting to reset device.
This requires upper layer support which seems not existed in the
networking subsystem.
> Could be a question of
> policy - might need to make this guest configurable.
Yes.
Thanks
>
>
>
>>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(virtqueue_wait_for_used);
>>>> +
>>>> +void virtqueue_wake_up(struct virtqueue *_vq)
>>>> +{
>>>> + struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +
>>>> + wake_up_interruptible(&vq->wq);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(virtqueue_wake_up);
>>>> +
>>>> MODULE_LICENSE("GPL");
>>>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>>>> index dcab9c7e8784..2eb62c774895 100644
>>>> --- a/include/linux/virtio.h
>>>> +++ b/include/linux/virtio.h
>>>> @@ -72,6 +72,9 @@ void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
>>>> void *virtqueue_get_buf_ctx(struct virtqueue *vq, unsigned int *len,
>>>> void **ctx);
>>>>
>>>> +int virtqueue_wait_for_used(struct virtqueue *vq);
>>>> +void virtqueue_wake_up(struct virtqueue *vq);
>>>> +
>>>> void virtqueue_disable_cb(struct virtqueue *vq);
>>>>
>>>> bool virtqueue_enable_cb(struct virtqueue *vq);
>>>> --
>>>> 2.25.1
Powered by blists - more mailing lists