If error path is taken while filling descriptor for a frame, skb
pointer is left in the entry. Later, on the ring entry reuse, the
same entry could be used as a part of a multi-descriptor frame,
and skb for that new frame could be stored in a different entry.

Then, the stale pointer will reach the completion routine, and passed
to the release operation.

Fix that by clearing the saved skb pointer at the error path.

Fixes: d2c96b9d5f83 ("net: rswitch: Add jumbo frames handling for TX")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 32b32aa7e01f..3ad5858d3cdd 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1703,6 +1703,7 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
 	return ret;
 
 err_unmap:
+	gq->skbs[(gq->cur + nr_desc - 1) % gq->ring_size] = NULL;
 	dma_unmap_single(ndev->dev.parent, dma_addr_orig, skb->len, DMA_TO_DEVICE);
 
 err_kfree:
-- 
2.39.5

On Mon,  2 Dec 2024 18:49:01 +0500 Nikita Yushchenko wrote:
> If error path is taken while filling descriptor for a frame, skb
> pointer is left in the entry. Later, on the ring entry reuse, the
> same entry could be used as a part of a multi-descriptor frame,
> and skb for that new frame could be stored in a different entry.
> 
> Then, the stale pointer will reach the completion routine, and passed
> to the release operation.
> 
> Fix that by clearing the saved skb pointer at the error path.

Why not move the assignment down, then? After we have successfully
mapped all entries?

Coincidentally rswitch_ext_desc_set() calls
rswitch_ext_desc_set_info1() for each desc, potentially timestamping
the same frame multiple times? Isn't that an issue?

I agree with Jake that patches 4 and 5 don't seem like obvious fixes,
would be great if you could post them as separate series, they need to
go to a different tree.
-- 
pw-bot: cr

> Why not move the assignment down, then? After we have successfully
> mapped all entries?

Just realized that moving assignment below the loop will open a race window. DT field is set inside the 
loop, and once it is set, completion interrupt becomes theoretically possible.

Furthermore, realized that existing code already has a race. Interrupt can happen after DT wield was 
updated but before cur is updated. Then, with the completion code won't check the entry (up to a new 
interrupt, that can theoretically not happen).

Will fix in the updated patches.

Nikita

>> If error path is taken while filling descriptor for a frame, skb
>> pointer is left in the entry. Later, on the ring entry reuse, the
>> same entry could be used as a part of a multi-descriptor frame,
>> and skb for that new frame could be stored in a different entry.
>>
>> Then, the stale pointer will reach the completion routine, and passed
>> to the release operation.
>>
>> Fix that by clearing the saved skb pointer at the error path.
> 
> Why not move the assignment down, then? After we have successfully
> mapped all entries?

That is a different possible way to fix the same issue.  Either can be used.

> Coincidentally rswitch_ext_desc_set() calls
> rswitch_ext_desc_set_info1() for each desc, potentially timestamping
> the same frame multiple times? Isn't that an issue?

Somebody familiar with how timestamping works shall comment on this.

> I agree with Jake that patches 4 and 5 don't seem like obvious fixes,
> would be great if you could post them as separate series, they need to
> go to a different tree.

Ok, will repost.

Shall I use [PATCH net] for all?
Or [PATCH] for fixes and [PATCH net] for improvements?

Nikita

On Thu, 5 Dec 2024 08:46:15 +0500 Nikita Yushchenko wrote:
> > I agree with Jake that patches 4 and 5 don't seem like obvious fixes,
> > would be great if you could post them as separate series, they need to
> > go to a different tree.  
> 
> Ok, will repost.
> 
> Shall I use [PATCH net] for all?
> Or [PATCH] for fixes and [PATCH net] for improvements?

Ideally [PATCH net] for fixes, [PATCH net-next] for improvements.
But it's not a big deal as long as they are separated.

On 12/2/2024 5:49 AM, Nikita Yushchenko wrote:
> If error path is taken while filling descriptor for a frame, skb
> pointer is left in the entry. Later, on the ring entry reuse, the
> same entry could be used as a part of a multi-descriptor frame,
> and skb for that new frame could be stored in a different entry.
> 
> Then, the stale pointer will reach the completion routine, and passed
> to the release operation.
> 
> Fix that by clearing the saved skb pointer at the error path.
> 
> Fixes: d2c96b9d5f83 ("net: rswitch: Add jumbo frames handling for TX")
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 32b32aa7e01f..3ad5858d3cdd 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1703,6 +1703,7 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
>  	return ret;
>  
>  err_unmap:
> +	gq->skbs[(gq->cur + nr_desc - 1) % gq->ring_size] = NULL;
>  	dma_unmap_single(ndev->dev.parent, dma_addr_orig, skb->len, DMA_TO_DEVICE);
>  

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  err_kfree: