[PATCH 0/5] net: renesas: rswitch: several fixes

[PATCH 0/5] net: renesas: rswitch: several fixes

[Nikita Yushchenko]

This series fixes several glitches found in the rswitch driver.

Submitting them first, before new functionality currently being
prepared.

Nikita Yushchenko (5):
  net: renesas: rswitch: fix possible early skb release
  net: renesas: rswitch: fix leaked pointer on error path
  net: renesas: rswitch: avoid use-after-put for a device tree node
  net: renesas: rswitch: do not deinit disabled ports
  net: renesas: rswitch: remove speed from gwca structure

 drivers/net/ethernet/renesas/rswitch.c | 14 +++++++-------
 drivers/net/ethernet/renesas/rswitch.h |  1 -
 2 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.39.5

[PATCH 1/5] net: renesas: rswitch: fix possible early skb release

[Nikita Yushchenko]

When sending frame split into multiple descriptors, hardware processes
descriptors one by one, including writing back DT values. The first
descriptor could be already marked as completed when processing of
next descriptors for the same frame is still in progress.

Although only the last descriptor is configured to generate interrupt,
completion of the first descriptor could be noticed by the driver when
handling interrupt for the previous frame.

Currently, driver stores skb in the entry that corresponds to the first
descriptor. This results into skb could be unmapped and freed when
hardware did not complete the send yet. This opens a window for
corrupting the data being sent.

Fix this by saving skb in the entry that corresponds to the last
descriptor used to send the frame.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index b80aa27a7214..32b32aa7e01f 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1681,8 +1681,9 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
 	if (dma_mapping_error(ndev->dev.parent, dma_addr_orig))
 		goto err_kfree;
 
-	gq->skbs[gq->cur] = skb;
-	gq->unmap_addrs[gq->cur] = dma_addr_orig;
+	/* Stored the skb at the last descriptor to avoid skb free before hardware completes send */
+	gq->skbs[(gq->cur + nr_desc - 1) % gq->ring_size] = skb;
+	gq->unmap_addrs[(gq->cur + nr_desc - 1) % gq->ring_size] = dma_addr_orig;
 
 	/* DT_FSTART should be set at last. So, this is reverse order. */
 	for (i = nr_desc; i-- > 0; ) {
-- 
2.39.5

Re: [PATCH 1/5] net: renesas: rswitch: fix possible early skb release

[Jacob Keller]

On 12/2/2024 5:49 AM, Nikita Yushchenko wrote:
> When sending frame split into multiple descriptors, hardware processes
> descriptors one by one, including writing back DT values. The first
> descriptor could be already marked as completed when processing of
> next descriptors for the same frame is still in progress.
> 
> Although only the last descriptor is configured to generate interrupt,
> completion of the first descriptor could be noticed by the driver when
> handling interrupt for the previous frame.
> 
> Currently, driver stores skb in the entry that corresponds to the first
> descriptor. This results into skb could be unmapped and freed when
> hardware did not complete the send yet. This opens a window for
> corrupting the data being sent.
> 
> Fix this by saving skb in the entry that corresponds to the last
> descriptor used to send the frame.
> 
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index b80aa27a7214..32b32aa7e01f 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1681,8 +1681,9 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
>  	if (dma_mapping_error(ndev->dev.parent, dma_addr_orig))
>  		goto err_kfree;
>  
> -	gq->skbs[gq->cur] = skb;
> -	gq->unmap_addrs[gq->cur] = dma_addr_orig;
> +	/* Stored the skb at the last descriptor to avoid skb free before hardware completes send */
> +	gq->skbs[(gq->cur + nr_desc - 1) % gq->ring_size] = skb;
> +	gq->unmap_addrs[(gq->cur + nr_desc - 1) % gq->ring_size] = dma_addr_orig;
>  

nr_desc is non-zero, so if nr_desc was 1, then this would point to
gq->cur + 0 mod ring_size, i.e. gq->cur.

I might have possibly computed that separately as a local variable since
this expression is repeated twice, but I don't think thats going to do
too much for readability either way.

Ok

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

>  	/* DT_FSTART should be set at last. So, this is reverse order. */
>  	for (i = nr_desc; i-- > 0; ) {

[PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Nikita Yushchenko]

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

Re: [PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Jacob Keller]

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:

Re: [PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Jakub Kicinski]

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

Re: [PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Nikita Yushchenko]

>> 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

Re: [PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Jakub Kicinski]

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.

Re: [PATCH 2/5] net: renesas: rswitch: fix leaked pointer on error path

[Nikita Yushchenko]

> 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

[PATCH 3/5] net: renesas: rswitch: avoid use-after-put for a device tree node

[Nikita Yushchenko]

The device tree node saved in the rswitch_device structure is used at
several driver locations. So passing this node to of_node_put() after
the first use is wrong.

Move of_node_put() for this node to exit paths.

Fixes: b46f1e579329 ("net: renesas: rswitch: Simplify struct phy * handling")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 3ad5858d3cdd..779c05b8e05f 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1891,7 +1891,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
 	rdev->np_port = rswitch_get_port_node(rdev);
 	rdev->disabled = !rdev->np_port;
 	err = of_get_ethdev_address(rdev->np_port, ndev);
-	of_node_put(rdev->np_port);
 	if (err) {
 		if (is_valid_ether_addr(rdev->etha->mac_addr))
 			eth_hw_addr_set(ndev, rdev->etha->mac_addr);
@@ -1921,6 +1920,7 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
 
 out_rxdmac:
 out_get_params:
+	of_node_put(rdev->np_port);
 	netif_napi_del(&rdev->napi);
 	free_netdev(ndev);
 
@@ -1934,6 +1934,7 @@ static void rswitch_device_free(struct rswitch_private *priv, unsigned int index
 
 	rswitch_txdmac_free(ndev);
 	rswitch_rxdmac_free(ndev);
+	of_node_put(rdev->np_port);
 	netif_napi_del(&rdev->napi);
 	free_netdev(ndev);
 }
-- 
2.39.5

Re: [PATCH 3/5] net: renesas: rswitch: avoid use-after-put for a device tree node

[Jacob Keller]

On 12/2/2024 5:49 AM, Nikita Yushchenko wrote:
> The device tree node saved in the rswitch_device structure is used at
> several driver locations. So passing this node to of_node_put() after
> the first use is wrong.
> 
> Move of_node_put() for this node to exit paths.
> 
> Fixes: b46f1e579329 ("net: renesas: rswitch: Simplify struct phy * handling")
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---

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

>  drivers/net/ethernet/renesas/rswitch.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 3ad5858d3cdd..779c05b8e05f 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1891,7 +1891,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
>  	rdev->np_port = rswitch_get_port_node(rdev);
>  	rdev->disabled = !rdev->np_port;
>  	err = of_get_ethdev_address(rdev->np_port, ndev);
> -	of_node_put(rdev->np_port);
>  	if (err) {
>  		if (is_valid_ether_addr(rdev->etha->mac_addr))
>  			eth_hw_addr_set(ndev, rdev->etha->mac_addr);
> @@ -1921,6 +1920,7 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
>  
>  out_rxdmac:
>  out_get_params:
> +	of_node_put(rdev->np_port);
>  	netif_napi_del(&rdev->napi);
>  	free_netdev(ndev);
>  
> @@ -1934,6 +1934,7 @@ static void rswitch_device_free(struct rswitch_private *priv, unsigned int index
>  
>  	rswitch_txdmac_free(ndev);
>  	rswitch_rxdmac_free(ndev);
> +	of_node_put(rdev->np_port);
>  	netif_napi_del(&rdev->napi);
>  	free_netdev(ndev);
>  }

[PATCH 4/5] net: renesas: rswitch: do not deinit disabled ports

[Nikita Yushchenko]

In rswitch_ether_port_init_all(), only enabled ports are initialized.
Then, rswitch_ether_port_deinit_all() shall also only deinitialize
enabled ports.

Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 779c05b8e05f..5980084d9211 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1527,7 +1527,7 @@ static void rswitch_ether_port_deinit_all(struct rswitch_private *priv)
 {
 	unsigned int i;
 
-	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
+	rswitch_for_each_enabled_port(priv, i) {
 		phy_exit(priv->rdev[i]->serdes);
 		rswitch_ether_port_deinit_one(priv->rdev[i]);
 	}
-- 
2.39.5

Re: [PATCH 4/5] net: renesas: rswitch: do not deinit disabled ports

[Jacob Keller]

On 12/2/2024 5:49 AM, Nikita Yushchenko wrote:
> In rswitch_ether_port_init_all(), only enabled ports are initialized.
> Then, rswitch_ether_port_deinit_all() shall also only deinitialize
> enabled ports.
> 
> Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---

Do you happen to have any data on whether this causes any visible issues?

At a glance of the code, rswitch_ether_port_deinit_one appears to check
various pointers before doing anything and skips any real work if those
pointers aren't initialized.

Either way, this seems like a good cleanup, though it might not warrant
a fixes or net target if there is no user visible bug associated with it.

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

>  drivers/net/ethernet/renesas/rswitch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 779c05b8e05f..5980084d9211 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1527,7 +1527,7 @@ static void rswitch_ether_port_deinit_all(struct rswitch_private *priv)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
> +	rswitch_for_each_enabled_port(priv, i) {
>  		phy_exit(priv->rdev[i]->serdes);
>  		rswitch_ether_port_deinit_one(priv->rdev[i]);
>  	}

[PATCH 5/5] net: renesas: rswitch: remove speed from gwca structure

[Nikita Yushchenko]

This field is set but never used.

GWCA is rswitch CPU interface module which connects rswitch to the
host over AXI bus. Speed of the switch ports is not anyhow related to
GWCA operation.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 3 ---
 drivers/net/ethernet/renesas/rswitch.h | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 5980084d9211..bef344e0b1fd 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1902,9 +1902,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
 	if (err < 0)
 		goto out_get_params;
 
-	if (rdev->priv->gwca.speed < rdev->etha->speed)
-		rdev->priv->gwca.speed = rdev->etha->speed;
-
 	err = rswitch_rxdmac_alloc(ndev);
 	if (err < 0)
 		goto out_rxdmac;
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 72e3ff596d31..303883369b94 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -993,7 +993,6 @@ struct rswitch_gwca {
 	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
 	u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
 	u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
-	int speed;
 };
 
 #define NUM_QUEUES_PER_NDEV	2
-- 
2.39.5

Re: [PATCH 5/5] net: renesas: rswitch: remove speed from gwca structure

[Jacob Keller]

On 12/2/2024 5:49 AM, Nikita Yushchenko wrote:
> This field is set but never used.
> 
> GWCA is rswitch CPU interface module which connects rswitch to the
> host over AXI bus. Speed of the switch ports is not anyhow related to
> GWCA operation.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---

I tried grepping the code and didn't find any other users either.

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

This one also could be a cleanup that goes to next since there doesn't
appear to be any user visible bug.

>  drivers/net/ethernet/renesas/rswitch.c | 3 ---
>  drivers/net/ethernet/renesas/rswitch.h | 1 -
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 5980084d9211..bef344e0b1fd 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1902,9 +1902,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, unsigned int index
>  	if (err < 0)
>  		goto out_get_params;
>  
> -	if (rdev->priv->gwca.speed < rdev->etha->speed)
> -		rdev->priv->gwca.speed = rdev->etha->speed;
> -
>  	err = rswitch_rxdmac_alloc(ndev);
>  	if (err < 0)
>  		goto out_rxdmac;
> diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
> index 72e3ff596d31..303883369b94 100644
> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h
> @@ -993,7 +993,6 @@ struct rswitch_gwca {
>  	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
>  	u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
>  	u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> -	int speed;
>  };
>  
>  #define NUM_QUEUES_PER_NDEV	2

Re: [PATCH 0/5] net: renesas: rswitch: several fixes

[Jacob Keller]

On 12/2/2024 5:48 AM, Nikita Yushchenko wrote:
> This series fixes several glitches found in the rswitch driver.
> 
> Submitting them first, before new functionality currently being
> prepared.
> 

The series lacks a net or net-next prefix to indicate the target tree.
Based on the subject titles, this appears to be a set of fixes that
could target net.

Please add the target tree prefix if you send this series again, or with
other series in the future.

Thanks,
Jake