mirred, redirect action vs. dev refcount issue

mirred, redirect action vs. dev refcount issue

[Stephen Hemminger]

Both the mirrored and redirect TC actions, hold a pointer to the
target device and increment the refcount.  The problem is that administrator
may want to remove the target device (for example ifb0) and this will
cause the kernel to get in the "can't delete ifb0 with references" state.

Fixing this isn't trivial but I think that the best way would be to have
the action API have a device notifier and walk the actions and call a new
hook (device_event) similar to existing walk. The device_event in the
action ops can then redirect any mirror/redirect that are going to a dead
device off to bit bucket.

Alternatively, the mirror/redirect could just use ifindex which is
a soft reference, so if device is removed, they just drop.

Lazy me favors the later.

Re: mirred, redirect action vs. dev refcount issue

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 21 Jul 2010 16:24:26 -0700

> Alternatively, the mirror/redirect could just use ifindex which is
> a soft reference, so if device is removed, they just drop.
> 
> Lazy me favors the later.

If it is the action rule holding onto the device, it should have
an appropriate netdevice notifier handler.

If it's a transient reference on receive, it should be transient
and released eventually.

Re: mirred, redirect action vs. dev refcount issue

[Stephen Hemminger]

On Wed, 21 Jul 2010 16:39:55 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 21 Jul 2010 16:24:26 -0700
> 
> > Alternatively, the mirror/redirect could just use ifindex which is
> > a soft reference, so if device is removed, they just drop.
> > 
> > Lazy me favors the later.
> 
> If it is the action rule holding onto the device, it should have
> an appropriate netdevice notifier handler.

There is no notifier there, and the module doesn't keep track of
list of filters. So that is why it has to be done at act api level.

> If it's a transient reference on receive, it should be transient
> and released eventually.

Kernel doesn't keep transient reference on receive any more.

Re: mirred, redirect action vs. dev refcount issue

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 21 Jul 2010 16:52:47 -0700

> On Wed, 21 Jul 2010 16:39:55 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
>> If it is the action rule holding onto the device, it should have
>> an appropriate netdevice notifier handler.
> 
> There is no notifier there, and the module doesn't keep track of
> list of filters. So that is why it has to be done at act api level.

Any rule, or route, or whatever that makes references to devices must
transparently accomodate the requested removal or downing of a device.

There is no way around this.

So either the action code needs to keep track of it's table entries on
some global list that can be traversed at notifier time, or we go with
the ifindex thing.

Whether the ifindex or the global list + delete scheme is better is a
topic for discussion.  Since from the user's perspective it is unclear
which semantic is less surprising, entries disappearing or suddenly
stop working (or start applying to a different device which has taken
a previous one's ifindex!).

Re: mirred, redirect action vs. dev refcount issue

[Stephen Hemminger]

On Wed, 21 Jul 2010 16:58:02 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 21 Jul 2010 16:52:47 -0700
> 
> > On Wed, 21 Jul 2010 16:39:55 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> >> If it is the action rule holding onto the device, it should have
> >> an appropriate netdevice notifier handler.
> > 
> > There is no notifier there, and the module doesn't keep track of
> > list of filters. So that is why it has to be done at act api level.
> 
> Any rule, or route, or whatever that makes references to devices must
> transparently accomodate the requested removal or downing of a device.
> 
> There is no way around this.
> 
> So either the action code needs to keep track of it's table entries on
> some global list that can be traversed at notifier time, or we go with
> the ifindex thing.
> 
> Whether the ifindex or the global list + delete scheme is better is a
> topic for discussion.  Since from the user's perspective it is unclear
> which semantic is less surprising, entries disappearing or suddenly
> stop working (or start applying to a different device which has taken
> a previous one's ifindex!).

ifindex is unique (until integer wraps) so that soft reference
works.

-- 

Re: mirred, redirect action vs. dev refcount issue

[jamal]

On Wed, 2010-07-21 at 17:00 -0700, Stephen Hemminger wrote:
> On Wed, 21 Jul 2010 16:58:02 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 

> > Whether the ifindex or the global list + delete scheme is better is a
> > topic for discussion.  Since from the user's perspective it is unclear
> > which semantic is less surprising, entries disappearing or suddenly
> > stop working (or start applying to a different device which has taken
> > a previous one's ifindex!).
> 
> ifindex is unique (until integer wraps) so that soft reference
> works.

The proper way to do it is via a notifier since we point to the
netdev - and yes it is a little more complex thats why i just
let the admin suffer (IMO) the well deserved consequences[1].

I am in travel mode - but i will do some background thinking and
come up with a good way to resolve it when i get back. Unless you
have a patch you want me to look at.

cheers,
jamal

[1] least element of suprise principle:
Admin adds a rule which says
"you see a packet matching blah incoming on eth0,
do action1 then action2 ... then actionN"
Say action2 is "mirror to ifb0".
And then this same admin goes and rmmods ifb0 - it is easier to
just reject this rmmod operation as we do todau. Maybe we could be 
kinder and be more informative and syslog something along the lines of
"rejected to unregister device you rat-bastard because you have a rule
which says we should mirror to ifb0". 
Thoughts?

Re: mirred, redirect action vs. dev refcount issue

[Stephen Hemminger]

This is what I am testing now.

--- a/include/net/tc_act/tc_mirred.h	2010-07-22 09:17:41.631561924 -0700
+++ b/include/net/tc_act/tc_mirred.h	2010-07-22 09:49:53.355325645 -0700
@@ -9,6 +9,7 @@ struct tcf_mirred {
 	int			tcfm_ifindex;
 	int			tcfm_ok_push;
 	struct net_device	*tcfm_dev;
+	struct list_head	tcfm_list;
 };
 #define to_mirred(pc) \
 	container_of(pc, struct tcf_mirred, common)
--- a/net/sched/act_mirred.c	2010-07-21 16:10:48.922308036 -0700
+++ b/net/sched/act_mirred.c	2010-07-22 09:52:32.229910928 -0700
@@ -1,3 +1,4 @@
+
 /*
  * net/sched/mirred.c	packet mirroring and redirect actions
  *
@@ -21,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/gfp.h>
+#include <linux/notifier.h>
 #include <net/net_namespace.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
@@ -33,6 +35,7 @@
 static struct tcf_common *tcf_mirred_ht[MIRRED_TAB_MASK + 1];
 static u32 mirred_idx_gen;
 static DEFINE_RWLOCK(mirred_lock);
+static LIST_HEAD(mirred_list);
 
 static struct tcf_hashinfo mirred_hash_info = {
 	.htab	=	tcf_mirred_ht,
@@ -47,7 +50,10 @@ static inline int tcf_mirred_release(str
 			m->tcf_bindcnt--;
 		m->tcf_refcnt--;
 		if(!m->tcf_bindcnt && m->tcf_refcnt <= 0) {
-			dev_put(m->tcfm_dev);
+			ASSERT_RTNL();
+			list_del(&m->tcfm_list);
+			if (m->tcfm_dev)
+				dev_put(m->tcfm_dev);
 			tcf_hash_destroy(&m->common, &mirred_hash_info);
 			return 1;
 		}
@@ -134,8 +140,11 @@ static int tcf_mirred_init(struct nlattr
 		m->tcfm_ok_push = ok_push;
 	}
 	spin_unlock_bh(&m->tcf_lock);
-	if (ret == ACT_P_CREATED)
+	if (ret == ACT_P_CREATED) {
+		ASSERT_RTNL();
+		list_add(&m->tcfm_list, &mirred_list);
 		tcf_hash_insert(pc, &mirred_hash_info);
+	}
 
 	return ret;
 }
@@ -164,9 +173,14 @@ static int tcf_mirred(struct sk_buff *sk
 	m->tcf_bstats.packets++;
 
 	dev = m->tcfm_dev;
+	if (!dev) {
+		printk_once(KERN_NOTICE "tc mirred: target device is gone\n");
+		goto out;
+	}
+
 	if (!(dev->flags & IFF_UP)) {
 		if (net_ratelimit())
-			pr_notice("tc mirred to Houston: device %s is gone!\n",
+			pr_notice("tc mirred to Houston: device %s is down\n",
 				  dev->name);
 		goto out;
 	}
@@ -230,6 +244,28 @@ nla_put_failure:
 	return -1;
 }
 
+static int mirred_device_event(struct notifier_block *unused,
+			       unsigned long event, void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct tcf_mirred *m;
+
+	if (event == NETDEV_UNREGISTER)
+		list_for_each_entry(m, &mirred_list, tcfm_list) {
+			if (m->tcfm_dev == dev) {
+				dev_put(dev);
+				m->tcfm_dev = NULL;
+			}
+		}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block mirred_device_notifier = {
+	.notifier_call = mirred_device_event,
+};
+
+
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
 	.hinfo		=	&mirred_hash_info,
@@ -250,12 +286,17 @@ MODULE_LICENSE("GPL");
 
 static int __init mirred_init_module(void)
 {
+	int err = register_netdevice_notifier(&mirred_device_notifier);
+	if (err)
+		return err;
+
 	pr_info("Mirror/redirect action on\n");
 	return tcf_register_action(&act_mirred_ops);
 }
 
 static void __exit mirred_cleanup_module(void)
 {
+	unregister_netdevice_notifier(&mirred_device_notifier);
 	tcf_unregister_action(&act_mirred_ops);
 }
 

Re: mirred, redirect action vs. dev refcount issue

[jamal]

On Thu, 2010-07-22 at 10:42 -0700, Stephen Hemminger wrote:
> This is what I am testing now.
> 

Looks very reasonable to me. Please add my signoff when you 
submit.

cheers,
jamal

[PATCH] net sched: fix race in mirred device removal

[Stephen Hemminger]

This fixes hang when target device of mirred packet classifier
action is removed.

If a mirror or redirection action is configured to cause packets
to go to another device, the classifier holds a ref count, but was assuming
the adminstrator cleaned up all redirections before removing. The fix
is to add a notifier and cleanup during unregister.

The new list is implicitly protected by RTNL mutex because
it is held during filter add/delete as well as notifier.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

---
Patch is against 2.6.35-rc6 (not net-next)

--- a/include/net/tc_act/tc_mirred.h	2010-07-22 21:32:40.944000510 -0700
+++ b/include/net/tc_act/tc_mirred.h	2010-07-22 21:32:44.280173100 -0700
@@ -9,6 +9,7 @@ struct tcf_mirred {
 	int			tcfm_ifindex;
 	int			tcfm_ok_push;
 	struct net_device	*tcfm_dev;
+	struct list_head	tcfm_list;
 };
 #define to_mirred(pc) \
 	container_of(pc, struct tcf_mirred, common)
--- a/net/sched/act_mirred.c	2010-07-22 21:32:40.927999682 -0700
+++ b/net/sched/act_mirred.c	2010-07-22 21:42:22.356826443 -0700
@@ -33,6 +33,7 @@
 static struct tcf_common *tcf_mirred_ht[MIRRED_TAB_MASK + 1];
 static u32 mirred_idx_gen;
 static DEFINE_RWLOCK(mirred_lock);
+static LIST_HEAD(mirred_list);
 
 static struct tcf_hashinfo mirred_hash_info = {
 	.htab	=	tcf_mirred_ht,
@@ -47,7 +48,9 @@ static inline int tcf_mirred_release(str
 			m->tcf_bindcnt--;
 		m->tcf_refcnt--;
 		if(!m->tcf_bindcnt && m->tcf_refcnt <= 0) {
-			dev_put(m->tcfm_dev);
+			list_del(&m->tcfm_list);
+			if (m->tcfm_dev)
+				dev_put(m->tcfm_dev);
 			tcf_hash_destroy(&m->common, &mirred_hash_info);
 			return 1;
 		}
@@ -134,8 +137,10 @@ static int tcf_mirred_init(struct nlattr
 		m->tcfm_ok_push = ok_push;
 	}
 	spin_unlock_bh(&m->tcf_lock);
-	if (ret == ACT_P_CREATED)
+	if (ret == ACT_P_CREATED) {
+		list_add(&m->tcfm_list, &mirred_list);
 		tcf_hash_insert(pc, &mirred_hash_info);
+	}
 
 	return ret;
 }
@@ -162,9 +167,14 @@ static int tcf_mirred(struct sk_buff *sk
 	m->tcf_tm.lastuse = jiffies;
 
 	dev = m->tcfm_dev;
+	if (!dev) {
+		printk_once(KERN_NOTICE "tc mirred: target device is gone\n");
+		goto out;
+	}
+
 	if (!(dev->flags & IFF_UP)) {
 		if (net_ratelimit())
-			pr_notice("tc mirred to Houston: device %s is gone!\n",
+			pr_notice("tc mirred to Houston: device %s is down\n",
 				  dev->name);
 		goto out;
 	}
@@ -232,6 +242,28 @@ nla_put_failure:
 	return -1;
 }
 
+static int mirred_device_event(struct notifier_block *unused,
+			       unsigned long event, void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct tcf_mirred *m;
+
+	if (event == NETDEV_UNREGISTER)
+		list_for_each_entry(m, &mirred_list, tcfm_list) {
+			if (m->tcfm_dev == dev) {
+				dev_put(dev);
+				m->tcfm_dev = NULL;
+			}
+		}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block mirred_device_notifier = {
+	.notifier_call = mirred_device_event,
+};
+
+
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
 	.hinfo		=	&mirred_hash_info,
@@ -252,12 +284,17 @@ MODULE_LICENSE("GPL");
 
 static int __init mirred_init_module(void)
 {
+	int err = register_netdevice_notifier(&mirred_device_notifier);
+	if (err)
+		return err;
+
 	pr_info("Mirror/redirect action on\n");
 	return tcf_register_action(&act_mirred_ops);
 }
 
 static void __exit mirred_cleanup_module(void)
 {
+	unregister_netdevice_notifier(&mirred_device_notifier);
 	tcf_unregister_action(&act_mirred_ops);
 }
 

Re: [PATCH] net sched: fix race in mirred device removal

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 22 Jul 2010 21:45:04 -0700

> This fixes hang when target device of mirred packet classifier
> action is removed.
> 
> If a mirror or redirection action is configured to cause packets
> to go to another device, the classifier holds a ref count, but was assuming
> the adminstrator cleaned up all redirections before removing. The fix
> is to add a notifier and cleanup during unregister.
> 
> The new list is implicitly protected by RTNL mutex because
> it is held during filter add/delete as well as notifier.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

Applied to net-2.6, thanks.