| Message ID | 20241202134904.3882317-3-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: renesas: rswitch: several fixes | expand |
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:
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.
>> 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.
> 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
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:
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(+)