[PATCH net-next v2 0/4] net: switchdev: Avoid sleep in atomic with DSA

[PATCH net-next v2 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Florian Fainelli]

Hi all,

This patch series is to resolve a sleeping function called in atomic context
debug splat that we observe with DSA.

Let me know what you think, I was also wondering if we should just always
make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
could cause invalid contexts to be used for rocker, mlxsw, i40e etc.

Changes in v2:

- Make patch 3 actually build, thanks to Marcelo for reporting the
  problem

Thanks!

Florian Fainelli (4):
  net: switchdev: Prepare for deferred functions modifying objects
  net: switchdev: Add object dump deferred operation
  net: switchdev: Add switchdev_port_bridge_getlink_deferred
  net: dsa: Utilize switchdev_port_bridge_getlink_deferred()

 include/net/switchdev.h   |   3 +
 net/dsa/slave.c           |   2 +-
 net/switchdev/switchdev.c | 171 +++++++++++++++++++++++++++++++++++++---------
 3 files changed, 141 insertions(+), 35 deletions(-)

-- 
2.9.3

[PATCH net-next v2 1/4] net: switchdev: Prepare for deferred functions modifying objects

[Florian Fainelli]

In preparation for adding support for deferred dump operations, allow
specifying a deferred function whose signature allows read/write
objects.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/switchdev/switchdev.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 017801f9dbaa..3d70ad02c617 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -100,11 +100,14 @@ static DEFINE_SPINLOCK(deferred_lock);
 
 typedef void switchdev_deferred_func_t(struct net_device *dev,
 				       const void *data);
+typedef void switchdev_deferred_func_rw_t(struct net_device *dev,
+					  void *data);
 
 struct switchdev_deferred_item {
 	struct list_head list;
 	struct net_device *dev;
 	switchdev_deferred_func_t *func;
+	switchdev_deferred_func_rw_t *func_rw;
 	unsigned long data[0];
 };
 
@@ -138,7 +141,10 @@ void switchdev_deferred_process(void)
 	ASSERT_RTNL();
 
 	while ((dfitem = switchdev_deferred_dequeue())) {
-		dfitem->func(dfitem->dev, dfitem->data);
+		if (dfitem->func)
+			dfitem->func(dfitem->dev, dfitem->data);
+		else if (dfitem->func_rw)
+			dfitem->func_rw(dfitem->dev, dfitem->data);
 		dev_put(dfitem->dev);
 		kfree(dfitem);
 	}
@@ -156,7 +162,8 @@ static DECLARE_WORK(deferred_process_work, switchdev_deferred_process_work);
 
 static int switchdev_deferred_enqueue(struct net_device *dev,
 				      const void *data, size_t data_len,
-				      switchdev_deferred_func_t *func)
+				      switchdev_deferred_func_t *func,
+				      switchdev_deferred_func_rw_t *func_rw)
 {
 	struct switchdev_deferred_item *dfitem;
 
@@ -165,6 +172,7 @@ static int switchdev_deferred_enqueue(struct net_device *dev,
 		return -ENOMEM;
 	dfitem->dev = dev;
 	dfitem->func = func;
+	dfitem->func_rw = func_rw;
 	memcpy(dfitem->data, data, data_len);
 	dev_hold(dev);
 	spin_lock_bh(&deferred_lock);
@@ -312,7 +320,8 @@ static int switchdev_port_attr_set_defer(struct net_device *dev,
 					 const struct switchdev_attr *attr)
 {
 	return switchdev_deferred_enqueue(dev, attr, sizeof(*attr),
-					  switchdev_port_attr_set_deferred);
+					  switchdev_port_attr_set_deferred,
+					  NULL);
 }
 
 /**
@@ -441,7 +450,8 @@ static int switchdev_port_obj_add_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
 	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
-					  switchdev_port_obj_add_deferred);
+					  switchdev_port_obj_add_deferred,
+					  NULL);
 }
 
 /**
@@ -511,7 +521,8 @@ static int switchdev_port_obj_del_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
 	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
-					  switchdev_port_obj_del_deferred);
+					  switchdev_port_obj_del_deferred,
+					  NULL);
 }
 
 /**
-- 
2.9.3

[PATCH net-next v2 2/4] net: switchdev: Add object dump deferred operation

[Florian Fainelli]

Add plumbing required to perform dump operations in a deferred context,
this mostly wraps the existing switchdev_obj_port_dump() function into a
switchdev_obj_port_dump_now() and adds two wrappers (normal and
deferred) around.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/switchdev/switchdev.c | 71 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 3d70ad02c617..4fa9972d72d2 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -545,26 +545,15 @@ int switchdev_port_obj_del(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
 
-/**
- *	switchdev_port_obj_dump - Dump port objects
- *
- *	@dev: port device
- *	@id: object ID
- *	@obj: object to dump
- *	@cb: function to call with a filled object
- *
- *	rtnl_lock must be held.
- */
-int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
-			    switchdev_obj_dump_cb_t *cb)
+static int switchdev_port_obj_dump_now(struct net_device *dev,
+				       struct switchdev_obj *obj,
+				       switchdev_obj_dump_cb_t *cb)
 {
 	const struct switchdev_ops *ops = dev->switchdev_ops;
 	struct net_device *lower_dev;
 	struct list_head *iter;
 	int err = -EOPNOTSUPP;
 
-	ASSERT_RTNL();
-
 	if (ops && ops->switchdev_port_obj_dump)
 		return ops->switchdev_port_obj_dump(dev, obj, cb);
 
@@ -580,6 +569,60 @@ int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
 
 	return err;
 }
+
+struct switchdev_obj_dump_deferred_item {
+	struct switchdev_obj obj;
+	switchdev_obj_dump_cb_t *cb;
+};
+
+static void switchdev_port_obj_dump_deferred(struct net_device *dev,
+					     void *data)
+{
+	struct switchdev_obj_dump_deferred_item *i = data;
+	struct switchdev_obj *obj = &i->obj;
+	int err;
+
+	err = switchdev_port_obj_dump_now(dev, obj, i->cb);
+	if (err && err != -EOPNOTSUPP)
+		netdev_err(dev, "failed (err=%d) to dump object (id=%d)\n",
+			   err, obj->id);
+	if (obj->complete)
+		obj->complete(dev, err, obj->complete_priv);
+}
+
+static int switchdev_port_obj_dump_defer(struct net_device *dev,
+					 struct switchdev_obj *obj,
+					 switchdev_obj_dump_cb_t *cb)
+{
+	struct switchdev_obj_dump_deferred_item item;
+
+	memcpy(&item.obj, obj, switchdev_obj_size(obj));
+	item.cb = cb;
+
+	return switchdev_deferred_enqueue(dev, &item, sizeof(item),
+					  NULL,
+					  switchdev_port_obj_dump_deferred);
+}
+
+/**
+ *	switchdev_port_obj_dump - Dump port objects
+ *
+ *	@dev: port device
+ *	@id: object ID
+ *	@obj: object to dump
+ *	@cb: function to call with a filled object
+ *
+ *	rtnl_lock must be held and must not be in atomic section,
+ *	in case SWITCHDEV_F_DEFER flag is not set.
+ */
+int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
+			    switchdev_obj_dump_cb_t *cb)
+{
+	if (obj->flags & SWITCHDEV_F_DEFER)
+		return switchdev_port_obj_dump_defer(dev, obj, cb);
+	ASSERT_RTNL();
+	return switchdev_port_obj_dump_now(dev, obj, cb);
+}
 EXPORT_SYMBOL_GPL(switchdev_port_obj_dump);
 
 static RAW_NOTIFIER_HEAD(switchdev_notif_chain);
-- 
2.9.3

[PATCH net-next v2 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Florian Fainelli]

Add switchdev_port_bridge_getlink_deferred() which does a deferred
object dump operation, this is required for e.g: DSA switches which
typically have sleeping I/O operations which is incompatible with being
in atomic context obviously.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/switchdev.h   |  3 ++
 net/switchdev/switchdev.c | 77 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index eba80c4fc56f..087761b0df49 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -189,6 +189,9 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
 int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 				  struct net_device *dev, u32 filter_mask,
 				  int nlflags);
+int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask, int nlflags);
 int switchdev_port_bridge_setlink(struct net_device *dev,
 				  struct nlmsghdr *nlh, u16 flags);
 int switchdev_port_bridge_dellink(struct net_device *dev,
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 4fa9972d72d2..f5db799fc0a0 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -776,12 +776,14 @@ static int switchdev_port_vlan_dump_cb(struct switchdev_obj *obj)
 	return err;
 }
 
-static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
-				    u32 filter_mask)
+static int __switchdev_port_vlan_fill(struct sk_buff *skb,
+				      struct net_device *dev,
+				      u32 filter_mask, u32 obj_flags)
 {
 	struct switchdev_vlan_dump dump = {
 		.vlan.obj.orig_dev = dev,
 		.vlan.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vlan.obj.flags = obj_flags,
 		.skb = skb,
 		.filter_mask = filter_mask,
 	};
@@ -802,17 +804,27 @@ static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
 	return err == -EOPNOTSUPP ? 0 : err;
 }
 
-/**
- *	switchdev_port_bridge_getlink - Get bridge port attributes
- *
- *	@dev: port device
- *
- *	Called for SELF on rtnl_bridge_getlink to get bridge port
- *	attributes.
- */
-int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
-				  struct net_device *dev, u32 filter_mask,
-				  int nlflags)
+static int switchdev_port_vlan_fill_deferred(struct sk_buff *skb,
+					     struct net_device *dev,
+					     u32 filter_mask)
+{
+	return __switchdev_port_vlan_fill(skb, dev, filter_mask,
+					  SWITCHDEV_F_DEFER);
+}
+
+static int switchdev_port_vlan_fill(struct sk_buff *skb,
+				    struct net_device *dev,
+				    u32 filter_mask)
+{
+	return __switchdev_port_vlan_fill(skb, dev, filter_mask, 0);
+}
+
+static int __switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask, int nlflags,
+					   int (*fill_cb)(struct sk_buff *skb,
+							  struct net_device *d,
+							  u32 filter_mask))
 {
 	struct switchdev_attr attr = {
 		.orig_dev = dev,
@@ -831,10 +843,47 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 
 	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
 				       attr.u.brport_flags, mask, nlflags,
-				       filter_mask, switchdev_port_vlan_fill);
+				       filter_mask, fill_cb);
+}
+
+/**
+ *	switchdev_port_bridge_getlink - Get bridge port attributes
+ *
+ *	@dev: port device
+ *
+ *	Called for SELF on rtnl_bridge_getlink to get bridge port
+ *	attributes.
+ */
+int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
+				  struct net_device *dev, u32 filter_mask,
+				  int nlflags)
+{
+	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
+					       nlflags,
+					       switchdev_port_vlan_fill);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink);
 
+/**
+ *	switchdev_port_bridge_getlink_deferred - Get bridge port attributes
+ *	(deferred variant)
+ *
+ *	@dev: port device
+ *
+ *	Called for SELF on rtnl_bridge_getlink to get bridge port
+ *	attributes.
+ */
+int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask,
+					   int nlflags)
+{
+	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
+					       nlflags,
+					       switchdev_port_vlan_fill_deferred);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink_deferred);
+
 static int switchdev_port_br_setflag(struct net_device *dev,
 				     struct nlattr *nlattr,
 				     unsigned long brport_flag)
-- 
2.9.3

[PATCH net-next v2 4/4] net: dsa: Utilize switchdev_port_bridge_getlink_deferred()

[Florian Fainelli]

Fixes the following sleeping in atomic splat:

[   69.008021] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:752
[   69.016523] in_atomic(): 1, irqs_disabled(): 0, pid: 1528, name: bridge
[   69.023167] INFO: lockdep is turned off.
[   69.027118] CPU: 1 PID: 1528 Comm: bridge Not tainted 4.10.0-rc2-00131-g719651624789-dirty #205
[   69.035840] Hardware name: Broadcom STB (Flattened Device Tree)
[   69.041796] [<c020fc40>] (unwind_backtrace) from [<c020ba28>] (show_stack+0x10/0x14)
[   69.049570] [<c020ba28>] (show_stack) from [<c04fb91c>] (dump_stack+0xb0/0xdc)
[   69.056823] [<c04fb91c>] (dump_stack) from [<c0244d00>] (___might_sleep+0x1a4/0x2a4)
[   69.064599] [<c0244d00>] (___might_sleep) from [<c08f3eb8>] (mutex_lock_nested+0x28/0x7d8)
[   69.072897] [<c08f3eb8>] (mutex_lock_nested) from [<c0674dbc>] (b53_vlan_dump+0x2c/0x104)
[   69.081105] [<c0674dbc>] (b53_vlan_dump) from [<c08eba88>] (switchdev_port_obj_dump_now+0x30/0x6c)
[   69.090094] [<c08eba88>] (switchdev_port_obj_dump_now) from [<c08ebb00>] (switchdev_port_obj_dump+0x3c/0x98)
[   69.099950] [<c08ebb00>] (switchdev_port_obj_dump) from [<c08ebc34>] (switchdev_port_vlan_fill+0x68/0x90)
[   69.109550] [<c08ebc34>] (switchdev_port_vlan_fill) from [<c07f13a4>] (ndo_dflt_bridge_getlink+0x28c/0x4fc)
[   69.119320] [<c07f13a4>] (ndo_dflt_bridge_getlink) from [<c08eb2c8>] (switchdev_port_bridge_getlink+0xc4/0xd4)
[   69.129350] [<c08eb2c8>] (switchdev_port_bridge_getlink) from [<c07f0b10>] (rtnl_bridge_getlink+0x12c/0x28c)
[   69.139206] [<c07f0b10>] (rtnl_bridge_getlink) from [<c0802e28>] (netlink_dump+0xe8/0x268)
[   69.147495] [<c0802e28>] (netlink_dump) from [<c0803864>] (__netlink_dump_start+0x12c/0x18c)
[   69.155958] [<c0803864>] (__netlink_dump_start) from [<c07f3c34>] (rtnetlink_rcv_msg+0x11c/0x228)
[   69.164857] [<c07f3c34>] (rtnetlink_rcv_msg) from [<c0805e3c>] (netlink_rcv_skb+0xc4/0xd8)
[   69.173145] [<c0805e3c>] (netlink_rcv_skb) from [<c07f1110>] (rtnetlink_rcv+0x28/0x30)
[   69.181087] [<c07f1110>] (rtnetlink_rcv) from [<c080577c>] (netlink_unicast+0x16c/0x238)
[   69.189201] [<c080577c>] (netlink_unicast) from [<c0805c3c>] (netlink_sendmsg+0x350/0x364)
[   69.197493] [<c0805c3c>] (netlink_sendmsg) from [<c07beab0>] (sock_sendmsg+0x14/0x24)
[   69.205350] [<c07beab0>] (sock_sendmsg) from [<c07bfdbc>] (SyS_sendto+0xb8/0xe0)
[   69.212769] [<c07bfdbc>] (SyS_sendto) from [<c07bfdfc>] (SyS_send+0x18/0x20)
[   69.219841] [<c07bfdfc>] (SyS_send) from [<c0208100>] (ret_fast_syscall+0x0/0x1c)

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5cd5b8137c08..b38536f951ea 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1038,7 +1038,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_netpoll_cleanup	= dsa_slave_netpoll_cleanup,
 	.ndo_poll_controller	= dsa_slave_poll_controller,
 #endif
-	.ndo_bridge_getlink	= switchdev_port_bridge_getlink,
+	.ndo_bridge_getlink	= switchdev_port_bridge_getlink_deferred,
 	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
 	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
 	.ndo_get_phys_port_id	= dsa_slave_get_phys_port_id,
-- 
2.9.3

Re: [PATCH net-next v2 1/4] net: switchdev: Prepare for deferred functions modifying objects

[Jiri Pirko]

Mon, Jan 09, 2017 at 09:45:20PM CET, f.fainelli@gmail.com wrote:
>In preparation for adding support for deferred dump operations, allow
>specifying a deferred function whose signature allows read/write
>objects.
>
>Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>---
> net/switchdev/switchdev.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>index 017801f9dbaa..3d70ad02c617 100644
>--- a/net/switchdev/switchdev.c
>+++ b/net/switchdev/switchdev.c
>@@ -100,11 +100,14 @@ static DEFINE_SPINLOCK(deferred_lock);
> 
> typedef void switchdev_deferred_func_t(struct net_device *dev,
> 				       const void *data);
>+typedef void switchdev_deferred_func_rw_t(struct net_device *dev,
>+					  void *data);
> 
> struct switchdev_deferred_item {
> 	struct list_head list;
> 	struct net_device *dev;
> 	switchdev_deferred_func_t *func;
>+	switchdev_deferred_func_rw_t *func_rw;

I'm missing why you need to have 2 funcs here. Why you just can't re-use
func?


> 	unsigned long data[0];
> };

Re: [PATCH net-next v2 1/4] net: switchdev: Prepare for deferred functions modifying objects

[Florian Fainelli]

On 01/10/2017 02:09 AM, Jiri Pirko wrote:
> Mon, Jan 09, 2017 at 09:45:20PM CET, f.fainelli@gmail.com wrote:
>> In preparation for adding support for deferred dump operations, allow
>> specifying a deferred function whose signature allows read/write
>> objects.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> net/switchdev/switchdev.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 017801f9dbaa..3d70ad02c617 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -100,11 +100,14 @@ static DEFINE_SPINLOCK(deferred_lock);
>>
>> typedef void switchdev_deferred_func_t(struct net_device *dev,
>> 				       const void *data);
>> +typedef void switchdev_deferred_func_rw_t(struct net_device *dev,
>> +					  void *data);
>>
>> struct switchdev_deferred_item {
>> 	struct list_head list;
>> 	struct net_device *dev;
>> 	switchdev_deferred_func_t *func;
>> +	switchdev_deferred_func_rw_t *func_rw;
> 
> I'm missing why you need to have 2 funcs here. Why you just can't re-use
> func?

I wanted to let the existing callers be passed down a const void *data,
and not update them with void *data, since there is value in keeping
that annotated. This may be considered overkill, I don't know.
-- 
Florian