[PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Jerry Wu]

Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
and is susceptible to the same crash.

This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
it uses the DSA framework which registers all ports.

Fix this by checking if the port pointer is valid before accessing it.

Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 08bee56aea35..6f917fd7af4d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 	/* Now, set PGIDs for each active LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-		struct net_device *bond = ocelot->ports[lag]->bond;
+		struct ocelot_port *ocelot_port = ocelot->ports[lag];
 		int num_active_ports = 0;
+		struct net_device *bond;
 		unsigned long bond_mask;
 		u8 aggr_idx[16];
 
-		if (!bond || (visited & BIT(lag)))
+		if (!port || !port->bond || (visited & BIT(lag)))
 			continue;
 
+		bond = port->bond;
 		bond_mask = ocelot_get_bond_mask(ocelot, bond);
 
 		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
-- 
2.52.0

[PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Jerry Wu]

Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
and is susceptible to the same crash.

This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
it uses the DSA framework which registers all ports.

Fix this by checking if the port pointer is valid before accessing it.

Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 08bee56aea35..6f917fd7af4d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 	/* Now, set PGIDs for each active LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-		struct net_device *bond = ocelot->ports[lag]->bond;
+		struct ocelot_port *ocelot_port = ocelot->ports[lag];
 		int num_active_ports = 0;
+		struct net_device *bond;
 		unsigned long bond_mask;
 		u8 aggr_idx[16];
 
-		if (!bond || (visited & BIT(lag)))
+		if (!ocelot_port || !ocelot_port->bond || (visited & BIT(lag)))
 			continue;
 
+		bond = ocelot_port->bond;
 		bond_mask = ocelot_get_bond_mask(ocelot, bond);
 
 		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
-- 
2.52.0

Re: [PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Vladimir Oltean]

Hi Jerry,

On Sat, Dec 20, 2025 at 07:01:23PM +0000, Jerry Wu wrote:
> Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
> fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
> The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
> and is susceptible to the same crash.
> 
> This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
> unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
> it uses the DSA framework which registers all ports.
> 
> Fix this by checking if the port pointer is valid before accessing it.
> 
> Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
> Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
> ---

The 4th item in maintainer-netdev.rst is "don't repost your patches
within one 24h period". This would have given me more than 4 minutes
between your v2 and... v2 (?!) to leave extra comments.

The area below "---" in the patch is discarded when applying the patch.
It is recommended that you use it for patch change information between
versions. You copied a bunch of new people in v2 which have no reference
to v1. Find your patches on https://lore.kernel.org/netdev/ and
https://lore.kernel.org/lkml/ and reference them, and explain the
changes you've made.

>  drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 08bee56aea35..6f917fd7af4d 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  
>  	/* Now, set PGIDs for each active LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -		struct net_device *bond = ocelot->ports[lag]->bond;
> +		struct ocelot_port *ocelot_port = ocelot->ports[lag];
>  		int num_active_ports = 0;
> +		struct net_device *bond;
>  		unsigned long bond_mask;
>  		u8 aggr_idx[16];
>  
> -		if (!bond || (visited & BIT(lag)))
> +		if (!ocelot_port || !ocelot_port->bond || (visited & BIT(lag)))
>  			continue;
>  
> +		bond = ocelot_port->bond;
>  		bond_mask = ocelot_get_bond_mask(ocelot, bond);

Because the "bond" variable is used only once, I had a review comment in
v1 to delete it, and leave the code with just this:

		bond_mask = ocelot_get_bond_mask(ocelot, ocelot_port->bond);

You didn't leave any reason for disregarding this element of the feedback.

>  
>  		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
> -- 
> 2.52.0
>

Re: [PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Christophe JAILLET]

Le 20/12/2025 à 19:56, Jerry Wu a écrit :
> Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
> fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
> The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
> and is susceptible to the same crash.
> 
> This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
> unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
> it uses the DSA framework which registers all ports.
> 
> Fix this by checking if the port pointer is valid before accessing it.
> 
> Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
> Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
> ---
>   drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 08bee56aea35..6f917fd7af4d 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>   
>   	/* Now, set PGIDs for each active LAG */
>   	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -		struct net_device *bond = ocelot->ports[lag]->bond;
> +		struct ocelot_port *ocelot_port = ocelot->ports[lag];

Hi,

Does it compile?

Should this be struct ocelot_port *port = ocelot->ports[lag]; ?

CJ

>   		int num_active_ports = 0;
> +		struct net_device *bond;
>   		unsigned long bond_mask;
>   		u8 aggr_idx[16];
>   
> -		if (!bond || (visited & BIT(lag)))
> +		if (!port || !port->bond || (visited & BIT(lag)))
>   			continue;
>   
> +		bond = port->bond;
>   		bond_mask = ocelot_get_bond_mask(ocelot, bond);
>   
>   		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {

Re: [PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Christophe JAILLET]

Le 20/12/2025 à 20:01, Jerry Wu a écrit :
> Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
> fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
> The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
> and is susceptible to the same crash.
> 
> This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
> unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
> it uses the DSA framework which registers all ports.
> 
> Fix this by checking if the port pointer is valid before accessing it.
> 
> Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
> Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
> ---
>   drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 08bee56aea35..6f917fd7af4d 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>   
>   	/* Now, set PGIDs for each active LAG */
>   	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -		struct net_device *bond = ocelot->ports[lag]->bond;
> +		struct ocelot_port *ocelot_port = ocelot->ports[lag];
>   		int num_active_ports = 0;
> +		struct net_device *bond;
>   		unsigned long bond_mask;
>   		u8 aggr_idx[16];
>   
> -		if (!bond || (visited & BIT(lag)))
> +		if (!ocelot_port || !ocelot_port->bond || (visited & BIT(lag)))

ok, but it would be clearer if this was a v3 of the patch.

It is also usually welcomed to explain what has changed between versions 
below the ---.

CJ

>   			continue;
>   
> +		bond = ocelot_port->bond;
>   		bond_mask = ocelot_get_bond_mask(ocelot, bond);
>   
>   		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {

Re: [PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Jerry Wu]

Dear Linux Kernel communities,

On Sat, Dec 20, 2025 at 20:00 UTC Vladimir Oltean Wrote
> The 4th item in maintainer-netdev.rst is "don't repost your patches
> within one 24h period". This would have given me more than 4 minutes
between your v2 and... v2 (?!) to leave extra comments.

> The area below "---" in the patch is discarded when applying the patch.
> It is recommended that you use it for patch change information between
> versions. You copied a bunch of new people in v2 which have no reference
> to v1. Find your patches on https://lore.kernel.org/netdev/ and
> https://lore.kernel.org/lkml/ and reference them, and explain the
> changes you've made.

Thank you for your kind suggestion. I'll learn to leverage it in my future
contribution. And I want to explain that the repeated patch was sent due
to some network issues as I thought in first email failed. The latest
patch is the correct one.

The context link is
https://lore.kernel.org/netdev/20251220180113.724txltmrkxzyaql@skbuf/T/

> Because the "bond" variable is used only once, I had a review comment in
> v1 to delete it, and leave the code with just this:

> bond_mask = ocelot_get_bond_mask(ocelot, ocelot_port->bond);

> You didn't leave any reason for disregarding this element of the feedback.

Sorry for the missing. I reserved the `bond` variable as near line 2355

>		for (port = lag; port < ocelot->num_phys_ports; port++) {
>			struct ocelot_port *ocelot_port = ocelot->ports[port];
>
>			if (!ocelot_port)
>				continue;
>
>			if (ocelot_port->bond == bond)
>				visited |= BIT(port);
>		}

I noticed that the bond variable would be used again so reserved it.
Sorry again for any inconvenience caused. If there is any information
needed or improper contribution practice from me please let me know as
I also found some other issues, being preparing to continue reporting.

Re: [PATCH net v2] net: mscc: ocelot: Fix crash when adding interface under a lag

[Vladimir Oltean]

On Sat, Dec 20, 2025 at 08:36:14PM +0000, Jerry Wu wrote:
> Dear Linux Kernel communities,
> 
> On Sat, Dec 20, 2025 at 20:00 UTC Vladimir Oltean Wrote
> > The 4th item in maintainer-netdev.rst is "don't repost your patches
> > within one 24h period". This would have given me more than 4 minutes
> between your v2 and... v2 (?!) to leave extra comments.
> 
> > The area below "---" in the patch is discarded when applying the patch.
> > It is recommended that you use it for patch change information between
> > versions. You copied a bunch of new people in v2 which have no reference
> > to v1. Find your patches on https://lore.kernel.org/netdev/ and
> > https://lore.kernel.org/lkml/ and reference them, and explain the
> > changes you've made.
> 
> Thank you for your kind suggestion. I'll learn to leverage it in my future
> contribution. And I want to explain that the repeated patch was sent due
> to some network issues as I thought in first email failed. The latest
> patch is the correct one.

Ok, but the content of the two v2 patches is not identical. One compiles,
and the other doesn't. Any change to a patch should constitute a new
version.

> 
> The context link is
> https://lore.kernel.org/netdev/20251220180113.724txltmrkxzyaql@skbuf/T/

You should provide the link to your own code submission and not to a reply, i.e.
https://lore.kernel.org/lkml/tencent_9E2B81D645D04DFE191C86F128212F842B05@qq.com/
In this case, this is not just a pedantic comment, because you didn't
post v1 to netdev, so your patch is [not found] when searched from that
lore instance rather than from lkml.

> 
> > Because the "bond" variable is used only once, I had a review comment in
> > v1 to delete it, and leave the code with just this:
> 
> > bond_mask = ocelot_get_bond_mask(ocelot, ocelot_port->bond);
> 
> > You didn't leave any reason for disregarding this element of the feedback.
> 
> Sorry for the missing. I reserved the `bond` variable as near line 2355
> 
> >		for (port = lag; port < ocelot->num_phys_ports; port++) {
> >			struct ocelot_port *ocelot_port = ocelot->ports[port];
> >
> >			if (!ocelot_port)
> >				continue;
> >
> >			if (ocelot_port->bond == bond)
> >				visited |= BIT(port);
> >		}
> 
> I noticed that the bond variable would be used again so reserved it.
> Sorry again for any inconvenience caused. If there is any information
> needed or improper contribution practice from me please let me know as
> I also found some other issues, being preparing to continue reporting.

Ok, so the reason is that "bond" is not used just once as I thought. It
is used one more time here:

		/* Mark all ports in the same LAG as visited to avoid applying
		 * the same config again.
		 */
		for (port = lag; port < ocelot->num_phys_ports; port++) {
			struct ocelot_port *ocelot_port = ocelot->ports[port];

			if (!ocelot_port)
				continue;

			if (ocelot_port->bond == bond)
						 ~~~~
				visited |= BIT(port);
		}

In that case yes, please disregard this comment, we need the variable saved.

After the 24 hour cool-off period, can you please resend the proper variant
of v2 as a new v3 with the change log added? To avoid the situation where
the wrong patch is applied.

[PATCH net v3] net: mscc: ocelot: Fix crash when adding interface under a lag

[Jerry Wu]

Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
and is susceptible to the same crash.

This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
it uses the DSA framework which registers all ports.

Fix this by checking if the port pointer is valid before accessing it.

Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
---
v3:
  - Resending the correct version that compiles properly.
  - Retained the 'bond' variable as confirmed necessary in discussion with Vladimir Oltean.
  - Added links to previous versions as requested.
  - Thanks to the reviewers for the gentle and detailed feedback and guidance.

v2: https://lore.kernel.org/netdev/20251220210808.325isrbvmhjp3tlg@skbuf/T/
  - Addressed comments from v1 regarding variable name and null checking.
  - Sent by accident as a draft.

v1: https://lore.kernel.org/lkml/20251220180113.724txltmrkxzyaql@skbuf/T/
  - Try to fix the crash in the same way as previous patch did but failed for
    - improper variable name that is shadowing the "int port" definition.
	- unnecessary and incorrect hunk for null checking.
	- improper commit commit title.

 drivers/net/ethernet/mscc/ocelot.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 08bee56aea35..c345d9b17c89 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2307,14 +2307,16 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 	/* Now, set PGIDs for each active LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-		struct net_device *bond = ocelot->ports[lag]->bond;
+		struct ocelot_port *ocelot_port = ocelot->ports[lag];
 		int num_active_ports = 0;
+		struct net_device *bond;
 		unsigned long bond_mask;
 		u8 aggr_idx[16];
 
-		if (!bond || (visited & BIT(lag)))
+		if (!ocelot_port || !ocelot_port->bond || (visited & BIT(lag)))
 			continue;
 
+		bond = ocelot_port->bond;
 		bond_mask = ocelot_get_bond_mask(ocelot, bond);
 
 		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
-- 
2.52.0

Re: [PATCH net v3] net: mscc: ocelot: Fix crash when adding interface under a lag

[Vladimir Oltean]

On Thu, Dec 25, 2025 at 08:36:17PM +0000, Jerry Wu wrote:
> Commit 15faa1f67ab4 ("lan966x: Fix crash when adding interface under a lag")
> fixed a similar issue in the lan966x driver caused by a NULL pointer dereference.
> The ocelot_set_aggr_pgids() function in the ocelot driver has similar logic
> and is susceptible to the same crash.
> 
> This issue specifically affects the ocelot_vsc7514.c frontend, which leaves
> unused ports as NULL pointers. The felix_vsc9959.c frontend is unaffected as
> it uses the DSA framework which registers all ports.
> 
> Fix this by checking if the port pointer is valid before accessing it.
> 
> Fixes: 528d3f190c98 ("net: mscc: ocelot: drop the use of the "lags" array")
> Signed-off-by: Jerry Wu <w.7erry@foxmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thanks!