mirred, redirect action vs. dev refcount issue

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.

From: Stephen Hemminger <[email protected]>
Date: Wed, 21 Jul 2010 16:24:26 -0700

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.

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.

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

From: Stephen Hemminger <[email protected]>
Date: Wed, 21 Jul 2010 16:52:47 -0700

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.

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?

This is what I am testing now.

@@ -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)

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

cheers,
jamal

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.

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

@@ -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)

From: Stephen Hemminger <[email protected]>
Date: Thu, 22 Jul 2010 21:45:04 -0700

Applied to net-2.6, thanks.