[PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[Andrew Lunn]

A port is not necessarily assigned to a netdev. And a port does not
need to be a member of a bridge. So when iterating over all ports,
check before using the netdev and bridge_dev for a port. Otherwise we
dereference a NULL pointer.

Fixes: da9c359e19f0 ("net: dsa: mv88e6xxx: check hardware VLAN in use")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 883fd9809dd2..4e5ab856b87a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1796,6 +1796,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
 				continue;
 
+			if (!ds->ports[port].netdev)
+				continue;
+
 			if (vlan.data[i] ==
 			    GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
 				continue;
@@ -1804,6 +1807,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 			    chip->ports[port].bridge_dev)
 				break; /* same bridge, check next VLAN */
 
+			if (!chip->ports[i].bridge_dev)
+				continue;
+
 			netdev_warn(ds->ports[port].netdev,
 				    "hardware VLAN %d already used by %s\n",
 				    vlan.vid,
-- 
2.10.2

Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> @@ -1804,6 +1807,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>  			    chip->ports[port].bridge_dev)
>  				break; /* same bridge, check next VLAN */
>  
> +			if (!chip->ports[i].bridge_dev)
> +				continue;
> +

The above truncated test:

			if (chip->ports[i].bridge_dev ==
			    chip->ports[port].bridge_dev)
				break; /* same bridge, check next VLAN */

should handle the case where bridge_dev is NULL, but if you want to
explicitly test it, I'd move it before this statement.

>  			netdev_warn(ds->ports[port].netdev,
>  				    "hardware VLAN %d already used by %s\n",
>  				    vlan.vid,


Thanks,

        Vivien

Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[Andrew Lunn]

On Sun, Dec 11, 2016 at 04:02:37PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > @@ -1804,6 +1807,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
> >  			    chip->ports[port].bridge_dev)
> >  				break; /* same bridge, check next VLAN */
> >  
> > +			if (!chip->ports[i].bridge_dev)
> > +				continue;
> > +
> 
> The above truncated test:
> 
> 			if (chip->ports[i].bridge_dev ==
> 			    chip->ports[port].bridge_dev)
> 				break; /* same bridge, check next VLAN */
> 
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.
> 
> >  			netdev_warn(ds->ports[port].netdev,
> >  				    "hardware VLAN %d already used by %s\n",
> >  				    vlan.vid,

Hi Vivien

I don't think you comment is correct. Here is the loop, with my two
additions.

                for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
                        if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
                                continue;

                        if (!ds->ports[port].netdev)
                                continue;

                        if (vlan.data[i] ==
                            GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
                                continue;

                        if (chip->ports[i].bridge_dev ==
                            chip->ports[port].bridge_dev)
                                break; /* same bridge, check next VLAN */

                        if (!chip->ports[i].bridge_dev)
                                continue;

                        netdev_warn(ds->ports[port].netdev,
                                    "hardware VLAN %d already used by %s\n",
                                    vlan.vid,
                                    netdev_name(chip->ports[i].bridge_dev));
                        err = -EOPNOTSUPP;
                        goto unlock;
                }

The opps was occurring in netdev_name(). I did not check which
one. The obvious one, or the netdev_warn(netdev,... one.

> The above truncated test:
> 
> 			if (chip->ports[i].bridge_dev ==
> 			    chip->ports[port].bridge_dev)
> 				break; /* same bridge, check next VLAN */
>
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.

This will not stop chip->ports[i].bridge_dev == NULL from reaching the
netdev_warn().

	Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> A port is not necessarily assigned to a netdev. And a port does not
> need to be a member of a bridge. So when iterating over all ports,
> check before using the netdev and bridge_dev for a port. Otherwise we
> dereference a NULL pointer.
>
> Fixes: da9c359e19f0 ("net: dsa: mv88e6xxx: check hardware VLAN in use")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Sun, 11 Dec 2016 21:07:19 +0100

> A port is not necessarily assigned to a netdev. And a port does not
> need to be a member of a bridge. So when iterating over all ports,
> check before using the netdev and bridge_dev for a port. Otherwise we
> dereference a NULL pointer.
> 
> Fixes: da9c359e19f0 ("net: dsa: mv88e6xxx: check hardware VLAN in use")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Applied.