| 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
| ||
|
Message-ID: <6df97716-6669-4d94-8923-41758f71fc07@redhat.com>
Date: Tue, 19 Mar 2024 17:41:35 +1000
From: Gavin Shan <gshan@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Will Deacon <will@...nel.org>, virtualization@...ts.linux.dev,
linux-kernel@...r.kernel.org, jasowang@...hat.com,
xuanzhuo@...ux.alibaba.com, yihyu@...hat.com, shan.gavin@...il.com,
linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>, mochs@...dia.com
Subject: Re: [PATCH] virtio_ring: Fix the stale index in available ring
On 3/19/24 17:04, Michael S. Tsirkin wrote:
> On Tue, Mar 19, 2024 at 04:54:15PM +1000, Gavin Shan wrote:
>> On 3/19/24 16:10, Michael S. Tsirkin wrote:
>>> On Tue, Mar 19, 2024 at 02:09:34AM -0400, Michael S. Tsirkin wrote:
>>>> On Tue, Mar 19, 2024 at 02:59:23PM +1000, Gavin Shan wrote:
>>>>> On 3/19/24 02:59, Will Deacon wrote:
>> [...]
>>>>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>>>>> index 49299b1f9ec7..7d852811c912 100644
>>>>>>> --- a/drivers/virtio/virtio_ring.c
>>>>>>> +++ b/drivers/virtio/virtio_ring.c
>>>>>>> @@ -687,9 +687,15 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>>>>>> avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
>>>>>>> vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
>>>>>>> - /* Descriptors and available array need to be set before we expose the
>>>>>>> - * new available array entries. */
>>>>>>> - virtio_wmb(vq->weak_barriers);
>>>>>>> + /*
>>>>>>> + * Descriptors and available array need to be set before we expose
>>>>>>> + * the new available array entries. virtio_wmb() should be enough
>>>>>>> + * to ensuere the order theoretically. However, a stronger barrier
>>>>>>> + * is needed by ARM64. Otherwise, the stale data can be observed
>>>>>>> + * by the host (vhost). A stronger barrier should work for other
>>>>>>> + * architectures, but performance loss is expected.
>>>>>>> + */
>>>>>>> + virtio_mb(false);
>>>>>>> vq->split.avail_idx_shadow++;
>>>>>>> vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev,
>>>>>>> vq->split.avail_idx_shadow);
>>>>>>
>>>>>> Replacing a DMB with a DSB is _very_ unlikely to be the correct solution
>>>>>> here, especially when ordering accesses to coherent memory.
>>>>>>
>>>>>> In practice, either the larger timing different from the DSB or the fact
>>>>>> that you're going from a Store->Store barrier to a full barrier is what
>>>>>> makes things "work" for you. Have you tried, for example, a DMB SY
>>>>>> (e.g. via __smb_mb()).
>>>>>>
>>>>>> We definitely shouldn't take changes like this without a proper
>>>>>> explanation of what is going on.
>>>>>>
>>>>>
>>>>> Thanks for your comments, Will.
>>>>>
>>>>> Yes, DMB should work for us. However, it seems this instruction has issues on
>>>>> NVidia's grace-hopper. It's hard for me to understand how DMB and DSB works
>>>>> from hardware level. I agree it's not the solution to replace DMB with DSB
>>>>> before we fully understand the root cause.
>>>>>
>>>>> I tried the possible replacement like below. __smp_mb() can avoid the issue like
>>>>> __mb() does. __ndelay(10) can avoid the issue, but __ndelay(9) doesn't.
>>>>>
>>>>> static inline int virtqueue_add_split(struct virtqueue *_vq, ...)
>>>>> {
>>>>> :
>>>>> /* Put entry in available array (but don't update avail->idx until they
>>>>> * do sync). */
>>>>> avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
>>>>> vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
>>>>>
>>>>> /* Descriptors and available array need to be set before we expose the
>>>>> * new available array entries. */
>>>>> // Broken: virtio_wmb(vq->weak_barriers);
>>>>> // Broken: __dma_mb();
>>>>> // Work: __mb();
>>>>> // Work: __smp_mb();
>>>
>>> Did you try __smp_wmb ? And wmb?
>>>
>>
>> virtio_wmb(false) is equivalent to __smb_wmb(), which is broken.
>>
>> __wmb() works either. No issue found with it.
>
> Oh interesting. So how do smp_mb() and wmb() disassemble on this
> platform? Can you please check?
>
I don't see they have been translated wrongly on Nvidia's grace-hopper:
===> virtio_wmb(vq->weak_barriers)
0xffff8000807b07c8 <+1168>: ldrb w0, [x20, #66]
0xffff8000807b07cc <+1172>: cbz w0, 0xffff8000807b089c <virtqueue_add_split+1380>
0xffff8000807b07d0 <+1176>: dmb ishst // same to __smp_wmb()
:
0xffff8000807b089c <+1380>: dmb oshst // same to __dma_wmb()
0xffff8000807b08a0 <+1384>: b 0xffff8000807b07d4 <virtqueue_add_split+1180>
===> wmb()
0xffff8000807b07c8 <+1168>: dsb st
>
>>>>> // Work: __ndelay(100);
>>>>> // Work: __ndelay(10);
>>>>> // Broken: __ndelay(9);
>>>>>
>>>>> vq->split.avail_idx_shadow++;
>>>>> vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev,
>>>>> vq->split.avail_idx_shadow);
>>>>
>>>> What if you stick __ndelay here?
>>>
>>> And keep virtio_wmb above?
>>>
>>
>> The result has been shared through a separate reply.
>>
>>>>
>>>>> vq->num_added++;
>>>>>
>>>>> pr_debug("Added buffer head %i to %p\n", head, vq);
>>>>> END_USE(vq);
>>>>> :
>>>>> }
>>>>>
>>>>> I also tried to measure the consumed time for various barrier-relative instructions using
>>>>> ktime_get_ns() which should have consumed most of the time. __smb_mb() is slower than
>>>>> __smp_wmb() but faster than __mb()
>>>>>
>>>>> Instruction Range of used time in ns
>>>>> ----------------------------------------------
>>>>> __smp_wmb() [32 1128032]
>>>>> __smp_mb() [32 1160096]
>>>>> __mb() [32 1162496]
>>>>>
Thanks,
Gavin
Powered by blists - more mailing lists