2023-01-09 22:43:57

by Bjorn Andersson

Subject: [PATCH] rpmsg: glink: Avoid infinite loop on intent for missing channel

In the event that an intent advertisement arrives on an unknown channel
the fifo is not advanced, resulting in the same message being handled
over and over.

Fixes: dacbb35e930f ("rpmsg: glink: Receive and store the remote intent buffers")
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index f36740cb6866..7b1320b1579e 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -946,12 +946,12 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
spin_unlock_irqrestore(&glink->idr_lock, flags);
if (!channel) {
dev_err(glink->dev, "intents for non-existing channel\n");
- return;
+ goto advance_rx;
}

msg = kmalloc(msglen, GFP_ATOMIC);
if (!msg)
- return;
+ goto advance_rx;

qcom_glink_rx_peak(glink, msg, 0, msglen);

@@ -973,6 +973,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
}

kfree(msg);
+advance_rx:
qcom_glink_rx_advance(glink, ALIGN(msglen, 8));
}

--
2.37.3

2023-02-14 22:55:11

by Chris Lew

Subject: Re: [PATCH] rpmsg: glink: Avoid infinite loop on intent for missing channel

On 1/9/2023 2:38 PM, Bjorn Andersson wrote:
> In the event that an intent advertisement arrives on an unknown channel
> the fifo is not advanced, resulting in the same message being handled
> over and over.
>
> Fixes: dacbb35e930f ("rpmsg: glink: Receive and store the remote intent buffers")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index f36740cb6866..7b1320b1579e 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -946,12 +946,12 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
> spin_unlock_irqrestore(&glink->idr_lock, flags);
> if (!channel) {
> dev_err(glink->dev, "intents for non-existing channel\n");
> - return;
> + goto advance_rx;
> }
>
> msg = kmalloc(msglen, GFP_ATOMIC);
> if (!msg)
> - return;
> + goto advance_rx;


Should we be dropping the packet for this case? If we try again later
more memory might be available to handle the command.

>
> qcom_glink_rx_peak(glink, msg, 0, msglen);
>
> @@ -973,6 +973,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
> }
>
> kfree(msg);
> +advance_rx:
> qcom_glink_rx_advance(glink, ALIGN(msglen, 8));
> }
>
>

2023-02-14 23:34:33

by Bjorn Andersson

Subject: Re: [PATCH] rpmsg: glink: Avoid infinite loop on intent for missing channel

On Tue, Feb 14, 2023 at 02:55:02PM -0800, Chris Lew wrote:
> On 1/9/2023 2:38 PM, Bjorn Andersson wrote:
> > In the event that an intent advertisement arrives on an unknown channel
> > the fifo is not advanced, resulting in the same message being handled
> > over and over.
> >
> > Fixes: dacbb35e930f ("rpmsg: glink: Receive and store the remote intent buffers")
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/rpmsg/qcom_glink_native.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> > index f36740cb6866..7b1320b1579e 100644
> > --- a/drivers/rpmsg/qcom_glink_native.c
> > +++ b/drivers/rpmsg/qcom_glink_native.c
> > @@ -946,12 +946,12 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
> > spin_unlock_irqrestore(&glink->idr_lock, flags);
> > if (!channel) {
> > dev_err(glink->dev, "intents for non-existing channel\n");
> > - return;
> > + goto advance_rx;
> > }
> > msg = kmalloc(msglen, GFP_ATOMIC);
> > if (!msg)
> > - return;
> > + goto advance_rx;
>
>
> Should we be dropping the packet for this case? If we try again later more
> memory might be available to handle the command.
>

You're right, we found a channel above, but we don't have enough memory
to handle the message right now. That seems like a message worth not
throwing away.

Thanks,
Bjorn

> > qcom_glink_rx_peak(glink, msg, 0, msglen);
> > @@ -973,6 +973,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
> > }
> > kfree(msg);
> > +advance_rx:
> > qcom_glink_rx_advance(glink, ALIGN(msglen, 8));
> > }
> >