LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To: Jason Wang <jasowang@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	<linux-remoteproc@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	<linux-stm32@st-md-mailman.stormreply.com>
Subject: Re: [PATCH] rpmsg: virtio: don't let virtio core to validate used length
Date: Wed, 24 Nov 2021 16:48:20 +0100	[thread overview]
Message-ID: <9dfb7cb7-d4b8-095f-c863-bedbb6b80c4c@foss.st.com> (raw)
In-Reply-To: <CACGkMEtC9sD2MX20Q3C3ecWH_TXCQMTS1qDLevKhuv=dyR14sQ@mail.gmail.com>



On 11/24/21 3:12 AM, Jason Wang wrote:
> On Tue, Nov 23, 2021 at 9:31 PM Arnaud POULIQUEN
> <arnaud.pouliquen@foss.st.com> 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@foss.st.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>> 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
>>>
>>
> 

      reply	other threads:[~2021-11-24 15:48 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-22 16:08 Arnaud Pouliquen
2021-11-23  2:15 ` Jason Wang
2021-11-23  2:16   ` Jason Wang
2021-11-23  6:15 ` Michael S. Tsirkin
2021-11-23 13:31   ` Arnaud POULIQUEN
2021-11-24  2:12     ` Jason Wang
2021-11-24 15:48       ` Arnaud POULIQUEN [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9dfb7cb7-d4b8-095f-c863-bedbb6b80c4c@foss.st.com \
    --to=arnaud.pouliquen@foss.st.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=jasowang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=mst@redhat.com \
    --cc=ohad@wizery.com \
    --subject='Re: [PATCH] rpmsg: virtio: don'\''t let virtio core to validate used length' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).