Skip to contentSkip to conversation

switchdev: fix: erasing too much of vlan obj when handling multiple vlan specs

PATCH Applied [email protected] 2 participants 7 messages Oct 23, 2015

Subsystems (7)
Networking Network drivers Networking DSA Ethernet bridge +3 more subsystems

v3 of 4 Applied Latest is v4 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH v3 1/3 switchdev: fix: erasing too much of vlan obj when handling multiple vlan specs

From: Scott Feldman <[email protected]>

When adding vlans with multiple IFLA_BRIDGE_VLAN_INFO attrs set in AFSPEC, we would wipe the vlan obj struct after the first IFLA_BRIDGE_VLAN_INFO. Fix this by only clearing what's necessary on each IFLA_BRIDGE_VLAN_INFO iteration.

Fixes 9e8f4a54 ("switchdev: push object ID back to object structure")
Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

---
v2->v3: no change
v1->v2: add Jiri's Acked-by

 net/switchdev/switchdev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/switchdev/switchdev.c +2 -2
index 73e3895..56d8479 100644
@@ -863,7 +863,7 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 			err = f(dev, &vlan.obj);
 			if (err)
 				return err;
-			memset(&vlan, 0, sizeof(vlan));
+			vlan.vid_begin = 0;
 		} else {
 			if (vlan.vid_begin)
 				return -EINVAL;

@@ -872,7 +872,7 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 			err = f(dev, &vlan.obj);
 			if (err)
 				return err;
-			memset(&vlan, 0, sizeof(vlan));
+			vlan.vid_begin = 0;
 		}
 	}
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH v3 2/3 switchdev: fix: pass correct obj size when deferring obj add

From: Scott Feldman <[email protected]>

Fixes 4d429c5dd ("switchdev: introduce possibility to defer obj_add/del")
Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

---
v2->v3: add Jiri's Acked-by
v1->v2: use correct "Fixes" tag, use common func to calc obj size for add/del

 net/switchdev/switchdev.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)
net/switchdev/switchdev.c +17 -2
index 56d8479..bff8e2b 100644
@@ -336,6 +336,21 @@ int switchdev_port_attr_set(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_attr_set);
 
+static size_t switchdev_obj_size(const struct switchdev_obj *obj)
+{
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_ID_PORT_VLAN:
+		return sizeof(struct switchdev_obj_port_vlan);
+	case SWITCHDEV_OBJ_ID_IPV4_FIB:
+		return sizeof(struct switchdev_obj_ipv4_fib);
+	case SWITCHDEV_OBJ_ID_PORT_FDB:
+		return sizeof(struct switchdev_obj_port_fdb);
+	default:
+		BUG();
+	}
+	return 0;
+}
+
 static int __switchdev_port_obj_add(struct net_device *dev,
 				    const struct switchdev_obj *obj,
 				    struct switchdev_trans *trans)

@@ -421,7 +436,7 @@ static void switchdev_port_obj_add_deferred(struct net_device *dev,
 static int switchdev_port_obj_add_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
-	return switchdev_deferred_enqueue(dev, obj, sizeof(*obj),
+	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
 					  switchdev_port_obj_add_deferred);
 }
 

@@ -489,7 +504,7 @@ static void switchdev_port_obj_del_deferred(struct net_device *dev,
 static int switchdev_port_obj_del_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
-	return switchdev_deferred_enqueue(dev, obj, sizeof(*obj),
+	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
 					  switchdev_port_obj_del_deferred);
 }
Signature · 1 line
1.7.10.4
author
#3 lore raw

PATCH v3 3/3 switchdev: split switchdev_attr into individual structs

In reply to [email protected]

From: Scott Feldman <[email protected]>

This was already done for switchdev_objs. Changing switchdev_attrs to new
style makes switchdev API consistent for both attrs and objs.

No functional changes here.

Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

---
v2->v3: remove something that slipped in from the future. I got a piece of another patchset mixed in with this one. Oops.

v1->v2: add Jiri's Acked-by

 .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |   24 ++++--
 drivers/net/ethernet/mellanox/mlxsw/switchx2.c     |    7 +-
 drivers/net/ethernet/rocker/rocker.c               |   23 ++++--
 include/net/switchdev.h                            |   42 +++++++++--
 net/bridge/br_stp.c                                |   24 +++---
 net/core/net-sysfs.c                               |   14 ++--
 net/core/rtnetlink.c                               |   14 ++--
 net/dsa/slave.c                                    |   10 ++-
 net/switchdev/switchdev.c                          |   77 +++++++++++++-------
 9 files changed, 163 insertions(+), 72 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +17 -7
index c39b7a1..efa1aa8 100644
@@ -56,15 +56,19 @@ static int mlxsw_sp_port_attr_get(struct net_device *dev,
 {
 	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
 	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+	struct switchdev_attr_port_parent_id *parent_id;
+	struct switchdev_attr_port_bridge_flags *brport_flags;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
-		attr->u.ppid.id_len = sizeof(mlxsw_sp->base_mac);
-		memcpy(&attr->u.ppid.id, &mlxsw_sp->base_mac,
-		       attr->u.ppid.id_len);
+		parent_id = SWITCHDEV_ATTR_PORT_PARENT_ID(attr);
+		parent_id->ppid.id_len = sizeof(mlxsw_sp->base_mac);
+		memcpy(&parent_id->ppid.id, &mlxsw_sp->base_mac,
+		       parent_id->ppid.id_len);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
-		attr->u.brport_flags =
+		brport_flags = SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS(attr);
+		brport_flags->brport_flags =
 			(mlxsw_sp_port->learning ? BR_LEARNING : 0) |
 			(mlxsw_sp_port->learning_sync ? BR_LEARNING_SYNC : 0);
 		break;

@@ -166,20 +170,26 @@ static int mlxsw_sp_port_attr_set(struct net_device *dev,
 				  struct switchdev_trans *trans)
 {
 	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+	struct switchdev_attr_port_stp_state *stp_state;
+	struct switchdev_attr_port_bridge_flags *brport_flags;
+	struct switchdev_attr_bridge_ageing_time *ageing_time;
 	int err = 0;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		stp_state = SWITCHDEV_ATTR_PORT_STP_STATE(attr);
 		err = mlxsw_sp_port_attr_stp_state_set(mlxsw_sp_port, trans,
-						       attr->u.stp_state);
+						       stp_state->state);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
+		brport_flags = SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS(attr);
 		err = mlxsw_sp_port_attr_br_flags_set(mlxsw_sp_port, trans,
-						      attr->u.brport_flags);
+						      brport_flags->brport_flags);
 		break;
 	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
+		ageing_time = SWITCHDEV_ATTR_BRIDGE_AGEING_TIME(attr);
 		err = mlxsw_sp_port_attr_br_ageing_set(mlxsw_sp_port, trans,
-						       attr->u.ageing_time);
+						       ageing_time->ageing_time);
 		break;
 	default:
 		err = -EOPNOTSUPP;
drivers/net/ethernet/mellanox/mlxsw/switchx2.c +5 -2
index 2fd2279..edabc82 100644
@@ -864,11 +864,14 @@ static int mlxsw_sx_port_attr_get(struct net_device *dev,
 {
 	struct mlxsw_sx_port *mlxsw_sx_port = netdev_priv(dev);
 	struct mlxsw_sx *mlxsw_sx = mlxsw_sx_port->mlxsw_sx;
+	struct switchdev_attr_port_parent_id *parent_id;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
-		attr->u.ppid.id_len = sizeof(mlxsw_sx->hw_id);
-		memcpy(&attr->u.ppid.id, &mlxsw_sx->hw_id, attr->u.ppid.id_len);
+		parent_id = SWITCHDEV_ATTR_PORT_PARENT_ID(attr);
+		parent_id->ppid.id_len = sizeof(mlxsw_sx->hw_id);
+		memcpy(&parent_id->ppid.id, &mlxsw_sx->hw_id,
+		       parent_id->ppid.id_len);
 		break;
 	default:
 		return -EOPNOTSUPP;
drivers/net/ethernet/rocker/rocker.c +17 -6
index 32a80d2..512dc51 100644
@@ -4327,14 +4327,19 @@ static int rocker_port_attr_get(struct net_device *dev,
 {
 	const struct rocker_port *rocker_port = netdev_priv(dev);
 	const struct rocker *rocker = rocker_port->rocker;
+	struct switchdev_attr_port_parent_id *parent_id;
+	struct switchdev_attr_port_bridge_flags *brport_flags;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
-		attr->u.ppid.id_len = sizeof(rocker->hw.id);
-		memcpy(&attr->u.ppid.id, &rocker->hw.id, attr->u.ppid.id_len);
+		parent_id = SWITCHDEV_ATTR_PORT_PARENT_ID(attr);
+		parent_id->ppid.id_len = sizeof(rocker->hw.id);
+		memcpy(&parent_id->ppid.id, &rocker->hw.id,
+		       parent_id->ppid.id_len);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
-		attr->u.brport_flags = rocker_port->brport_flags;
+		brport_flags = SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS(attr);
+		brport_flags->brport_flags = rocker_port->brport_flags;
 		break;
 	default:
 		return -EOPNOTSUPP;

@@ -4378,20 +4383,26 @@ static int rocker_port_attr_set(struct net_device *dev,
 				struct switchdev_trans *trans)
 {
 	struct rocker_port *rocker_port = netdev_priv(dev);
+	struct switchdev_attr_port_stp_state *stp_state;
+	struct switchdev_attr_port_bridge_flags *brport_flags;
+	struct switchdev_attr_bridge_ageing_time *ageing_time;
 	int err = 0;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		stp_state = SWITCHDEV_ATTR_PORT_STP_STATE(attr);
 		err = rocker_port_stp_update(rocker_port, trans, 0,
-					     attr->u.stp_state);
+					     stp_state->state);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
+		brport_flags = SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS(attr);
 		err = rocker_port_brport_flags_set(rocker_port, trans,
-						   attr->u.brport_flags);
+						   brport_flags->brport_flags);
 		break;
 	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
+		ageing_time = SWITCHDEV_ATTR_BRIDGE_AGEING_TIME(attr);
 		err = rocker_port_bridge_ageing_time(rocker_port, trans,
-						     attr->u.ageing_time);
+						     ageing_time->ageing_time);
 		break;
 	default:
 		err = -EOPNOTSUPP;
include/net/switchdev.h +36 -6

50 lines not on this page · raw message

net/bridge/br_stp.c +14 -10
index 80c34d7..50bc679 100644
@@ -39,15 +39,17 @@ void br_log_state(const struct net_bridge_port *p)
 
 void br_set_state(struct net_bridge_port *p, unsigned int state)
 {
-	struct switchdev_attr attr = {
-		.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
-		.flags = SWITCHDEV_F_DEFER,
-		.u.stp_state = state,
+	struct switchdev_attr_port_stp_state attr = {
+		.attr = {
+			.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
+			.flags = SWITCHDEV_F_DEFER,
+		},
+		.state = state,
 	};
 	int err;
 
 	p->state = state;
-	err = switchdev_port_attr_set(p->dev, &attr);
+	err = switchdev_port_attr_set(p->dev, &attr.attr);
 	if (err)
 		br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
 				(unsigned int) p->port_no, p->dev->name);

@@ -569,10 +571,12 @@ int br_set_max_age(struct net_bridge *br, unsigned long val)
 
 int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
 {
-	struct switchdev_attr attr = {
-		.id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
-		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
-		.u.ageing_time = ageing_time,
+	struct switchdev_attr_bridge_ageing_time attr = {
+		.attr = {
+			.id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
+			.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
+		},
+		.ageing_time = ageing_time,
 	};
 	unsigned long t = clock_t_to_jiffies(ageing_time);
 	int err;

@@ -580,7 +584,7 @@ int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
 	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
 		return -ERANGE;
 
-	err = switchdev_port_attr_set(br->dev, &attr);
+	err = switchdev_port_attr_set(br->dev, &attr.attr);
 	if (err)
 		return err;
net/core/net-sysfs.c +8 -6
index f88a62a..1ee6d44 100644
@@ -470,15 +470,17 @@ static ssize_t phys_switch_id_show(struct device *dev,
 		return restart_syscall();
 
 	if (dev_isalive(netdev)) {
-		struct switchdev_attr attr = {
-			.id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
-			.flags = SWITCHDEV_F_NO_RECURSE,
+		struct switchdev_attr_port_parent_id parent_id = {
+			.attr = {
+				.id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
+				.flags = SWITCHDEV_F_NO_RECURSE,
+			},
 		};
 
-		ret = switchdev_port_attr_get(netdev, &attr);
+		ret = switchdev_port_attr_get(netdev, &parent_id.attr);
 		if (!ret)
-			ret = sprintf(buf, "%*phN\n", attr.u.ppid.id_len,
-				      attr.u.ppid.id);
+			ret = sprintf(buf, "%*phN\n", parent_id.ppid.id_len,
+				      parent_id.ppid.id);
 	}
 	rtnl_unlock();
net/core/rtnetlink.c +8 -6
index 2477595..656e7a5 100644
@@ -1024,20 +1024,22 @@ static int rtnl_phys_port_name_fill(struct sk_buff *skb, struct net_device *dev)
 static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
 {
 	int err;
-	struct switchdev_attr attr = {
-		.id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
-		.flags = SWITCHDEV_F_NO_RECURSE,
+	struct switchdev_attr_port_parent_id parent_id = {
+		.attr = {
+			.id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
+			.flags = SWITCHDEV_F_NO_RECURSE,
+		},
 	};
 
-	err = switchdev_port_attr_get(dev, &attr);
+	err = switchdev_port_attr_get(dev, &parent_id.attr);
 	if (err) {
 		if (err == -EOPNOTSUPP)
 			return 0;
 		return err;
 	}
 
-	if (nla_put(skb, IFLA_PHYS_SWITCH_ID, attr.u.ppid.id_len,
-		    attr.u.ppid.id))
+	if (nla_put(skb, IFLA_PHYS_SWITCH_ID, parent_id.ppid.id_len,
+		    parent_id.ppid.id))
 		return -EMSGSIZE;
 
 	return 0;
net/dsa/slave.c +7 -3
index b0b8da0..e41803f 100644
@@ -458,15 +458,17 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
+	struct switchdev_attr_port_stp_state *stp_state;
 	int ret;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		stp_state = SWITCHDEV_ATTR_PORT_STP_STATE(attr);
 		if (switchdev_trans_ph_prepare(trans))
 			ret = ds->drv->port_stp_update ? 0 : -EOPNOTSUPP;
 		else
 			ret = ds->drv->port_stp_update(ds, p->port,
-						       attr->u.stp_state);
+						       stp_state->state);
 		break;
 	default:
 		ret = -EOPNOTSUPP;

@@ -595,11 +597,13 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
+	struct switchdev_attr_port_parent_id *parent_id;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
-		attr->u.ppid.id_len = sizeof(ds->index);
-		memcpy(&attr->u.ppid.id, &ds->index, attr->u.ppid.id_len);
+		parent_id = SWITCHDEV_ATTR_PORT_PARENT_ID(attr);
+		parent_id->ppid.id_len = sizeof(ds->index);
+		memcpy(&parent_id->ppid.id, &ds->index, parent_id->ppid.id_len);
 		break;
 	default:
 		return -EOPNOTSUPP;
net/switchdev/switchdev.c +51 -26

143 lines not on this page · raw message

Signature · 1 line
1.7.10.4
maintainer Applied
#4 lore raw

Re: [PATCH net-next v3 3/3] switchdev: split switchdev_attr into individual structs

From: [email protected]
Date: Thu, 22 Oct 2015 20:25:17 -0700

Quoted 9 lines

From: Scott Feldman <[email protected]>

This was already done for switchdev_objs. Changing switchdev_attrs to new
style makes switchdev API consistent for both attrs and objs.

No functional changes here.

Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

Applied.

maintainer
#5 lore raw

Re: [PATCH net-next v3 3/3] switchdev: split switchdev_attr into individual structs

From: David Miller <[email protected]>
Date: Fri, 23 Oct 2015 06:32:44 -0700 (PDT)

Quoted 8 lines

From: [email protected]
Date: Thu, 22 Oct 2015 20:25:17 -0700

Quoted 5 lines

From: Scott Feldman <[email protected]>

This was already done for switchdev_objs. Changing switchdev_attrs to new
style makes switchdev API consistent for both attrs and objs.

Quoted 6 lines
Quoted 4 lines

No functional changes here.

Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

Applied.

This breaks the build, I'm reverting this patch series:

net/bridge/br_stp_if.c: In function ‘br_init_port’:
net/bridge/br_stp_if.c:42:3: error: unknown field ‘u’ specified in initializer net/bridge/br_stp_if.c:42:3: warning: excess elements in struct initializer [enabled by default] net/bridge/br_stp_if.c:42:3: warning: (near initialization for ‘attr’) [enabled by default]

maintainer Applied
#6 lore raw
In reply to [email protected]

From: [email protected]
Date: Thu, 22 Oct 2015 20:25:15 -0700

Quoted 10 lines

From: Scott Feldman <[email protected]>

When adding vlans with multiple IFLA_BRIDGE_VLAN_INFO attrs set in AFSPEC, we would wipe the vlan obj struct after the first IFLA_BRIDGE_VLAN_INFO. Fix this by only clearing what's necessary on each IFLA_BRIDGE_VLAN_INFO iteration.

Fixes 9e8f4a54 ("switchdev: push object ID back to object structure")
Signed-off-by Scott Feldman <[email protected]>
Acked-by Jiri Pirko <[email protected]>

Applied.