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

[Cong Wang]

On Mon, Nov 13, 2017 at 10:35 PM, Jiri Pirko  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.

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

[Jiri Pirko]

Tue, Nov 14, 2017 at 06:51:42AM CET, xiyou.wangc...@gmail.com wrote:
>On Mon, Nov 13, 2017 at 9:17 PM, Jiri Pirko  wrote:
>> Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangc...@gmail.com wrote:
>>>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko  wrote:
 From: Jiri Pirko 

 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.

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

[Cong Wang]

On Mon, Nov 13, 2017 at 9:17 PM, Jiri Pirko  wrote:
> Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangc...@gmail.com wrote:
>>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko  wrote:
>>> From: Jiri Pirko 
>>>
>>> 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.

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

[Jiri Pirko]

Mon, Nov 13, 2017 at 08:53:57PM CET, xiyou.wangc...@gmail.com wrote:
>On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko  wrote:
>> From: Jiri Pirko 
>>
>> 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?

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

[Cong Wang]

On Mon, Nov 13, 2017 at 6:05 AM, Jiri Pirko  wrote:
> From: Jiri Pirko 
>
> 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.

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

[David Ahern]

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.

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

[Jiri Pirko]

From: Jiri Pirko 

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 
---
 include/linux/netdevice.h | 44 
 net/core/dev.c|  2 +-
 net/sched/act_mirred.c|  8 
 3 files changed, 49 insertions(+), 5 deletions(-)

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;
boolproto_down;
+   unsigned intnetns_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(>tcfm_list);
dev = rcu_dereference_protected(m->tcfm_dev, 1);
if (dev)
-   dev_put(dev);
+   dev_put_netns(dev);
spin_unlock_bh(_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)
-