On 6/3/2026 8:01 PM, Dmitry Baryshkov wrote:
> On Wed, Jun 03, 2026 at 06:14:30PM +0800, Chunkai Deng wrote:
>> glink_smem_rx_advance() wraps the tail index with a single subtraction,
>> which only corrects for one full wrap. The advance count is derived from
>> remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
>> if such a count reaches or exceeds pipe->native.length, the tail remains
> Would not such a packet already cause issues as it will overflow the
> FIFO?
Yes -- looking at qcom_glink_rx_data() again, the existing avail check
("avail < sizeof(hdr) + chunk_size") makes the scenario I described in
the commit message effectively unreachable in practice: if a remote
ever supplied a chunk_size such that the advance count would exceed
pipe->native.length, that check would never pass and we would return
-EAGAIN indefinitely without ever reaching rx_advance. So the
out-of-bounds offset I described would only arise from a malformed
remote, and at that point modulo does not make anything more
trustworthy.
As mentioned in my reply to Bjorn, I’ll split this patch out and
discuss its optimization separately.
Thanks,
Chunkai
>> outside [0, length) after the subtraction and the next FIFO access uses
>> an out-of-bounds offset.
>>
>> Use modulo so the tail is always normalised back into [0, length),
>> keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
>> checks added to the FIFO helpers.
>>
>> Signed-off-by: Chunkai Deng <[email protected]>
>> ---
>> drivers/rpmsg/qcom_glink_smem.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_smem.c
>> b/drivers/rpmsg/qcom_glink_smem.c
>> index 42ad315d7910..4f143921b719 100644
>> --- a/drivers/rpmsg/qcom_glink_smem.c
>> +++ b/drivers/rpmsg/qcom_glink_smem.c
>> @@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe
>> *np,
>>
>> tail += count;
>> if (tail >= pipe->native.length)
>> - tail -= pipe->native.length;
>> + tail %= pipe->native.length;
>>
>> *pipe->tail = cpu_to_le32(tail);
>> }
>>
>> --
>> 2.34.1
>>
