| Message ID | 1481486839-19502-1-git-send-email-andrew@lunn.ch |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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.
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,
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(+)