On 11/24/21 3:12 AM, Jason Wang wrote: > On Tue, Nov 23, 2021 at 9:31 PM Arnaud POULIQUEN > <arnaud.pouliquen@xxxxxxxxxxx> wrote: >> >> Hello Mickael, Jason, >> >> On 11/23/21 7:15 AM, Michael S. Tsirkin wrote: >>> On Mon, Nov 22, 2021 at 05:08:12PM +0100, Arnaud Pouliquen wrote: >>>> For RX virtqueue, the used length is validated in all the three paths >>>> (big, small and mergeable). For control vq, we never tries to use used >>>> length. So this patch forbids the core to validate the used length. >>> >>> Jason commented on this. This is copy paste from virtio net >>> where the change was merely an optimization. >>> >> >> Right, I did it too fast last night (European time) to share the regression as >> soon as possible. >> For that, I copied and pasted the first commit I found related to the problem. >> Need to rework this. >> >>>> Without patch the rpmsg client sample does not work. >>> >>> Hmm that's not enough of a description. Could you please >>> provide more detail? Does rpmsg device set used length to a >>> value > dma read buffer size? what kind of error message >>> do you get? what are the plans to fix the device? >> >> Let's me explain the context. >> I run the rpmsg client sample test to communicate with a remote processor >> that runs a Zephyr FW designed to answer to the Linux kernel driver sample. >> >> The Zephyr is relying on OpenAMP library to implement the RPMsg and VirtIO layers. >> >> In TX direction (Linux to Zephyr) 8 buffers of 512 bytes are allocated. >> The first 8 RPMsg sent are OK. But when virtio loop back the the TX buffer index >> 0 (so already used and free one time) the following error occurs in >> virtqueue_get_buf_ctx_split[1]: >> " virtio_rpmsg_bus virtio0: output:used len 28 is larger than in buflen 0" >> >> I have investigated the problem further today. Here is my analysis >> >> rpmsg_send_offchannel_raw >> -> virtqueue_add_outbuf >> -> virtqueue_add >> -> virtqueue_add_split >> Here we use the "out_sgs" (in_sgs == 0) >> buflen is not incremented in loop [2] >> We don't enter in loop [3] as "in_sgs == 0" >> consequence is that vq->buflen[head] is set to 0 [4] >> >> [1] >> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L799 >> [2] >> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L551 >> [3] >> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L567 >> [4] >> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L622 >> >> >> An alternative to fix the issue is to set buflen in loop 2, but I'm not enough >> expert to ensure that this will not have any side effect... >> >> @@ -559,10 +559,11 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> * table since it use stream DMA mapping. >> */ >> i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length, >> VRING_DESC_F_NEXT, >> indirect); >> + buflen += sg->length; > > This is not what spec what: > > "Each entry in the ring is a pair: id indicates the head entry of the > descriptor chain describing the buffer (this matches an entry placed > in the available ring by the guest earlier), and len the total of > bytes written into the buffer." > > For TX, the used length should be 0. > >> } >> } >> for (; n < (out_sgs + in_sgs); n++) { >> >> So can you tell me if you prefer me to send a V2 updating the commit message or >> a new message to fix virtio_ring (or both)? > > See above, for the driver side, the suppress_used_validation is > sufficient. For the device side, it needs to be fixed (0 for TX used > length) too. I confirm that set len to 0 in virtq_used_elem struct, when release the in-buffer on device (remote processor) side, fixes the issue. Need to improve the behaviour in OpenAMP lib but for legacy support of the different Virtio libraries the suppress_used_validation seems necessary. I will send a V2 with an update of the commit message Thanks, Arnaud > > Thanks > >> >> Thanks, >> Arnaud >> >>> >>>> Fixes: 939779f5152d ("virtio_ring: validate used buffer length") >>>> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx> >>>> Cc: Jason Wang <jasowang@xxxxxxxxxx> >>>> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx> >>>> --- >>>> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf >>>> --- >>>> drivers/rpmsg/virtio_rpmsg_bus.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c >>>> index 9c112aa65040..5f73f19c2c38 100644 >>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c >>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c >>>> @@ -1054,6 +1054,7 @@ static struct virtio_driver virtio_ipc_driver = { >>>> .feature_table_size = ARRAY_SIZE(features), >>>> .driver.name = KBUILD_MODNAME, >>>> .driver.owner = THIS_MODULE, >>>> + .suppress_used_validation = true, >>>> .id_table = id_table, >>>> .probe = rpmsg_probe, >>>> .remove = rpmsg_remove, >>>> -- >>>> 2.17.1 >>> >> >
