Re: [PATCH net-next] net: airoha: Reset PPE cpu port configuration in airoha_ppe_hw_init()

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

> On Tue, 17 Mar 2026 17:40:47 +0100 Lorenzo Bianconi wrote:
> > @@ -155,6 +171,11 @@ static void airoha_ppe_hw_init(struct airoha_ppe *ppe)
> >  						 AIROHA_MAX_MTU) |
> >  				      FIELD_PREP(FP1_EGRESS_MTU_MASK,
> >  						 AIROHA_MAX_MTU));
> > +			if (!port)
> > +				continue;
> > +
> > +			airoha_ppe_set_cpu_port(port, i);
> 
> AI says:
> 
> Can this lead to a NULL pointer dereference if a port is not fully
> initialized?
> 
> In airoha_probe(), all GDM ports defined in the device tree are allocated
> and the eth->ports[] array is populated with pointers, but port->qdma is
> left as NULL.
> 
> During airoha_register_gdm_devices(), the ports are registered sequentially
> with register_netdev(). Since register_netdev() drops the rtnl_lock(),
> userspace could react to the RTM_NEWLINK event of the first registered port
> and apply a tc flow offload rule.
> 
> This would trigger the following call chain:
>   .ndo_setup_tc() -> airoha_ppe_setup_tc_block_cb() -> airoha_ppe_offload_setup() 
>    -> airoha_ppe_hw_init()
> 
> If airoha_ppe_hw_init() iterates over the array, it will find the subsequent
> port that has been allocated but not yet registered, meaning its port->qdma
> is still NULL. The call to airoha_ppe_set_cpu_port(port, i) will then
> dereference the NULL port->qdma.
> 
> Would it be better to check if (!port || !port->qdma) before calling
> airoha_ppe_set_cpu_port()?

Hi Jakub,

I agree there is a race between airoha_register_gdm_devices() and
airoha_ppe_setup_tc_block_cb(). Instead of not running
airoha_ppe_set_cpu_port() in airoha_ppe_hw_init() for a given port and not
configuring the hw correctly, I guess it would be better to grab
flow_offload_mutex mutex in airoha_register_gdm_devices routine and wait for
all devices to be properly registered before executing
airoha_ppe_setup_tc_block_cb(). Something like:

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 961325da833b..f5bb917f0f6e 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -2927,21 +2927,24 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
 
 static int airoha_register_gdm_devices(struct airoha_eth *eth)
 {
-	int i;
+	int i, err = 0;
+
+	mutex_lock(&flow_offload_mutex);
 
 	for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
 		struct airoha_gdm_port *port = eth->ports[i];
-		int err;
 
 		if (!port)
 			continue;
 
 		err = register_netdev(port->dev);
 		if (err)
-			return err;
+			break;
 	}
 
-	return 0;
+	mutex_unlock(&flow_offload_mutex);
+
+	return err;
 }
 
 static int airoha_probe(struct platform_device *pdev)
diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index 7df4dbcd8861..3330ee54e20e 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -551,6 +551,8 @@ struct airoha_gdm_port {
 #define AIROHA_RXD4_PPE_CPU_REASON	GENMASK(20, 16)
 #define AIROHA_RXD4_FOE_ENTRY		GENMASK(15, 0)
 
+extern struct mutex flow_offload_mutex;
+
 struct airoha_ppe {
 	struct airoha_ppe_dev dev;
 	struct airoha_eth *eth;
diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
index 7666b1d2f4f6..96d092f7214e 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -15,7 +15,7 @@
 #include "airoha_regs.h"
 #include "airoha_eth.h"
 
-static DEFINE_MUTEX(flow_offload_mutex);
+DEFINE_MUTEX(flow_offload_mutex);
 static DEFINE_SPINLOCK(ppe_lock);
 
 static const struct rhashtable_params airoha_flow_table_params = {


What do you think?
I noticed commit f44218cd5e6a is already applied to net-next. I will post a
follow-up patch, agree?

Regards,
Lorenzo

> -- 
> pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]