diff mbox series

[net] net: forbid netdev used by mirred tc act from being moved to another netns

Message ID 20171113140541.1128-1-jiri@resnulli.us
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [net] net: forbid netdev used by mirred tc act from being moved to another netns | expand

Commit Message

Jiri Pirko Nov. 13, 2017, 2:05 p.m. UTC 
From: Jiri Pirko <jiri@mellanox.com>

Currently, user may choose to move device that is used by mirred action
to another network namespace. That is wrong as the action still remains
in the original namespace and references non-existing ifindex.
See an example to illustrate this:

$ ip link add dummyx1 type dummy
$ ip link add dummyx2 type dummy
$ tc qdisc add dev dummyx1 clsact
$ tc filter add dev dummyx1 ingress protocol all matchall action mirred egress mirror dev dummyx2
$ tc -s filter show dev dummyx1 ingress
filter protocol all pref 49152 matchall
filter protocol all pref 49152 matchall handle 0x1
        action order 1: mirred (Egress Mirror to device dummyx2) pipe
        index 1 ref 1 bind 1 installed 16 sec used 16 sec
        Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

$ ip netns add testx1
$ ip link set dummyx2 netns testx1
$ tc -s filter show dev dummyx1 ingress
filter protocol all pref 49152 matchall
filter protocol all pref 49152 matchall handle 0x1
        action order 1: mirred (Egress Mirror to device if13) pipe
                                                        ^^^^
        index 1 ref 1 bind 1 installed 56 sec used 56 sec
        Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

It may even happen that this ifindex is used by another totally
unrelated netdevice. So fix this by disallowing the netdevice used by
mirred action to move to another network namespace.

Fixes: ce286d327341 ("[NET]: Implement network device movement between namespaces")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c            |  2 +-
 net/sched/act_mirred.c    |  8 ++++----
 3 files changed, 49 insertions(+), 5 deletions(-)

Comments

David Ahern Nov. 13, 2017, 5:37 p.m. UTC | #1 
On 11/13/17 7:05 AM, Jiri Pirko wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 11596a3..877979f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8250,7 +8250,7 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
>  
>  	/* Don't allow namespace local devices to be moved. */
>  	err = -EINVAL;
> -	if (dev->features & NETIF_F_NETNS_LOCAL)
> +	if (dev->features & NETIF_F_NETNS_LOCAL || dev_netns_blocked(dev))
>  		goto out;
>  
>  	/* Ensure the device has been registrered */

Add the extack arg to dev_change_net_namespace and tell user why the
namespace change is not allowed. And for the netns_blocked case, EINVAL
does not seem the proper error code since the device is legit.
Cong Wang Nov. 13, 2017, 7:53 p.m. UTC | #2 
On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Currently, user may choose to move device that is used by mirred action
> to another network namespace. That is wrong as the action still remains
> in the original namespace and references non-existing ifindex.

It is a pure display issue, the action itself should function well
because we only use ifindex to lookup netdevice once and
we save the netdevice pointer in action.

If you really want to fix it, just tell iprout2 to display netnsid together
with ifindex.
Jiri Pirko Nov. 14, 2017, 5:17 a.m. UTC | #3 
Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangcong@gmail.com wrote:
>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Currently, user may choose to move device that is used by mirred action
>> to another network namespace. That is wrong as the action still remains
>> in the original namespace and references non-existing ifindex.
>
>It is a pure display issue, the action itself should function well
>because we only use ifindex to lookup netdevice once and
>we save the netdevice pointer in action.
>
>If you really want to fix it, just tell iprout2 to display netnsid together
>with ifindex.

It is not only display issue. I think it is wrong to let a netdevice
dissapear from underneath the mirred action. You certainly cannot add an
action mirred with device from another net namespace. So should we allow
that?
Cong Wang Nov. 14, 2017, 5:51 a.m. UTC | #4 
On Mon, Nov 13, 2017 at 9:17 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangcong@gmail.com wrote:
>>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Currently, user may choose to move device that is used by mirred action
>>> to another network namespace. That is wrong as the action still remains
>>> in the original namespace and references non-existing ifindex.
>>
>>It is a pure display issue, the action itself should function well
>>because we only use ifindex to lookup netdevice once and
>>we save the netdevice pointer in action.
>>
>>If you really want to fix it, just tell iprout2 to display netnsid together
>>with ifindex.
>
> It is not only display issue. I think it is wrong to let a netdevice

What's wrong with it? Is it mis-functioning?

> dissapear from underneath the mirred action. You certainly cannot add an


It disappears only because we don't display it properly, nothing else.


> action mirred with device from another net namespace. So should we allow
> that?

On the other hand why linking a device to mirred action prevents it
from moving to another netns? Also, device can be moved back too.

I don't see anything wrong with it except displaying it.
Jiri Pirko Nov. 14, 2017, 6:35 a.m. UTC | #5 
Tue, Nov 14, 2017 at 06:51:42AM CET, xiyou.wangcong@gmail.com wrote:
>On Mon, Nov 13, 2017 at 9:17 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangcong@gmail.com wrote:
>>>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Currently, user may choose to move device that is used by mirred action
>>>> to another network namespace. That is wrong as the action still remains
>>>> in the original namespace and references non-existing ifindex.
>>>
>>>It is a pure display issue, the action itself should function well
>>>because we only use ifindex to lookup netdevice once and
>>>we save the netdevice pointer in action.
>>>
>>>If you really want to fix it, just tell iprout2 to display netnsid together
>>>with ifindex.
>>
>> It is not only display issue. I think it is wrong to let a netdevice
>
>What's wrong with it? Is it mis-functioning?

Nope.

>
>> dissapear from underneath the mirred action. You certainly cannot add an
>
>
>It disappears only because we don't display it properly, nothing else.

Okay.

>
>
>> action mirred with device from another net namespace. So should we allow
>> that?
>
>On the other hand why linking a device to mirred action prevents it
>from moving to another netns? Also, device can be moved back too.
>
>I don't see anything wrong with it except displaying it.

Okay. What about my question? Should we allow adding an action mirred
pointing to a netdev in another netns? I think it would make sense in
case we consider movement of mirred device legit.
Cong Wang Nov. 14, 2017, 6:53 p.m. UTC | #6 
On Mon, Nov 13, 2017 at 10:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> Okay. What about my question? Should we allow adding an action mirred
> pointing to a netdev in another netns? I think it would make sense in
> case we consider movement of mirred device legit.

I don't think it is possible to add an action pointing to any netdev in
other netns in current code base, you just can't find it.

Moving a netdev after linking it to an action is different, if you want to
argue this using above question. Because we allow other "linking"
netdev to be moved too, like a tunnel device on top of a physical
one (this is why we have netnsid).

The "linking" of a mirred action might not be as strong as a tunnel
device "linking", but the idea is pretty much similar, I don't see
anything fundamentally wrong.
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2eaac7d..35672e6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1898,6 +1898,7 @@  struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	struct lock_class_key	*qdisc_running_key;
 	bool			proto_down;
+	unsigned int		netns_blocker_count;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
@@ -3345,6 +3346,49 @@  static inline void dev_hold(struct net_device *dev)
 	this_cpu_inc(*dev->pcpu_refcnt);
 }
 
+static inline void dev_netns_blocker_inc(struct net_device *dev)
+{
+	dev->netns_blocker_count++;
+}
+
+static inline void dev_netns_blocker_dec(struct net_device *dev)
+{
+	WARN_ON(dev->netns_blocker_count-- == 0);
+}
+
+/**
+ *	dev_put - release reference to device, allow netns change
+ *	@dev: network device
+ *
+ * Release reference to device to allow it to be freed.
+ * Also, release netns blocker reference and allow it
+ * to be moved to another network namespace.
+ */
+static inline void dev_put_netns(struct net_device *dev)
+{
+	dev_netns_blocker_dec(dev);
+	dev_put(dev);
+}
+
+/**
+ *	dev_hold_netns - get reference to device, disallow netns change
+ *	@dev: network device
+ *
+ * Hold reference to device to keep it from being freed.
+ * Also, hold netns blocker reference to keep it from
+ * being moved to another network namespace.
+ */
+static inline void dev_hold_netns(struct net_device *dev)
+{
+	dev_hold(dev);
+	dev_netns_blocker_inc(dev);
+}
+
+static inline bool dev_netns_blocked(struct net_device *dev)
+{
+	return dev->netns_blocker_count;
+}
+
 /* Carrier loss detection, dial on demand. The functions netif_carrier_on
  * and _off may be called from IRQ context, but it is caller
  * who is responsible for serialization of these calls.
diff --git a/net/core/dev.c b/net/core/dev.c
index 11596a3..877979f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8250,7 +8250,7 @@  int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 
 	/* Don't allow namespace local devices to be moved. */
 	err = -EINVAL;
-	if (dev->features & NETIF_F_NETNS_LOCAL)
+	if (dev->features & NETIF_F_NETNS_LOCAL || dev_netns_blocked(dev))
 		goto out;
 
 	/* Ensure the device has been registrered */
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 416627c..5a7b949 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -60,7 +60,7 @@  static void tcf_mirred_release(struct tc_action *a, int bind)
 	list_del(&m->tcfm_list);
 	dev = rcu_dereference_protected(m->tcfm_dev, 1);
 	if (dev)
-		dev_put(dev);
+		dev_put_netns(dev);
 	spin_unlock_bh(&mirred_list_lock);
 }
 
@@ -141,8 +141,8 @@  static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	if (dev != NULL) {
 		m->tcfm_ifindex = parm->ifindex;
 		if (ret != ACT_P_CREATED)
-			dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
-		dev_hold(dev);
+			dev_put_netns(rcu_dereference_protected(m->tcfm_dev, 1));
+		dev_hold_netns(dev);
 		rcu_assign_pointer(m->tcfm_dev, dev);
 		m->tcfm_mac_header_xmit = mac_header_xmit;
 	}
@@ -296,7 +296,7 @@  static int mirred_device_event(struct notifier_block *unused,
 		spin_lock_bh(&mirred_list_lock);
 		list_for_each_entry(m, &mirred_list, tcfm_list) {
 			if (rcu_access_pointer(m->tcfm_dev) == dev) {
-				dev_put(dev);
+				dev_put_netns(dev);
 				/* Note : no rcu grace period necessary, as
 				 * net_device are already rcu protected.
 				 */