linux-kernel - Re: [PATCH 1/2] rpmsg: glink: Handle rejected intent request better

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZxfHKncYLeiIH3io@hovoldconsulting.com>
Date: Tue, 22 Oct 2024 17:39:22 +0200
From: Johan Hovold <johan@...nel.org>
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Chris Lew <quic_clew@...cinc.com>,
	Konrad Dybcio <konradybcio@...nel.org>,
	linux-arm-msm@...r.kernel.org,
	Bjorn Andersson <quic_bjorande@...cinc.com>,
	linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH 1/2] rpmsg: glink: Handle rejected intent request better

On Tue, Oct 22, 2024 at 04:17:11AM +0000, Bjorn Andersson wrote:
> The initial implementation of request intent response handling dealt
> with two outcomes; granted allocations, and all other cases being
> considered -ECANCELLED (likely from "cancelling the operation as the
> remote is going down").

For the benefit of casual reviewers and contributors, could you add
introductory comment about what "intents" are?

> But on some channels intent allocation is not supported, instead the
> remote will pre-allocate and announce a fixed number of intents for the
> sender to use. If for such channels an rpmsg_send() is being invoked
> before any channels have been announced, an intent request will be
> issued and as this comes back rejected the call is failed with
> -ECANCELLED.

It's actually the one L -ECANCELED

s/is failed/fails/ ?
 
> Given that this is reported in the same way as the remote being shut
> down, there's no way for the client to differentiate the two cases.
> 
> In line with the original GLINK design, change the return value to
> -EAGAIN for the case where the remote rejects an intent allocation
> request.
> 
> It's tempting to handle this case in the GLINK core, as we expect
> intents to show up in this case. But there's no way to distinguish
> between this case and a rejection for a too big allocation, nor is it
> possible to predict if a currently used (and seeminly suitable) intent

seemingly

> will be returned for reuse or not. As such, returning the error to the
> client and allow it to react seems to be the only sensible solution.

s/allow/allowing/ ?

> In addition to this, commit 'c05dfce0b89e ("rpmsg: glink: Wait for
> intent, not just request ack")' changed the logic such that the code
> always wait for an intent request response and an intent. This works out
> in most cases, but in the event that a intent request is rejected and no

an intent

> further intent arrives (e.g. client asks for a too big intent), the code
> will stall for 10 seconds and then return -ETIMEDOUT; instead of a more
> suitable error.
> 
> This change also resulted in intent requests racing with the shutdown of
> the remote would be exposed to this same problem, unless some intent
> happens to arrive. A patch for this was developed and posted by Sarannya
> S [1], and has been incorporated here.
> 
> To summarize, the intent request can end in 4 ways:
> - Timeout, no response arrived => return -ETIMEDOUT
> - Abort TX, the edge is going away => return -ECANCELLED
> - Intent request was rejected => return -EAGAIN
> - Intent request was accepted, and an intent arrived => return 0
> 
> This patch was developed with input from Sarannya S, Deepak Kumar Singh,
> and Chris Lew.
> 
> [1] https://lore.kernel.org/all/20240925072328.1163183-1-quic_deesin@quicinc.com/
> 
> Fixes: c05dfce0b89e ("rpmsg: glink: Wait for intent, not just request ack")
> Cc: stable@...r.kernel.org
> Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>

Nit picks aside, this was all nice and clear.

Tested-by: Johan Hovold <johan+linaro@...nel.org>