LinuxLists.cc - [PATCH] rpmsg: virtio: Fix broken rpmsg

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

Hi,

On 6/29/22 19:43, Mathieu Poirier wrote:
> Hi Anup,
>
> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
>> fails due to both virtqueues (Rx and Tx) marked as broken by the
>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>>
>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
>> Signed-off-by: Anup Patel <[email protected]>
>> ---
>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 905ac7910c98..71a64d2c7644 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> /* and half is dedicated for TX */
>> vrp->sbufs = bufs_va + total_buf_space / 2;
>>
>> + /* From this point on, we can notify and get callbacks. */
>> + virtio_device_ready(vdev);
>> +
>
> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> potentially be called (by way of rpmsg_recv_done()), which will race with
> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> rpmsg_recv_done() will fail, potentially breaking remote processors' state
> machines that don't expect their initial name service to fail when the "device"
> has been marked as ready.
>
> What does make me curious though is that nobody on the remoteproc mailing list
> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> their rig?

I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
I confirm the issue!

Concerning the solution, I share Mathieu's concern. This could break legacy.
I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.

I this case the patch would be:

+ /*
+ * Unbreak the virtqueues to allow to add buffers before setting the vdev status
+ * to ready
+ */
+ __virtio_unbreak_device(vdev);
+

/* set up the receive buffers */
for (i = 0; i < vrp->num_bufs / 2; i++) {
struct scatterlist sg;
void *cpu_addr = vrp->rbufs + i * vrp->buf_size;

Regards,
Arnaud

>
> Thanks,
> Mathieu
>
>> /* set up the receive buffers */
>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>> struct scatterlist sg;
>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> */
>> notify = virtqueue_kick_prepare(vrp->rvq);
>>
>> - /* From this point on, we can notify and get callbacks. */
>> - virtio_device_ready(vdev);
>> -
>> /* tell the remote processor it can start sending messages */
>> /*
>> * this might be concurrent with callbacks, but we are only
>> --
>> 2.34.1
>>

2022-06-30 18:02:29

by Mathieu Poirier

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

+ [email protected]
+ [email protected]
+ [email protected]

On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
<[email protected]> wrote:
>
> Hi,
>
> On 6/29/22 19:43, Mathieu Poirier wrote:
> > Hi Anup,
> >
> > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> >> fails due to both virtqueues (Rx and Tx) marked as broken by the
> >> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> >> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> >>
> >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> >> Signed-off-by: Anup Patel <[email protected]>
> >> ---
> >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index 905ac7910c98..71a64d2c7644 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >> /* and half is dedicated for TX */
> >> vrp->sbufs = bufs_va + total_buf_space / 2;
> >>
> >> + /* From this point on, we can notify and get callbacks. */
> >> + virtio_device_ready(vdev);
> >> +
> >
> > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> > potentially be called (by way of rpmsg_recv_done()), which will race with
> > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> > rpmsg_recv_done() will fail, potentially breaking remote processors' state
> > machines that don't expect their initial name service to fail when the "device"
> > has been marked as ready.
> >
> > What does make me curious though is that nobody on the remoteproc mailing list
> > has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> > their rig?
>
> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> I confirm the issue!
>
> Concerning the solution, I share Mathieu's concern. This could break legacy.
> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
>
> I this case the patch would be:
>
> + /*
> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> + * to ready
> + */
> + __virtio_unbreak_device(vdev);
> +
>
> /* set up the receive buffers */
> for (i = 0; i < vrp->num_bufs / 2; i++) {
> struct scatterlist sg;
> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;

This will indeed fix the problem. On the flip side the kernel
documentation for __virtio_unbreak_device() puzzles me...
It clearly states that it should be used for probing and restoring but
_not_ directly by the driver. Function rpmsg_probe() is part of
probing but also the entry point to a driver.

Michael and virtualisation folks, is this the right way to move forward?

>
> Regards,
> Arnaud
>
> >
> > Thanks,
> > Mathieu
> >
> >> /* set up the receive buffers */
> >> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >> struct scatterlist sg;
> >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >> */
> >> notify = virtqueue_kick_prepare(vrp->rvq);
> >>
> >> - /* From this point on, we can notify and get callbacks. */
> >> - virtio_device_ready(vdev);
> >> -
> >> /* tell the remote processor it can start sending messages */
> >> /*
> >> * this might be concurrent with callbacks, but we are only
> >> --
> >> 2.34.1
> >>

2022-06-30 19:26:36

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> fails due to both virtqueues (Rx and Tx) marked as broken by the
> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>
> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> Signed-off-by: Anup Patel <[email protected]>

Yea, I've more or less reverted that. (There's VIRTIO_HARDEN_NOTIFICATION
that we'll use to try and develop this to something more reasonable).

> ---
> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 905ac7910c98..71a64d2c7644 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> /* and half is dedicated for TX */
> vrp->sbufs = bufs_va + total_buf_space / 2;
>
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* set up the receive buffers */
> for (i = 0; i < vrp->num_bufs / 2; i++) {
> struct scatterlist sg;
> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> */
> notify = virtqueue_kick_prepare(vrp->rvq);
>
> - /* From this point on, we can notify and get callbacks. */
> - virtio_device_ready(vdev);
> -
> /* tell the remote processor it can start sending messages */
> /*
> * this might be concurrent with callbacks, but we are only
> --
> 2.34.1
>
>

2022-06-30 19:42:46

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> + [email protected]
> + [email protected]
> + [email protected]
>
> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On 6/29/22 19:43, Mathieu Poirier wrote:
> > > Hi Anup,
> > >
> > > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> > >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> > >> fails due to both virtqueues (Rx and Tx) marked as broken by the
> > >> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> > >> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> > >>
> > >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > >> Signed-off-by: Anup Patel <[email protected]>
> > >> ---
> > >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> > >> 1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > >> index 905ac7910c98..71a64d2c7644 100644
> > >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > >> /* and half is dedicated for TX */
> > >> vrp->sbufs = bufs_va + total_buf_space / 2;
> > >>
> > >> + /* From this point on, we can notify and get callbacks. */
> > >> + virtio_device_ready(vdev);
> > >> +
> > >
> > > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> > > potentially be called (by way of rpmsg_recv_done()), which will race with
> > > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> > > rpmsg_recv_done() will fail, potentially breaking remote processors' state
> > > machines that don't expect their initial name service to fail when the "device"
> > > has been marked as ready.
> > >
> > > What does make me curious though is that nobody on the remoteproc mailing list
> > > has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> > > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> > > their rig?
> >
> > I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> > I confirm the issue!
> >
> > Concerning the solution, I share Mathieu's concern. This could break legacy.
> > I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> >
> > I this case the patch would be:
> >
> > + /*
> > + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> > + * to ready
> > + */
> > + __virtio_unbreak_device(vdev);
> > +
> >
> > /* set up the receive buffers */
> > for (i = 0; i < vrp->num_bufs / 2; i++) {
> > struct scatterlist sg;
> > void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>
> This will indeed fix the problem. On the flip side the kernel
> documentation for __virtio_unbreak_device() puzzles me...
> It clearly states that it should be used for probing and restoring but
> _not_ directly by the driver. Function rpmsg_probe() is part of
> probing but also the entry point to a driver.
>
> Michael and virtualisation folks, is this the right way to move forward?

I don't think it is, __virtio_unbreak_device is intended for core use.

> >
> > Regards,
> > Arnaud
> >
> > >
> > > Thanks,
> > > Mathieu
> > >
> > >> /* set up the receive buffers */
> > >> for (i = 0; i < vrp->num_bufs / 2; i++) {
> > >> struct scatterlist sg;
> > >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > >> */
> > >> notify = virtqueue_kick_prepare(vrp->rvq);
> > >>
> > >> - /* From this point on, we can notify and get callbacks. */
> > >> - virtio_device_ready(vdev);
> > >> -
> > >> /* tell the remote processor it can start sending messages */
> > >> /*
> > >> * this might be concurrent with callbacks, but we are only
> > >> --
> > >> 2.34.1
> > >>

2022-06-30 19:50:12

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Wed, Jun 29, 2022 at 11:43:18AM -0600, Mathieu Poirier wrote:
> Hi Anup,
>
> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> > The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> > fails due to both virtqueues (Rx and Tx) marked as broken by the
> > __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> > (which unbreaks queues) should be called before virtqueue_add_inbuf().
> >
> > Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > Signed-off-by: Anup Patel <[email protected]>
> > ---
> > drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 905ac7910c98..71a64d2c7644 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > /* and half is dedicated for TX */
> > vrp->sbufs = bufs_va + total_buf_space / 2;
> >
> > + /* From this point on, we can notify and get callbacks. */
> > + virtio_device_ready(vdev);
> > +
>
> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> potentially be called (by way of rpmsg_recv_done()), which will race with
> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> rpmsg_recv_done() will fail, potentially breaking remote processors' state
> machines that don't expect their initial name service to fail when the "device"
> has been marked as ready.

When you say available I am guessing you really need used.

With a non broken device you won't get a callback
until some buffers have been used.

Or, if no used buffers are present then you will get another
callback down the road.

>
> What does make me curious though is that nobody on the remoteproc mailing list
> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> their rig?
>
> Thanks,
> Mathieu
>
> > /* set up the receive buffers */
> > for (i = 0; i < vrp->num_bufs / 2; i++) {
> > struct scatterlist sg;
> > @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > */
> > notify = virtqueue_kick_prepare(vrp->rvq);
> >
> > - /* From this point on, we can notify and get callbacks. */
> > - virtio_device_ready(vdev);
> > -
> > /* tell the remote processor it can start sending messages */
> > /*
> > * this might be concurrent with callbacks, but we are only
> > --
> > 2.34.1
> >
>

2022-07-01 01:28:00

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> > + [email protected]
> > + [email protected]
> > + [email protected]
> >
> > On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On 6/29/22 19:43, Mathieu Poirier wrote:
> > > > Hi Anup,
> > > >
> > > > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> > > >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> > > >> fails due to both virtqueues (Rx and Tx) marked as broken by the
> > > >> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> > > >> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> > > >>
> > > >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > > >> Signed-off-by: Anup Patel <[email protected]>
> > > >> ---
> > > >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> > > >> 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > >> index 905ac7910c98..71a64d2c7644 100644
> > > >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > >> /* and half is dedicated for TX */
> > > >> vrp->sbufs = bufs_va + total_buf_space / 2;
> > > >>
> > > >> + /* From this point on, we can notify and get callbacks. */
> > > >> + virtio_device_ready(vdev);
> > > >> +
> > > >
> > > > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> > > > potentially be called (by way of rpmsg_recv_done()), which will race with
> > > > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> > > > rpmsg_recv_done() will fail, potentially breaking remote processors' state
> > > > machines that don't expect their initial name service to fail when the "device"
> > > > has been marked as ready.
> > > >
> > > > What does make me curious though is that nobody on the remoteproc mailing list
> > > > has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> > > > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> > > > their rig?
> > >
> > > I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> > > I confirm the issue!
> > >
> > > Concerning the solution, I share Mathieu's concern. This could break legacy.
> > > I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> > >
> > > I this case the patch would be:
> > >
> > > + /*
> > > + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> > > + * to ready
> > > + */
> > > + __virtio_unbreak_device(vdev);
> > > +
> > >
> > > /* set up the receive buffers */
> > > for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > struct scatterlist sg;
> > > void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >
> > This will indeed fix the problem. On the flip side the kernel
> > documentation for __virtio_unbreak_device() puzzles me...
> > It clearly states that it should be used for probing and restoring but
> > _not_ directly by the driver. Function rpmsg_probe() is part of
> > probing but also the entry point to a driver.
> >
> > Michael and virtualisation folks, is this the right way to move forward?
>
> I don't think it is, __virtio_unbreak_device is intended for core use.

Can we fill the rx after virtio_device_ready() in this case?

Btw, the driver set driver ok after registering, we probably get a svq
kick before DRIVER_OK?

Thanks

>
> > >
> > > Regards,
> > > Arnaud
> > >
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > >> /* set up the receive buffers */
> > > >> for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > >> struct scatterlist sg;
> > > >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > >> */
> > > >> notify = virtqueue_kick_prepare(vrp->rvq);
> > > >>
> > > >> - /* From this point on, we can notify and get callbacks. */
> > > >> - virtio_device_ready(vdev);
> > > >> -
> > > >> /* tell the remote processor it can start sending messages */
> > > >> /*
> > > >> * this might be concurrent with callbacks, but we are only
> > > >> --
> > > >> 2.34.1
> > > >>
>

2022-07-01 06:40:45

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> > > + [email protected]
> > > + [email protected]
> > > + [email protected]
> > >
> > > On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> > > <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 6/29/22 19:43, Mathieu Poirier wrote:
> > > > > Hi Anup,
> > > > >
> > > > > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> > > > >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> > > > >> fails due to both virtqueues (Rx and Tx) marked as broken by the
> > > > >> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> > > > >> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> > > > >>
> > > > >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > > > >> Signed-off-by: Anup Patel <[email protected]>
> > > > >> ---
> > > > >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> > > > >> 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > >> index 905ac7910c98..71a64d2c7644 100644
> > > > >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > >> /* and half is dedicated for TX */
> > > > >> vrp->sbufs = bufs_va + total_buf_space / 2;
> > > > >>
> > > > >> + /* From this point on, we can notify and get callbacks. */
> > > > >> + virtio_device_ready(vdev);
> > > > >> +
> > > > >
> > > > > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> > > > > potentially be called (by way of rpmsg_recv_done()), which will race with
> > > > > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> > > > > rpmsg_recv_done() will fail, potentially breaking remote processors' state
> > > > > machines that don't expect their initial name service to fail when the "device"
> > > > > has been marked as ready.
> > > > >
> > > > > What does make me curious though is that nobody on the remoteproc mailing list
> > > > > has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> > > > > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> > > > > their rig?
> > > >
> > > > I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> > > > I confirm the issue!
> > > >
> > > > Concerning the solution, I share Mathieu's concern. This could break legacy.
> > > > I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> > > >
> > > > I this case the patch would be:
> > > >
> > > > + /*
> > > > + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> > > > + * to ready
> > > > + */
> > > > + __virtio_unbreak_device(vdev);
> > > > +
> > > >
> > > > /* set up the receive buffers */
> > > > for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > > struct scatterlist sg;
> > > > void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> > >
> > > This will indeed fix the problem. On the flip side the kernel
> > > documentation for __virtio_unbreak_device() puzzles me...
> > > It clearly states that it should be used for probing and restoring but
> > > _not_ directly by the driver. Function rpmsg_probe() is part of
> > > probing but also the entry point to a driver.
> > >
> > > Michael and virtualisation folks, is this the right way to move forward?
> >
> > I don't think it is, __virtio_unbreak_device is intended for core use.
>
> Can we fill the rx after virtio_device_ready() in this case?
>
> Btw, the driver set driver ok after registering, we probably get a svq
> kick before DRIVER_OK?
>
> Thanks

Is this an ack for the original patch?

> >
> > > >
> > > > Regards,
> > > > Arnaud
> > > >
> > > > >
> > > > > Thanks,
> > > > > Mathieu
> > > > >
> > > > >> /* set up the receive buffers */
> > > > >> for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > > >> struct scatterlist sg;
> > > > >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > >> */
> > > > >> notify = virtqueue_kick_prepare(vrp->rvq);
> > > > >>
> > > > >> - /* From this point on, we can notify and get callbacks. */
> > > > >> - virtio_device_ready(vdev);
> > > > >> -
> > > > >> /* tell the remote processor it can start sending messages */
> > > > >> /*
> > > > >> * this might be concurrent with callbacks, but we are only
> > > > >> --
> > > > >> 2.34.1
> > > > >>
> >

2022-07-01 09:07:31

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

Hello,

On 6/30/22 21:19, Michael S. Tsirkin wrote:
> On Wed, Jun 29, 2022 at 11:43:18AM -0600, Mathieu Poirier wrote:
>> Hi Anup,
>>
>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>>>
>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
>>> Signed-off-by: Anup Patel <[email protected]>
>>> ---
>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 905ac7910c98..71a64d2c7644 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> /* and half is dedicated for TX */
>>> vrp->sbufs = bufs_va + total_buf_space / 2;
>>>
>>> + /* From this point on, we can notify and get callbacks. */
>>> + virtio_device_ready(vdev);
>>> +
>>
>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
>> potentially be called (by way of rpmsg_recv_done()), which will race with
>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
>> machines that don't expect their initial name service to fail when the "device"
>> has been marked as ready.
>
> When you say available I am guessing you really need used.
>
> With a non broken device you won't get a callback
> until some buffers have been used.
>
> Or, if no used buffers are present then you will get another
> callback down the road.

In current implementation the Linux rpmsg_virtio driver allocates the
virtio buffers for the coprocessor rpmsg virtio device transmission and
then updates the virtio device status in shared memory to inform the
coprocessor that it is ready for inter-processor communication.

So from coprocessor perspective, when the virtio device is ready
(set to VIRTIO_CONFIG_S_DRIVER_OK), it can
start to get available buffers and send virtio buffers to the Linux.

With the patch proposed, the virtio is set to VIRTIO_CONFIG_S_DRIVER_OK
while no buffer are available for the coprocessor transmission.

I'm agree that, if the Linux rpmsg_virtio driver has not allocated the
buffer, the coprocessor will fail to get available virtio buffer for
communication and so has "just" to wait that some buffers are available
in the virtqueue.

But this change the behavior and can lead to an unexpected error case
for some legacy coprocessor firmware...
Should we take the risk that this legacy is no longer compatible?

That said regarding the virtio spec 1.1 chapter 3.1.1 [1], I also wonder
if the introduction of the virqueue broken flag is compliant with the
spec?
But i guess this is probably a matter of interpretation...

"
The driver MUST follow this sequence to initialize a device:
[...]
7. Perform device-specific setup, including discovery of virtqueues for
the device, optional per-bus setup, reading and possibly writing the
device’s virtio configuration space, and population of virtqueues.
8. Set the DRIVER_OK status bit. At this point the device is “live”.
"

My question is what means in point 7. "and population of virtqueues"?

In my interpretation the call of "virtqueue_add_inbuf()" populates the
RX virtqueue.
That would mean that calling virtqueue_add_inbuf before calling
virtio_device_ready() should be possible.

Thanks,
Arnaud

[1]https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-920001

>
>
>>
>> What does make me curious though is that nobody on the remoteproc mailing list
>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
>> their rig?
>>
>> Thanks,
>> Mathieu
>>
>>> /* set up the receive buffers */
>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>> struct scatterlist sg;
>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> */
>>> notify = virtqueue_kick_prepare(vrp->rvq);
>>>
>>> - /* From this point on, we can notify and get callbacks. */
>>> - virtio_device_ready(vdev);
>>> -
>>> /* tell the remote processor it can start sending messages */
>>> /*
>>> * this might be concurrent with callbacks, but we are only
>>> --
>>> 2.34.1
>>>
>>
>

2022-07-01 09:47:39

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Fri, Jul 01, 2022 at 11:00:28AM +0200, Arnaud POULIQUEN wrote:
> Hello,
>
> On 6/30/22 21:19, Michael S. Tsirkin wrote:
> > On Wed, Jun 29, 2022 at 11:43:18AM -0600, Mathieu Poirier wrote:
> >> Hi Anup,
> >>
> >> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> >>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> >>> fails due to both virtqueues (Rx and Tx) marked as broken by the
> >>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> >>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> >>>
> >>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> >>> Signed-off-by: Anup Patel <[email protected]>
> >>> ---
> >>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> index 905ac7910c98..71a64d2c7644 100644
> >>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>> /* and half is dedicated for TX */
> >>> vrp->sbufs = bufs_va + total_buf_space / 2;
> >>>
> >>> + /* From this point on, we can notify and get callbacks. */
> >>> + virtio_device_ready(vdev);
> >>> +
> >>
> >> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> >> potentially be called (by way of rpmsg_recv_done()), which will race with
> >> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> >> rpmsg_recv_done() will fail, potentially breaking remote processors' state
> >> machines that don't expect their initial name service to fail when the "device"
> >> has been marked as ready.
> >
> > When you say available I am guessing you really need used.
> >
> > With a non broken device you won't get a callback
> > until some buffers have been used.
> >
> > Or, if no used buffers are present then you will get another
> > callback down the road.
>
> In current implementation the Linux rpmsg_virtio driver allocates the
> virtio buffers for the coprocessor rpmsg virtio device transmission and
> then updates the virtio device status in shared memory to inform the
> coprocessor that it is ready for inter-processor communication.
>
> So from coprocessor perspective, when the virtio device is ready
> (set to VIRTIO_CONFIG_S_DRIVER_OK), it can
> start to get available buffers and send virtio buffers to the Linux.
>
> With the patch proposed, the virtio is set to VIRTIO_CONFIG_S_DRIVER_OK
> while no buffer are available for the coprocessor transmission.
>
> I'm agree that, if the Linux rpmsg_virtio driver has not allocated the
> buffer, the coprocessor will fail to get available virtio buffer for
> communication and so has "just" to wait that some buffers are available
> in the virtqueue.
>
> But this change the behavior and can lead to an unexpected error case
> for some legacy coprocessor firmware...
> Should we take the risk that this legacy is no longer compatible?
>
>
> That said regarding the virtio spec 1.1 chapter 3.1.1 [1], I also wonder
> if the introduction of the virqueue broken flag is compliant with the
> spec?
> But i guess this is probably a matter of interpretation...
>
> "
> The driver MUST follow this sequence to initialize a device:
> [...]
> 7. Perform device-specific setup, including discovery of virtqueues for
> the device, optional per-bus setup, reading and possibly writing the
> device’s virtio configuration space, and population of virtqueues.
> 8. Set the DRIVER_OK status bit. At this point the device is “live”.
> "
>
> My question is what means in point 7. "and population of virtqueues"?
>
> In my interpretation the call of "virtqueue_add_inbuf()" populates the
> RX virtqueue.
> That would mean that calling virtqueue_add_inbuf before calling
> virtio_device_ready() should be possible.
>
> Thanks,
> Arnaud
>
> [1]https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-920001

I think I agree. For example, the networking device uses "population" in this
sense:

It is generally a good idea to keep the receive virtqueue as
fully populated as possible: if it runs out, network performance
will suffer.

>
> >
> >
> >>
> >> What does make me curious though is that nobody on the remoteproc mailing list
> >> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> >> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> >> their rig?
> >>
> >> Thanks,
> >> Mathieu
> >>
> >>> /* set up the receive buffers */
> >>> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >>> struct scatterlist sg;
> >>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>> */
> >>> notify = virtqueue_kick_prepare(vrp->rvq);
> >>>
> >>> - /* From this point on, we can notify and get callbacks. */
> >>> - virtio_device_ready(vdev);
> >>> -
> >>> /* tell the remote processor it can start sending messages */
> >>> /*
> >>> * this might be concurrent with callbacks, but we are only
> >>> --
> >>> 2.34.1
> >>>
> >>
> >

2022-07-04 04:59:23

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
> > On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> > > > + [email protected]
> > > > + [email protected]
> > > > + [email protected]
> > > >
> > > > On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 6/29/22 19:43, Mathieu Poirier wrote:
> > > > > > Hi Anup,
> > > > > >
> > > > > > On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> > > > > >> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> > > > > >> fails due to both virtqueues (Rx and Tx) marked as broken by the
> > > > > >> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> > > > > >> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> > > > > >>
> > > > > >> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > > > > >> Signed-off-by: Anup Patel <[email protected]>
> > > > > >> ---
> > > > > >> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> > > > > >> 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >>
> > > > > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > >> index 905ac7910c98..71a64d2c7644 100644
> > > > > >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > >> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > > >> /* and half is dedicated for TX */
> > > > > >> vrp->sbufs = bufs_va + total_buf_space / 2;
> > > > > >>
> > > > > >> + /* From this point on, we can notify and get callbacks. */
> > > > > >> + virtio_device_ready(vdev);
> > > > > >> +
> > > > > >
> > > > > > Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> > > > > > potentially be called (by way of rpmsg_recv_done()), which will race with
> > > > > > virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> > > > > > rpmsg_recv_done() will fail, potentially breaking remote processors' state
> > > > > > machines that don't expect their initial name service to fail when the "device"
> > > > > > has been marked as ready.
> > > > > >
> > > > > > What does make me curious though is that nobody on the remoteproc mailing list
> > > > > > has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> > > > > > i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> > > > > > their rig?
> > > > >
> > > > > I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> > > > > I confirm the issue!
> > > > >
> > > > > Concerning the solution, I share Mathieu's concern. This could break legacy.
> > > > > I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> > > > >
> > > > > I this case the patch would be:
> > > > >
> > > > > + /*
> > > > > + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> > > > > + * to ready
> > > > > + */
> > > > > + __virtio_unbreak_device(vdev);
> > > > > +
> > > > >
> > > > > /* set up the receive buffers */
> > > > > for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > > > struct scatterlist sg;
> > > > > void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> > > >
> > > > This will indeed fix the problem. On the flip side the kernel
> > > > documentation for __virtio_unbreak_device() puzzles me...
> > > > It clearly states that it should be used for probing and restoring but
> > > > _not_ directly by the driver. Function rpmsg_probe() is part of
> > > > probing but also the entry point to a driver.
> > > >
> > > > Michael and virtualisation folks, is this the right way to move forward?
> > >
> > > I don't think it is, __virtio_unbreak_device is intended for core use.
> >
> > Can we fill the rx after virtio_device_ready() in this case?
> >
> > Btw, the driver set driver ok after registering, we probably get a svq
> > kick before DRIVER_OK?
> >
> > Thanks
>
> Is this an ack for the original patch?

Nope, I meant, instead of moving virtio_device_ready() a little bit
earlier, can we only move the rvq filling after virtio_device_ready().

Thanks

>
> > >
> > > > >
> > > > > Regards,
> > > > > Arnaud
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Mathieu
> > > > > >
> > > > > >> /* set up the receive buffers */
> > > > > >> for (i = 0; i < vrp->num_bufs / 2; i++) {
> > > > > >> struct scatterlist sg;
> > > > > >> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > > >> */
> > > > > >> notify = virtqueue_kick_prepare(vrp->rvq);
> > > > > >>
> > > > > >> - /* From this point on, we can notify and get callbacks. */
> > > > > >> - virtio_device_ready(vdev);
> > > > > >> -
> > > > > >> /* tell the remote processor it can start sending messages */
> > > > > >> /*
> > > > > >> * this might be concurrent with callbacks, but we are only
> > > > > >> --
> > > > > >> 2.34.1
> > > > > >>
> > >
>

2022-07-04 10:18:33

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

Hello Jason,

On 7/4/22 06:35, Jason Wang wrote:
> On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
>>
>> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
>>> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
>>>>
>>>> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
>>>>> + [email protected]
>>>>> + [email protected]
>>>>> + [email protected]
>>>>>
>>>>> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 6/29/22 19:43, Mathieu Poirier wrote:
>>>>>>> Hi Anup,
>>>>>>>
>>>>>>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
>>>>>>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
>>>>>>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
>>>>>>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
>>>>>>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>>>>>>>>
>>>>>>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
>>>>>>>> Signed-off-by: Anup Patel <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>> index 905ac7910c98..71a64d2c7644 100644
>>>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>> /* and half is dedicated for TX */
>>>>>>>> vrp->sbufs = bufs_va + total_buf_space / 2;
>>>>>>>>
>>>>>>>> + /* From this point on, we can notify and get callbacks. */
>>>>>>>> + virtio_device_ready(vdev);
>>>>>>>> +
>>>>>>>
>>>>>>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
>>>>>>> potentially be called (by way of rpmsg_recv_done()), which will race with
>>>>>>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
>>>>>>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
>>>>>>> machines that don't expect their initial name service to fail when the "device"
>>>>>>> has been marked as ready.
>>>>>>>
>>>>>>> What does make me curious though is that nobody on the remoteproc mailing list
>>>>>>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
>>>>>>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
>>>>>>> their rig?
>>>>>>
>>>>>> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
>>>>>> I confirm the issue!
>>>>>>
>>>>>> Concerning the solution, I share Mathieu's concern. This could break legacy.
>>>>>> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
>>>>>>
>>>>>> I this case the patch would be:
>>>>>>
>>>>>> + /*
>>>>>> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
>>>>>> + * to ready
>>>>>> + */
>>>>>> + __virtio_unbreak_device(vdev);
>>>>>> +
>>>>>>
>>>>>> /* set up the receive buffers */
>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>> struct scatterlist sg;
>>>>>> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>>>>
>>>>> This will indeed fix the problem. On the flip side the kernel
>>>>> documentation for __virtio_unbreak_device() puzzles me...
>>>>> It clearly states that it should be used for probing and restoring but
>>>>> _not_ directly by the driver. Function rpmsg_probe() is part of
>>>>> probing but also the entry point to a driver.
>>>>>
>>>>> Michael and virtualisation folks, is this the right way to move forward?
>>>>
>>>> I don't think it is, __virtio_unbreak_device is intended for core use.
>>>
>>> Can we fill the rx after virtio_device_ready() in this case?
>>>
>>> Btw, the driver set driver ok after registering, we probably get a svq
>>> kick before DRIVER_OK?

By "registering" you mean calling rpmsg_virtio_add_ctrl_dev and
rpmsg_ns_register_device?

The rpmsg_ns_register_device has to be called before. Because it has to be
probed to handle the first message coming from the remote side to create
associated rpmsg local device. It doesn't send message.

The risk could be for the rpmsg_ctrl device. Registering it
after the virtio_device_ready(vdev) call could make sense...

>>>
>>> Thanks
>>
>> Is this an ack for the original patch?
>
> Nope, I meant, instead of moving virtio_device_ready() a little bit
> earlier, can we only move the rvq filling after virtio_device_ready().
>
> Thanks

Please find some concerns about this inversion here:
https://lore.kernel.org/lkml/[email protected]/

Regarding __virtio_unbreak_device. The pending virtio_break_device is
used by some virtio driver.
Could we consider that it makes sense to also have a
virtio_unbreak_device interface?

I do not well understand the reason of the commit:
8b4ec69d7e09 ("virtio: harden vring IRQ", 2022-05-27)
So following alternative is probably pretty naive:
Is the use of virtqueue_disable_cb could be an alternative to the
vq->broken usage allowing to register buffer while preventing virtqueue IRQ?

Thanks,
Arnaud

>
>>
>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Arnaud
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Mathieu
>>>>>>>
>>>>>>>> /* set up the receive buffers */
>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>>>> struct scatterlist sg;
>>>>>>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>> */
>>>>>>>> notify = virtqueue_kick_prepare(vrp->rvq);
>>>>>>>>
>>>>>>>> - /* From this point on, we can notify and get callbacks. */
>>>>>>>> - virtio_device_ready(vdev);
>>>>>>>> -
>>>>>>>> /* tell the remote processor it can start sending messages */
>>>>>>>> /*
>>>>>>>> * this might be concurrent with callbacks, but we are only
>>>>>>>> --
>>>>>>>> 2.34.1
>>>>>>>>
>>>>
>>
>

2022-07-06 04:22:42

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Mon, Jul 4, 2022 at 5:45 PM Arnaud POULIQUEN
<[email protected]> wrote:
>
> Hello Jason,
>
> On 7/4/22 06:35, Jason Wang wrote:
> > On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
> >>
> >> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
> >>> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
> >>>>
> >>>> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> >>>>> + [email protected]
> >>>>> + [email protected]
> >>>>> + [email protected]
> >>>>>
> >>>>> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> >>>>> <[email protected]> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 6/29/22 19:43, Mathieu Poirier wrote:
> >>>>>>> Hi Anup,
> >>>>>>>
> >>>>>>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> >>>>>>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> >>>>>>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
> >>>>>>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> >>>>>>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> >>>>>>>>
> >>>>>>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> >>>>>>>> Signed-off-by: Anup Patel <[email protected]>
> >>>>>>>> ---
> >>>>>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> >>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>> index 905ac7910c98..71a64d2c7644 100644
> >>>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>>>> /* and half is dedicated for TX */
> >>>>>>>> vrp->sbufs = bufs_va + total_buf_space / 2;
> >>>>>>>>
> >>>>>>>> + /* From this point on, we can notify and get callbacks. */
> >>>>>>>> + virtio_device_ready(vdev);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> >>>>>>> potentially be called (by way of rpmsg_recv_done()), which will race with
> >>>>>>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> >>>>>>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
> >>>>>>> machines that don't expect their initial name service to fail when the "device"
> >>>>>>> has been marked as ready.
> >>>>>>>
> >>>>>>> What does make me curious though is that nobody on the remoteproc mailing list
> >>>>>>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> >>>>>>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> >>>>>>> their rig?
> >>>>>>
> >>>>>> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> >>>>>> I confirm the issue!
> >>>>>>
> >>>>>> Concerning the solution, I share Mathieu's concern. This could break legacy.
> >>>>>> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> >>>>>>
> >>>>>> I this case the patch would be:
> >>>>>>
> >>>>>> + /*
> >>>>>> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> >>>>>> + * to ready
> >>>>>> + */
> >>>>>> + __virtio_unbreak_device(vdev);
> >>>>>> +
> >>>>>>
> >>>>>> /* set up the receive buffers */
> >>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >>>>>> struct scatterlist sg;
> >>>>>> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >>>>>
> >>>>> This will indeed fix the problem. On the flip side the kernel
> >>>>> documentation for __virtio_unbreak_device() puzzles me...
> >>>>> It clearly states that it should be used for probing and restoring but
> >>>>> _not_ directly by the driver. Function rpmsg_probe() is part of
> >>>>> probing but also the entry point to a driver.
> >>>>>
> >>>>> Michael and virtualisation folks, is this the right way to move forward?
> >>>>
> >>>> I don't think it is, __virtio_unbreak_device is intended for core use.
> >>>
> >>> Can we fill the rx after virtio_device_ready() in this case?
> >>>
> >>> Btw, the driver set driver ok after registering, we probably get a svq
> >>> kick before DRIVER_OK?
>
> By "registering" you mean calling rpmsg_virtio_add_ctrl_dev and
> rpmsg_ns_register_device?

Yes.

>
> The rpmsg_ns_register_device has to be called before. Because it has to be
> probed to handle the first message coming from the remote side to create
> associated rpmsg local device.

I couldn't find the code to do this, maybe you can give me some hint on this.

> It doesn't send message.

I see the function register the device to the bus, I wonder if this
means the device could be probed and used by the driver before
virtio_device_ready().

>
> The risk could be for the rpmsg_ctrl device. Registering it
> after the virtio_device_ready(vdev) call could make sense...

I see.

>
> >>>
> >>> Thanks
> >>
> >> Is this an ack for the original patch?
> >
> > Nope, I meant, instead of moving virtio_device_ready() a little bit
> > earlier, can we only move the rvq filling after virtio_device_ready().
> >
> > Thanks
>
> Please find some concerns about this inversion here:
> https://lore.kernel.org/lkml/[email protected]/
>
> Regarding __virtio_unbreak_device. The pending virtio_break_device is
> used by some virtio driver.
> Could we consider that it makes sense to also have a
> virtio_unbreak_device interface?

We don't want to allow the driver to unbreak a device since it's
easier to have bugs.

>
>
> I do not well understand the reason of the commit:
> 8b4ec69d7e09 ("virtio: harden vring IRQ", 2022-05-27)

It tries to forbid the virtqueue callbacks to be called before
virtio_device_ready(). This helps to prevent the malicious device from
attacking the driver.

But unfortunately, it breaks several driver because:

1) some driver have races in probe/remove
2) it tries to reuse vq->broken which may break the driver that call
virqueue_add() before virtio_device_ready() which is allowed by the
spec

There's a discussion to have a better behavior that doesn't break the
existing drivers. And the IRQ hardening feature is marked as broken
now, so rpmsg should be fine without any extra effort.

> So following alternative is probably pretty naive:
> Is the use of virtqueue_disable_cb could be an alternative to the
> vq->broken usage allowing to register buffer while preventing virtqueue IRQ?

Probably not, there's no guarantee that the device will not send
notification after virqtueue_disable_cb().

Thanks

>
> Thanks,
> Arnaud
>
> >
> >>
> >>>>
> >>>>>>
> >>>>>> Regards,
> >>>>>> Arnaud
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Mathieu
> >>>>>>>
> >>>>>>>> /* set up the receive buffers */
> >>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >>>>>>>> struct scatterlist sg;
> >>>>>>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>>>> */
> >>>>>>>> notify = virtqueue_kick_prepare(vrp->rvq);
> >>>>>>>>
> >>>>>>>> - /* From this point on, we can notify and get callbacks. */
> >>>>>>>> - virtio_device_ready(vdev);
> >>>>>>>> -
> >>>>>>>> /* tell the remote processor it can start sending messages */
> >>>>>>>> /*
> >>>>>>>> * this might be concurrent with callbacks, but we are only
> >>>>>>>> --
> >>>>>>>> 2.34.1
> >>>>>>>>
> >>>>
> >>
> >
>

2022-07-06 06:59:11

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On 7/6/22 06:03, Jason Wang wrote:
> On Mon, Jul 4, 2022 at 5:45 PM Arnaud POULIQUEN
> <[email protected]> wrote:
>>
>> Hello Jason,
>>
>> On 7/4/22 06:35, Jason Wang wrote:
>>> On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
>>>>
>>>> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
>>>>> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
>>>>>>
>>>>>> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
>>>>>>> + [email protected]
>>>>>>> + [email protected]
>>>>>>> + [email protected]
>>>>>>>
>>>>>>> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 6/29/22 19:43, Mathieu Poirier wrote:
>>>>>>>>> Hi Anup,
>>>>>>>>>
>>>>>>>>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
>>>>>>>>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
>>>>>>>>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
>>>>>>>>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
>>>>>>>>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>>>>>>>>>>
>>>>>>>>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
>>>>>>>>>> Signed-off-by: Anup Patel <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>> index 905ac7910c98..71a64d2c7644 100644
>>>>>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>>>> /* and half is dedicated for TX */
>>>>>>>>>> vrp->sbufs = bufs_va + total_buf_space / 2;
>>>>>>>>>>
>>>>>>>>>> + /* From this point on, we can notify and get callbacks. */
>>>>>>>>>> + virtio_device_ready(vdev);
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
>>>>>>>>> potentially be called (by way of rpmsg_recv_done()), which will race with
>>>>>>>>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
>>>>>>>>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
>>>>>>>>> machines that don't expect their initial name service to fail when the "device"
>>>>>>>>> has been marked as ready.
>>>>>>>>>
>>>>>>>>> What does make me curious though is that nobody on the remoteproc mailing list
>>>>>>>>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
>>>>>>>>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
>>>>>>>>> their rig?
>>>>>>>>
>>>>>>>> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
>>>>>>>> I confirm the issue!
>>>>>>>>
>>>>>>>> Concerning the solution, I share Mathieu's concern. This could break legacy.
>>>>>>>> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
>>>>>>>>
>>>>>>>> I this case the patch would be:
>>>>>>>>
>>>>>>>> + /*
>>>>>>>> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
>>>>>>>> + * to ready
>>>>>>>> + */
>>>>>>>> + __virtio_unbreak_device(vdev);
>>>>>>>> +
>>>>>>>>
>>>>>>>> /* set up the receive buffers */
>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>>>> struct scatterlist sg;
>>>>>>>> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>>>>>>
>>>>>>> This will indeed fix the problem. On the flip side the kernel
>>>>>>> documentation for __virtio_unbreak_device() puzzles me...
>>>>>>> It clearly states that it should be used for probing and restoring but
>>>>>>> _not_ directly by the driver. Function rpmsg_probe() is part of
>>>>>>> probing but also the entry point to a driver.
>>>>>>>
>>>>>>> Michael and virtualisation folks, is this the right way to move forward?
>>>>>>
>>>>>> I don't think it is, __virtio_unbreak_device is intended for core use.
>>>>>
>>>>> Can we fill the rx after virtio_device_ready() in this case?
>>>>>
>>>>> Btw, the driver set driver ok after registering, we probably get a svq
>>>>> kick before DRIVER_OK?
>>
>> By "registering" you mean calling rpmsg_virtio_add_ctrl_dev and
>> rpmsg_ns_register_device?
>
> Yes.
>
>>
>> The rpmsg_ns_register_device has to be called before. Because it has to be
>> probed to handle the first message coming from the remote side to create
>> associated rpmsg local device.
>
> I couldn't find the code to do this, maybe you can give me some hint on this.

The rpmsg_ns is available here :
https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_ns.c

It is probed on rpmsg_ns_register_device call.
https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L974

>
>> It doesn't send message.
>
> I see the function register the device to the bus, I wonder if this
> means the device could be probed and used by the driver before
> virtio_device_ready().
>
>>
>> The risk could be for the rpmsg_ctrl device. Registering it
>> after the virtio_device_ready(vdev) call could make sense...
>
> I see.
>
>>
>>>>>
>>>>> Thanks
>>>>
>>>> Is this an ack for the original patch?
>>>
>>> Nope, I meant, instead of moving virtio_device_ready() a little bit
>>> earlier, can we only move the rvq filling after virtio_device_ready().
>>>
>>> Thanks
>>
>> Please find some concerns about this inversion here:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> Regarding __virtio_unbreak_device. The pending virtio_break_device is
>> used by some virtio driver.
>> Could we consider that it makes sense to also have a
>> virtio_unbreak_device interface?
>
> We don't want to allow the driver to unbreak a device since it's
> easier to have bugs.
>
>>
>>
>> I do not well understand the reason of the commit:
>> 8b4ec69d7e09 ("virtio: harden vring IRQ", 2022-05-27)
>
> It tries to forbid the virtqueue callbacks to be called before
> virtio_device_ready(). This helps to prevent the malicious device from
> attacking the driver.
>
> But unfortunately, it breaks several driver because:
>
> 1) some driver have races in probe/remove
> 2) it tries to reuse vq->broken which may break the driver that call
> virqueue_add() before virtio_device_ready() which is allowed by the
> spec
>
> There's a discussion to have a better behavior that doesn't break the
> existing drivers. And the IRQ hardening feature is marked as broken
> now, so rpmsg should be fine without any extra effort.

Thanks for the explanations.
If the discussions are in a mail thread could you give me the reference?

Thanks,
Arnaud

>
>> So following alternative is probably pretty naive:
>> Is the use of virtqueue_disable_cb could be an alternative to the
>> vq->broken usage allowing to register buffer while preventing virtqueue IRQ?
>
> Probably not, there's no guarantee that the device will not send
> notification after virqtueue_disable_cb().
>
> Thanks
>
>>
>> Thanks,
>> Arnaud
>>
>>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Arnaud
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Mathieu
>>>>>>>>>
>>>>>>>>>> /* set up the receive buffers */
>>>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>>>>>> struct scatterlist sg;
>>>>>>>>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>>>> */
>>>>>>>>>> notify = virtqueue_kick_prepare(vrp->rvq);
>>>>>>>>>>
>>>>>>>>>> - /* From this point on, we can notify and get callbacks. */
>>>>>>>>>> - virtio_device_ready(vdev);
>>>>>>>>>> -
>>>>>>>>>> /* tell the remote processor it can start sending messages */
>>>>>>>>>> /*
>>>>>>>>>> * this might be concurrent with callbacks, but we are only
>>>>>>>>>> --
>>>>>>>>>> 2.34.1
>>>>>>>>>>
>>>>>>
>>>>
>>>
>>
>

2022-07-08 06:42:32

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On Wed, Jul 6, 2022 at 2:57 PM Arnaud POULIQUEN
<[email protected]> wrote:
>
>
>
> On 7/6/22 06:03, Jason Wang wrote:
> > On Mon, Jul 4, 2022 at 5:45 PM Arnaud POULIQUEN
> > <[email protected]> wrote:
> >>
> >> Hello Jason,
> >>
> >> On 7/4/22 06:35, Jason Wang wrote:
> >>> On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
> >>>>
> >>>> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
> >>>>> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
> >>>>>>
> >>>>>> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
> >>>>>>> + [email protected]
> >>>>>>> + [email protected]
> >>>>>>> + [email protected]
> >>>>>>>
> >>>>>>> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
> >>>>>>> <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On 6/29/22 19:43, Mathieu Poirier wrote:
> >>>>>>>>> Hi Anup,
> >>>>>>>>>
> >>>>>>>>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
> >>>>>>>>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
> >>>>>>>>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
> >>>>>>>>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
> >>>>>>>>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> >>>>>>>>>> Signed-off-by: Anup Patel <[email protected]>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> >>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>>>> index 905ac7910c98..71a64d2c7644 100644
> >>>>>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>>>>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>>>>>> /* and half is dedicated for TX */
> >>>>>>>>>> vrp->sbufs = bufs_va + total_buf_space / 2;
> >>>>>>>>>>
> >>>>>>>>>> + /* From this point on, we can notify and get callbacks. */
> >>>>>>>>>> + virtio_device_ready(vdev);
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
> >>>>>>>>> potentially be called (by way of rpmsg_recv_done()), which will race with
> >>>>>>>>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
> >>>>>>>>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
> >>>>>>>>> machines that don't expect their initial name service to fail when the "device"
> >>>>>>>>> has been marked as ready.
> >>>>>>>>>
> >>>>>>>>> What does make me curious though is that nobody on the remoteproc mailing list
> >>>>>>>>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
> >>>>>>>>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
> >>>>>>>>> their rig?
> >>>>>>>>
> >>>>>>>> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
> >>>>>>>> I confirm the issue!
> >>>>>>>>
> >>>>>>>> Concerning the solution, I share Mathieu's concern. This could break legacy.
> >>>>>>>> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
> >>>>>>>>
> >>>>>>>> I this case the patch would be:
> >>>>>>>>
> >>>>>>>> + /*
> >>>>>>>> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
> >>>>>>>> + * to ready
> >>>>>>>> + */
> >>>>>>>> + __virtio_unbreak_device(vdev);
> >>>>>>>> +
> >>>>>>>>
> >>>>>>>> /* set up the receive buffers */
> >>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >>>>>>>> struct scatterlist sg;
> >>>>>>>> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >>>>>>>
> >>>>>>> This will indeed fix the problem. On the flip side the kernel
> >>>>>>> documentation for __virtio_unbreak_device() puzzles me...
> >>>>>>> It clearly states that it should be used for probing and restoring but
> >>>>>>> _not_ directly by the driver. Function rpmsg_probe() is part of
> >>>>>>> probing but also the entry point to a driver.
> >>>>>>>
> >>>>>>> Michael and virtualisation folks, is this the right way to move forward?
> >>>>>>
> >>>>>> I don't think it is, __virtio_unbreak_device is intended for core use.
> >>>>>
> >>>>> Can we fill the rx after virtio_device_ready() in this case?
> >>>>>
> >>>>> Btw, the driver set driver ok after registering, we probably get a svq
> >>>>> kick before DRIVER_OK?
> >>
> >> By "registering" you mean calling rpmsg_virtio_add_ctrl_dev and
> >> rpmsg_ns_register_device?
> >
> > Yes.
> >
> >>
> >> The rpmsg_ns_register_device has to be called before. Because it has to be
> >> probed to handle the first message coming from the remote side to create
> >> associated rpmsg local device.
> >
> > I couldn't find the code to do this, maybe you can give me some hint on this.
>
> The rpmsg_ns is available here :
> https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_ns.c
>
> It is probed on rpmsg_ns_register_device call.
> https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L974

Yes but what I want to ask is, it looks to me
rpmsg_ns_register_device() only creates a rpmsg device. Do you mean
the rpmsg driver that will handle the first message during its probe?

>
>
> >
> >> It doesn't send message.
> >
> > I see the function register the device to the bus, I wonder if this
> > means the device could be probed and used by the driver before
> > virtio_device_ready().
> >
> >>
> >> The risk could be for the rpmsg_ctrl device. Registering it
> >> after the virtio_device_ready(vdev) call could make sense...
> >
> > I see.
> >
> >>
> >>>>>
> >>>>> Thanks
> >>>>
> >>>> Is this an ack for the original patch?
> >>>
> >>> Nope, I meant, instead of moving virtio_device_ready() a little bit
> >>> earlier, can we only move the rvq filling after virtio_device_ready().
> >>>
> >>> Thanks
> >>
> >> Please find some concerns about this inversion here:
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >> Regarding __virtio_unbreak_device. The pending virtio_break_device is
> >> used by some virtio driver.
> >> Could we consider that it makes sense to also have a
> >> virtio_unbreak_device interface?
> >
> > We don't want to allow the driver to unbreak a device since it's
> > easier to have bugs.
> >
> >>
> >>
> >> I do not well understand the reason of the commit:
> >> 8b4ec69d7e09 ("virtio: harden vring IRQ", 2022-05-27)
> >
> > It tries to forbid the virtqueue callbacks to be called before
> > virtio_device_ready(). This helps to prevent the malicious device from
> > attacking the driver.
> >
> > But unfortunately, it breaks several driver because:
> >
> > 1) some driver have races in probe/remove
> > 2) it tries to reuse vq->broken which may break the driver that call
> > virqueue_add() before virtio_device_ready() which is allowed by the
> > spec
> >
> > There's a discussion to have a better behavior that doesn't break the
> > existing drivers. And the IRQ hardening feature is marked as broken
> > now, so rpmsg should be fine without any extra effort.
>
> Thanks for the explanations.
> If the discussions are in a mail thread could you give me the reference?

Here're the discussions and commits:

https://lore.kernel.org/lkml/[email protected]/

https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=6a9720576cd00d30722c5f755bd17d4cfa9df636

Thanks

>
> Thanks,
> Arnaud
>
> >
> >> So following alternative is probably pretty naive:
> >> Is the use of virtqueue_disable_cb could be an alternative to the
> >> vq->broken usage allowing to register buffer while preventing virtqueue IRQ?
> >
> > Probably not, there's no guarantee that the device will not send
> > notification after virqtueue_disable_cb().
> >
> > Thanks
> >
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> Regards,
> >>>>>>>> Arnaud
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> Mathieu
> >>>>>>>>>
> >>>>>>>>>> /* set up the receive buffers */
> >>>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
> >>>>>>>>>> struct scatterlist sg;
> >>>>>>>>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>>>>>> */
> >>>>>>>>>> notify = virtqueue_kick_prepare(vrp->rvq);
> >>>>>>>>>>
> >>>>>>>>>> - /* From this point on, we can notify and get callbacks. */
> >>>>>>>>>> - virtio_device_ready(vdev);
> >>>>>>>>>> -
> >>>>>>>>>> /* tell the remote processor it can start sending messages */
> >>>>>>>>>> /*
> >>>>>>>>>> * this might be concurrent with callbacks, but we are only
> >>>>>>>>>> --
> >>>>>>>>>> 2.34.1
> >>>>>>>>>>
> >>>>>>
> >>>>
> >>>
> >>
> >
>

2022-07-08 08:25:18

[permalink] [raw]

Subject: Re: [PATCH] rpmsg: virtio: Fix broken rpmsg_probe()

On 7/8/22 08:19, Jason Wang wrote:
> On Wed, Jul 6, 2022 at 2:57 PM Arnaud POULIQUEN
> <[email protected]> wrote:
>>
>>
>>
>> On 7/6/22 06:03, Jason Wang wrote:
>>> On Mon, Jul 4, 2022 at 5:45 PM Arnaud POULIQUEN
>>> <[email protected]> wrote:
>>>>
>>>> Hello Jason,
>>>>
>>>> On 7/4/22 06:35, Jason Wang wrote:
>>>>> On Fri, Jul 1, 2022 at 2:16 PM Michael S. Tsirkin <[email protected]> wrote:
>>>>>>
>>>>>> On Fri, Jul 01, 2022 at 09:22:15AM +0800, Jason Wang wrote:
>>>>>>> On Fri, Jul 1, 2022 at 3:20 AM Michael S. Tsirkin <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On Thu, Jun 30, 2022 at 11:51:30AM -0600, Mathieu Poirier wrote:
>>>>>>>>> + [email protected]
>>>>>>>>> + [email protected]
>>>>>>>>> + [email protected]
>>>>>>>>>
>>>>>>>>> On Thu, 30 Jun 2022 at 10:20, Arnaud POULIQUEN
>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 6/29/22 19:43, Mathieu Poirier wrote:
>>>>>>>>>>> Hi Anup,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jun 08, 2022 at 10:43:34PM +0530, Anup Patel wrote:
>>>>>>>>>>>> The rpmsg_probe() is broken at the moment because virtqueue_add_inbuf()
>>>>>>>>>>>> fails due to both virtqueues (Rx and Tx) marked as broken by the
>>>>>>>>>>>> __vring_new_virtqueue() function. To solve this, virtio_device_ready()
>>>>>>>>>>>> (which unbreaks queues) should be called before virtqueue_add_inbuf().
>>>>>>>>>>>>
>>>>>>>>>>>> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
>>>>>>>>>>>> Signed-off-by: Anup Patel <[email protected]>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>>>>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>>>> index 905ac7910c98..71a64d2c7644 100644
>>>>>>>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>>>>>>>> @@ -929,6 +929,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>>>>>> /* and half is dedicated for TX */
>>>>>>>>>>>> vrp->sbufs = bufs_va + total_buf_space / 2;
>>>>>>>>>>>>
>>>>>>>>>>>> + /* From this point on, we can notify and get callbacks. */
>>>>>>>>>>>> + virtio_device_ready(vdev);
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> Calling virtio_device_ready() here means that virtqueue_get_buf_ctx_split() can
>>>>>>>>>>> potentially be called (by way of rpmsg_recv_done()), which will race with
>>>>>>>>>>> virtqueue_add_inbuf(). If buffers in the virtqueue aren't available then
>>>>>>>>>>> rpmsg_recv_done() will fail, potentially breaking remote processors' state
>>>>>>>>>>> machines that don't expect their initial name service to fail when the "device"
>>>>>>>>>>> has been marked as ready.
>>>>>>>>>>>
>>>>>>>>>>> What does make me curious though is that nobody on the remoteproc mailing list
>>>>>>>>>>> has complained about commit 8b4ec69d7e09 breaking their environment... By now,
>>>>>>>>>>> i.e rc4, that should have happened. Anyone from TI, ST and Xilinx care to test this on
>>>>>>>>>>> their rig?
>>>>>>>>>>
>>>>>>>>>> I tested on STm32mp1 board using tag v5.19-rc4(03c765b0e3b4)
>>>>>>>>>> I confirm the issue!
>>>>>>>>>>
>>>>>>>>>> Concerning the solution, I share Mathieu's concern. This could break legacy.
>>>>>>>>>> I made a short test and I would suggest to use __virtio_unbreak_device instead, tounbreak the virtqueues without changing the init sequence.
>>>>>>>>>>
>>>>>>>>>> I this case the patch would be:
>>>>>>>>>>
>>>>>>>>>> + /*
>>>>>>>>>> + * Unbreak the virtqueues to allow to add buffers before setting the vdev status
>>>>>>>>>> + * to ready
>>>>>>>>>> + */
>>>>>>>>>> + __virtio_unbreak_device(vdev);
>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> /* set up the receive buffers */
>>>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>>>>>> struct scatterlist sg;
>>>>>>>>>> void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>>>>>>>>
>>>>>>>>> This will indeed fix the problem. On the flip side the kernel
>>>>>>>>> documentation for __virtio_unbreak_device() puzzles me...
>>>>>>>>> It clearly states that it should be used for probing and restoring but
>>>>>>>>> _not_ directly by the driver. Function rpmsg_probe() is part of
>>>>>>>>> probing but also the entry point to a driver.
>>>>>>>>>
>>>>>>>>> Michael and virtualisation folks, is this the right way to move forward?
>>>>>>>>
>>>>>>>> I don't think it is, __virtio_unbreak_device is intended for core use.
>>>>>>>
>>>>>>> Can we fill the rx after virtio_device_ready() in this case?
>>>>>>>
>>>>>>> Btw, the driver set driver ok after registering, we probably get a svq
>>>>>>> kick before DRIVER_OK?
>>>>
>>>> By "registering" you mean calling rpmsg_virtio_add_ctrl_dev and
>>>> rpmsg_ns_register_device?
>>>
>>> Yes.
>>>
>>>>
>>>> The rpmsg_ns_register_device has to be called before. Because it has to be
>>>> probed to handle the first message coming from the remote side to create
>>>> associated rpmsg local device.
>>>
>>> I couldn't find the code to do this, maybe you can give me some hint on this.
>>
>> The rpmsg_ns is available here :
>> https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_ns.c
>>
>> It is probed on rpmsg_ns_register_device call.
>> https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L974
>
> Yes but what I want to ask is, it looks to me
> rpmsg_ns_register_device() only creates a rpmsg device. Do you mean
> the rpmsg driver that will handle the first message during its probe?

No it will be out of its probe, in its callback. the callback is called
by the virtio-rpmsg based on the rpmsg receiver address.

For the details:
In rpmsg virtio implementation there is a mechanism to discover the
RPMsg services supported by the remote processor: the name service
announcement. For instance for the rpmsg_tty[1], the remote processor
sends a rpmsg service announcement message indicating that it supports
the "rpmsg-tty" service.
On linux side the rpmsg_ns receives the message and creates a rpmsg
channel that leads to a rpmsg_tty device creation on the rpmsg bus.

If the rpmsg_ns is not registered (so no rpmsg receiver address
registered), then when the "ns announcement" is received,the message
is dropped, the service not initialized.

[1]:https://elixir.bootlin.com/linux/v5.19-rc4/source/drivers/tty/rpmsg_tty.c

>
>>
>>
>>>
>>>> It doesn't send message.
>>>
>>> I see the function register the device to the bus, I wonder if this
>>> means the device could be probed and used by the driver before
>>> virtio_device_ready().
>>>
>>>>
>>>> The risk could be for the rpmsg_ctrl device. Registering it
>>>> after the virtio_device_ready(vdev) call could make sense...
>>>
>>> I see.
>>>
>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>
>>>>>> Is this an ack for the original patch?
>>>>>
>>>>> Nope, I meant, instead of moving virtio_device_ready() a little bit
>>>>> earlier, can we only move the rvq filling after virtio_device_ready().
>>>>>
>>>>> Thanks
>>>>
>>>> Please find some concerns about this inversion here:
>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> Regarding __virtio_unbreak_device. The pending virtio_break_device is
>>>> used by some virtio driver.
>>>> Could we consider that it makes sense to also have a
>>>> virtio_unbreak_device interface?
>>>
>>> We don't want to allow the driver to unbreak a device since it's
>>> easier to have bugs.
>>>
>>>>
>>>>
>>>> I do not well understand the reason of the commit:
>>>> 8b4ec69d7e09 ("virtio: harden vring IRQ", 2022-05-27)
>>>
>>> It tries to forbid the virtqueue callbacks to be called before
>>> virtio_device_ready(). This helps to prevent the malicious device from
>>> attacking the driver.
>>>
>>> But unfortunately, it breaks several driver because:
>>>
>>> 1) some driver have races in probe/remove
>>> 2) it tries to reuse vq->broken which may break the driver that call
>>> virqueue_add() before virtio_device_ready() which is allowed by the
>>> spec
>>>
>>> There's a discussion to have a better behavior that doesn't break the
>>> existing drivers. And the IRQ hardening feature is marked as broken
>>> now, so rpmsg should be fine without any extra effort.
>>
>> Thanks for the explanations.
>> If the discussions are in a mail thread could you give me the reference?
>
> Here're the discussions and commits:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=c346dae4f3fbce51bbd4f2ec5e8c6f9b91e93163
> https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=6a9720576cd00d30722c5f755bd17d4cfa9df636

Thanks for the links!
So no more update planed in drivers/rpmsg/virtio_rpmsg_bus.c, if i well understood...

Thanks,
Arnaud

>
> Thanks
>
>>
>> Thanks,
>> Arnaud
>>
>>>
>>>> So following alternative is probably pretty naive:
>>>> Is the use of virtqueue_disable_cb could be an alternative to the
>>>> vq->broken usage allowing to register buffer while preventing virtqueue IRQ?
>>>
>>> Probably not, there's no guarantee that the device will not send
>>> notification after virqtueue_disable_cb().
>>>
>>> Thanks
>>>
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Arnaud
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Mathieu
>>>>>>>>>>>
>>>>>>>>>>>> /* set up the receive buffers */
>>>>>>>>>>>> for (i = 0; i < vrp->num_bufs / 2; i++) {
>>>>>>>>>>>> struct scatterlist sg;
>>>>>>>>>>>> @@ -983,9 +986,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>>>>>> */
>>>>>>>>>>>> notify = virtqueue_kick_prepare(vrp->rvq);
>>>>>>>>>>>>
>>>>>>>>>>>> - /* From this point on, we can notify and get callbacks. */
>>>>>>>>>>>> - virtio_device_ready(vdev);
>>>>>>>>>>>> -
>>>>>>>>>>>> /* tell the remote processor it can start sending messages */
>>>>>>>>>>>> /*
>>>>>>>>>>>> * this might be concurrent with callbacks, but we are only
>>>>>>>>>>>> --
>>>>>>>>>>>> 2.34.1
>>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

2022-07-12 09:09:36