[RFC] netfilter: WIP: Xtables idletimer target implementation

[RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

This patch implements an idletimer Xtables target that can be used to
identify when interfaces have been idle for a certain period of time.

It adds a file to the sysfs for each interface that is brought up.  The file
contains the time remaining before the event is triggered.

The timeout should be set when the Xtable rule is defined with the --timeout
parameter set.

There are still some issues to be resolved:

How to treat several rules for the same interface?

We need a key for the timer list.  I'm using the targinfo pointer for that,
but this looks shaky, because there is no assurance that this pointer will
live for the entire lifetime of the rule.

This is now an x_tables target, so other protocols need to be implemented.
I've only implemented the ipv4 part so far.

Cc: Timo Teras <timo.teras@iki.fi>
Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com>
---
 include/linux/netfilter/xt_IDLETIMER.h |   36 +++
 net/netfilter/Kconfig                  |    9 +
 net/netfilter/Makefile                 |    1 +
 net/netfilter/xt_IDLETIMER.c           |  388 ++++++++++++++++++++++++++++++++
 4 files changed, 434 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/netfilter/xt_IDLETIMER.h
 create mode 100644 net/netfilter/xt_IDLETIMER.c

diff --git a/include/linux/netfilter/xt_IDLETIMER.h b/include/linux/netfilter/xt_IDLETIMER.h
new file mode 100644
index 0000000..f372c0c
--- /dev/null
+++ b/include/linux/netfilter/xt_IDLETIMER.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/linux/netfilter/xt_IDLETIMER.h
+ *
+ * Header file for Xtables timer target module.
+ *
+ * Copyright (C) 2004, 2010 Nokia Corporation
+ * Written by Timo Teras <ext-timo.teras@nokia.com>
+ *
+ * Converted to x_tables and forward-ported to 2.6.34
+ * by Luciano Coelho <luciano.coelho@nokia.com>
+ *
+ * Contact: Luciano Coelho <luciano.coelho@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef _XT_TIMER_H
+#define _XT_TIMER_H
+
+struct xt_idletimer_info {
+	unsigned int timeout;
+};
+
+#endif
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 8593a77..b5aa336 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -584,6 +584,15 @@ config NETFILTER_XT_TARGET_TCPOPTSTRIP
 	  This option adds a "TCPOPTSTRIP" target, which allows you to strip
 	  TCP options from TCP packets.
 
+config NETFILTER_XT_TARGET_IDLETIMER
+	tristate  "IDLETIMER target support"
+	help
+	  This option adds an `IDLETIMER' target. Each matching packet resets
+	  the timer associated with input and/or output interfaces. Timer
+	  expiry causes kobject uevent. Idle timer can be read via sysfs.
+
+	  To compile it as a module, choose M here.  If unsure, say N.
+
 # alphabetically ordered list of matches
 
 comment "Xtables matches"
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 14e3a8f..e28420a 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_TCPMSS) += xt_TCPMSS.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_TCPOPTSTRIP) += xt_TCPOPTSTRIP.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_TEE) += xt_TEE.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_TRACE) += xt_TRACE.o
+obj-$(CONFIG_NETFILTER_XT_TARGET_IDLETIMER) += xt_IDLETIMER.o
 
 # matches
 obj-$(CONFIG_NETFILTER_XT_MATCH_CLUSTER) += xt_cluster.o
diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
new file mode 100644
index 0000000..3be8e8e
--- /dev/null
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -0,0 +1,388 @@
+/*
+ * linux/net/netfilter/xt_IDLETIMER.c
+ *
+ * Netfilter module to trigger a timer when packet matches.
+ * After timer expires a kevent will be sent.
+ *
+ * Copyright (C) 2004, 2010 Nokia Corporation
+ * Written by Timo Teras <ext-timo.teras@nokia.com>
+ *
+ * Converted to x_tables and reworked for upstream inclusion
+ * by Luciano Coelho <luciano.coelho@nokia.com>
+ *
+ * Contact: Luciano Coelho <luciano.coelho@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <linux/module.h>
+#include <linux/timer.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/notifier.h>
+#include <linux/netfilter.h>
+#include <linux/rtnetlink.h>
+#include <linux/netfilter/x_tables.h>
+#include <linux/netfilter/xt_IDLETIMER.h>
+#include <linux/kobject.h>
+#include <linux/workqueue.h>
+#include <linux/netfilter_ipv4/ip_tables.h>
+
+static ssize_t utimer_attr_show(struct device *dev,
+				struct device_attribute *attr, char *buf);
+
+struct utimer_t {
+	char name[IFNAMSIZ];
+	struct net *net;
+
+	struct list_head entry;
+	struct timer_list timer;
+	struct work_struct work;
+
+	const struct xt_idletimer_info *info;
+};
+
+static LIST_HEAD(active_utimer_head);
+static DEFINE_SPINLOCK(list_lock);
+static DEVICE_ATTR(idletimer, 0444, utimer_attr_show, NULL);
+
+/* TODO: instead of finding by name, we need to iterate all with this name */
+static
+struct utimer_t *__utimer_find_by_name(const char *name, const struct net *net)
+{
+	struct utimer_t *entry;
+
+	list_for_each_entry(entry, &active_utimer_head, entry) {
+		if (!strcmp(name, entry->name) &&
+		    (net == entry->net)) {
+			return entry;
+		}
+	}
+
+	return NULL;
+}
+
+static
+struct utimer_t *__utimer_find_by_info(const struct xt_idletimer_info *info)
+{
+	struct utimer_t *entry;
+
+	list_for_each_entry(entry, &active_utimer_head, entry) {
+		if (info == entry->info) {
+			return entry;
+		}
+	}
+
+	return NULL;
+}
+
+static void utimer_delete(const struct xt_idletimer_info *info)
+{
+	struct utimer_t *timer;
+
+	spin_lock_bh(&list_lock);
+	timer = __utimer_find_by_info(info);
+	if (!timer)
+		goto out;
+
+	pr_debug("xt_idletimer: deleting timer '%s' ns %p\n",
+		 timer->name, timer->net);
+
+	list_del(&timer->entry);
+	del_timer_sync(&timer->timer);
+	put_net(timer->net);
+	kfree(timer);
+
+out:
+	spin_unlock_bh(&list_lock);
+}
+
+static void utimer_work(struct work_struct *work)
+{
+	struct utimer_t *timer = container_of(work, struct utimer_t, work);
+	struct net_device *netdev;
+
+	netdev = dev_get_by_name(timer->net, timer->name);
+
+	if (netdev != NULL) {
+		sysfs_notify(&netdev->dev.kobj, NULL,
+			     "idletimer");
+		dev_put(netdev);
+	}
+}
+
+static void utimer_expired(unsigned long data)
+{
+	struct utimer_t *timer = (struct utimer_t *) data;
+
+	pr_debug("xt_idletimer: timer '%s' ns %p expired\n",
+		 timer->name, timer->net);
+
+	schedule_work(&timer->work);
+}
+
+static struct utimer_t *utimer_create(const char *name,
+				      struct net *net,
+				      const struct xt_idletimer_info *info)
+{
+	struct utimer_t *timer;
+
+	timer = kmalloc(sizeof(struct utimer_t), GFP_ATOMIC);
+	if (timer == NULL)
+		return NULL;
+
+	list_add(&timer->entry, &active_utimer_head);
+	strlcpy(timer->name, name, sizeof(timer->name));
+	timer->net = get_net(net);
+
+	init_timer(&timer->timer);
+	setup_timer(&timer->timer, utimer_expired, (unsigned long) timer);
+
+	timer->info = info;
+
+	INIT_WORK(&timer->work, utimer_work);
+
+	pr_debug("xt_idletimer: created timer '%s' ns %p, timeout period %u\n",
+		 timer->name, timer->net, timer->info->timeout);
+
+	return timer;
+}
+
+static void utimer_reset(const struct xt_idletimer_info *info)
+{
+	struct utimer_t *timer;
+
+	spin_lock_bh(&list_lock);
+	timer = __utimer_find_by_info(info);
+	if (timer) {
+		pr_debug("xt_idletimer: resetting timer '%s' ns %p\n",
+			 timer->name, timer->net);
+		mod_timer(&timer->timer,
+			  msecs_to_jiffies(timer->info->timeout * 1000) + jiffies);
+	}
+	spin_unlock_bh(&list_lock);
+}
+
+static ssize_t utimer_attr_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct utimer_t *timer;
+	struct net_device *netdev = to_net_dev(dev);
+	unsigned long expires = 0;
+
+	spin_lock_bh(&list_lock);
+	timer = __utimer_find_by_name(netdev->name, dev_net(netdev));
+	if (timer)
+		expires = timer->timer.expires;
+	spin_unlock_bh(&list_lock);
+
+	if (expires > jiffies)
+		return sprintf(buf, "%u\n",
+			       jiffies_to_msecs(expires - jiffies) / 1000);
+
+	return sprintf(buf, "0\n");
+}
+
+static int utimer_notifier_call(struct notifier_block *this,
+				unsigned long event, void *ptr)
+{
+	struct net_device *netdev = ptr;
+	struct utimer_t *timer;
+	int ret;
+
+	/*
+	 * FIXME: Currently namespaces and sysfs don't coexist very nicely,
+	 * so we need to make sure that the kobject associated with this
+	 * device has a valid sysfs dirent in this namespace
+	 */
+	if (!netdev->dev.kobj.sd) {
+		pr_debug("xt_idletimer: event ignored: interface %s doesn't "
+			 "have an associated sd in this namespace.\n",
+			 netdev->name);
+		goto out;
+	}
+
+	spin_lock_bh(&list_lock);
+	timer = __utimer_find_by_name(netdev->name, dev_net(netdev));
+	spin_unlock_bh(&list_lock);
+	if (!timer)
+		goto out;
+
+	switch (event) {
+	case NETDEV_UP:
+		pr_debug("xt_idletimer: NETDEV_UP: %s\n", netdev->name);
+
+		ret = device_create_file(&netdev->dev,
+					 &dev_attr_idletimer);
+		WARN_ON(ret);
+
+		/*
+		 * TODO: need to loop over all timers associated with this
+		 * name and net and start their timers them
+		 */
+		utimer_reset(timer->info);
+
+		break;
+	case NETDEV_DOWN:
+		pr_debug("xt_idletimer: NETDEV_DOWN: %s\n", netdev->name);
+
+		device_remove_file(&netdev->dev,
+				   &dev_attr_idletimer);
+		/*
+		 * TODO: need to loop over all timers associated with this
+		 * name and net and stop their timers them
+		 */
+		del_timer_sync(&timer->timer);
+
+		break;
+	default:
+		break;
+	}
+
+out:
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block utimer_notifier_block = {
+	.notifier_call	= utimer_notifier_call,
+};
+
+
+static int utimer_init(void)
+{
+	return register_netdevice_notifier(&utimer_notifier_block);
+}
+
+static void utimer_fini(void)
+{
+	struct net_device *dev;
+	struct net *net;
+
+	unregister_netdevice_notifier(&utimer_notifier_block);
+	rtnl_lock();
+	for_each_net(net) {
+		for_each_netdev(net, dev) {
+			utimer_notifier_call(&utimer_notifier_block,
+					     NETDEV_DOWN, dev);
+		}
+	}
+	rtnl_unlock();
+}
+
+/*
+ * The actual xt_tables plugin.
+ */
+static unsigned int xt_idletimer_target(struct sk_buff *skb,
+					 const struct xt_action_param *par)
+{
+	const struct xt_idletimer_info *info = par->targinfo;
+
+	if (par->in != NULL)
+		utimer_reset(info);
+
+	if (par->out != NULL) {
+		utimer_reset(info);
+	}
+
+	return XT_CONTINUE;
+}
+
+static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
+{
+	const struct xt_idletimer_info *info = par->targinfo;
+	const struct ipt_entry *entryinfo = par->entryinfo;
+	const struct ipt_ip *ip = &entryinfo->ip;
+
+	pr_debug("xt_idletimer: checkentry targinfo %p\n", par->targinfo);
+
+	if (info->timeout == 0) {
+		pr_debug("xt_idletimer: timeout value is zero\n");
+		return -EINVAL;
+	}
+
+	/* FIXME: implement support for other protocol families */
+	switch (par->family) {
+	case NFPROTO_IPV4   :
+		pr_debug("xt_idletimer: NFPROTO_IPV4\n");
+		break;
+
+	default:
+		pr_debug("xt_idletimer: Unsupported protocol family family!\n");
+		return -EINVAL;
+	}
+
+	if (strlen(ip->iniface) == 0 && strlen(ip->outiface) == 0) {
+		pr_debug("xt_idletimer: in or out interface must "
+			 "be specified\n");
+		return -EINVAL;
+	}
+
+	if (utimer_create(ip->iniface, par->net, info) == NULL) {
+		pr_debug("xt_idletimer: failed to create timer\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void xt_idletimer_destroy(const struct xt_tgdtor_param *par)
+{
+	const struct xt_idletimer_info *info = par->targinfo;
+
+	pr_debug("xt_idletimer: destroy targinfo %p\n", par->targinfo);
+
+	utimer_delete(info);
+}
+
+static struct xt_target xt_idletimer = {
+	.name		= "IDLETIMER",
+	.family		= NFPROTO_IPV4,
+	.target		= xt_idletimer_target,
+	.targetsize     = sizeof(struct xt_idletimer_info),
+	.checkentry	= xt_idletimer_checkentry,
+	.destroy        = xt_idletimer_destroy,
+	.me		= THIS_MODULE,
+};
+
+static int __init init(void)
+{
+	int ret;
+
+	ret = utimer_init();
+	if (ret)
+		return ret;
+
+	ret =  xt_register_target(&xt_idletimer);
+	if (ret < 0) {
+		utimer_fini();
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit fini(void)
+{
+	xt_unregister_target(&xt_idletimer);
+	utimer_fini();
+}
+
+module_init(init);
+module_exit(fini);
+
+MODULE_AUTHOR("Timo Teras <ext-timo.teras@nokia.com>");
+MODULE_AUTHOR("Luciano Coelho <luciano.coelho@nokia.com>");
+MODULE_DESCRIPTION("Xtables idletimer target module");
+MODULE_LICENSE("GPL");
-- 
1.6.3.3

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Jan Engelhardt]

On Thursday 2010-05-27 22:54, Luciano Coelho wrote:

>This patch implements an idletimer Xtables target that can be used to
>identify when interfaces have been idle for a certain period of time.
>
>It adds a file to the sysfs for each interface that is brought up.  The file
>contains the time remaining before the event is triggered.
>
>The timeout should be set when the Xtable rule is defined with the --timeout
>parameter set.
>
>There are still some issues to be resolved:
>
>How to treat several rules for the same interface?
>
>We need a key for the timer list.  I'm using the targinfo pointer for that,
>but this looks shaky, because there is no assurance that this pointer will
>live for the entire lifetime of the rule.

Well xt_quota for example has its targinfo around at all times,
as have other modules.

>This is now an x_tables target, so other protocols need to be implemented.

Huh? You're not looking at packets, so why does it need proto-specific
stuff?

>+static
>+struct utimer_t *__utimer_find_by_name(const char *name, const struct net *net)
>+{
>+	struct utimer_t *entry;
>+
>+	list_for_each_entry(entry, &active_utimer_head, entry) {
>+		if (!strcmp(name, entry->name) &&
>+		    (net == entry->net)) {
>+			return entry;
>+		}
>+	}
>+
>+	return NULL;
>+}
>+
>+static
>+struct utimer_t *__utimer_find_by_info(const struct xt_idletimer_info *info)
>+{
>+	struct utimer_t *entry;
>+
>+	list_for_each_entry(entry, &active_utimer_head, entry) {
>+		if (info == entry->info) {
>+			return entry;
>+		}
>+	}
>+
>+	return NULL;
>+}

Can do with less braces.

>+static void utimer_expired(unsigned long data)
>+{
>+	struct utimer_t *timer = (struct utimer_t *) data;
>+
>+	pr_debug("xt_idletimer: timer '%s' ns %p expired\n",
>+		 timer->name, timer->net);
>+
>+	schedule_work(&timer->work);
>+}

You don't need xt_idletimer, because pr_debug already prints
that (with #define pr_fmt(fmt) KBUILD_MODNAME ": " as many
other modules do)

>+
>+static struct utimer_t *utimer_create(const char *name,
>+				      struct net *net,
>+				      const struct xt_idletimer_info *info)
>+{
>+	struct utimer_t *timer;
>+
>+	timer = kmalloc(sizeof(struct utimer_t), GFP_ATOMIC);
>+	if (timer == NULL)
>+		return NULL;

This is called from user context, so GFP_KERNEL will perfectly suffice.

>+static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
>+{
>+	const struct xt_idletimer_info *info = par->targinfo;
>+	const struct ipt_entry *entryinfo = par->entryinfo;
>+	const struct ipt_ip *ip = &entryinfo->ip;

I'm not sure spying on ipt_ip is a long-term viable solution.

>+	pr_debug("xt_idletimer: checkentry targinfo %p\n", par->targinfo);
>+
>+	if (info->timeout == 0) {
>+		pr_debug("xt_idletimer: timeout value is zero\n");
>+		return -EINVAL;
>+	}
>+
>+	/* FIXME: implement support for other protocol families */
>+	switch (par->family) {
>+	case NFPROTO_IPV4   :
>+		pr_debug("xt_idletimer: NFPROTO_IPV4\n");
>+		break;
>+
>+	default:
>+		pr_debug("xt_idletimer: Unsupported protocol family family!\n");
>+		return -EINVAL;
>+	}
>+
>+	if (strlen(ip->iniface) == 0 && strlen(ip->outiface) == 0) {
>+		pr_debug("xt_idletimer: in or out interface must "
>+			 "be specified\n");
>+		return -EINVAL;
>+	}
>+
>+	if (utimer_create(ip->iniface, par->net, info) == NULL) {
>+		pr_debug("xt_idletimer: failed to create timer\n");
>+		return -ENOMEM;
>+	}

What about outiface?
What blows up when iniface is empty?

>+	return 0;
>+}
>+
>+static void xt_idletimer_destroy(const struct xt_tgdtor_param *par)
>+{
>+	const struct xt_idletimer_info *info = par->targinfo;
>+
>+	pr_debug("xt_idletimer: destroy targinfo %p\n", par->targinfo);
>+
>+	utimer_delete(info);
>+}
>+
>+static int __init init(void)
>+{
>+	int ret;
>+
>+	ret = utimer_init();
>+	if (ret)
>+		return ret;
>+
>+	ret =  xt_register_target(&xt_idletimer);
>+	if (ret < 0) {
>+		utimer_fini();
>+		return ret;
>+	}
>+
>+	return 0;
>+}
>+
>+static void __exit fini(void)
>+{
>+	xt_unregister_target(&xt_idletimer);
>+	utimer_fini();
>+}
>+
>+module_init(init);
>+module_exit(fini);

Call it just exit?
Also give the functions better names (see other modules), that is going
to be unrecognizable in stacktraces otherwise.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

Hi Jan,

Thanks a lot for your comments!

On Fri, 2010-05-28 at 01:17 +0200, ext Jan Engelhardt wrote:
> On Thursday 2010-05-27 22:54, Luciano Coelho wrote:
> >There are still some issues to be resolved:
> >
> >How to treat several rules for the same interface?
> >
> >We need a key for the timer list.  I'm using the targinfo pointer for that,
> >but this looks shaky, because there is no assurance that this pointer will
> >live for the entire lifetime of the rule.
> 
> Well xt_quota for example has its targinfo around at all times,
> as have other modules.

Great, so this will work.  I had checked the x_tables code and it seemed
that the lifetime of targinfo was sufficient, but I was not sure this
would be safe in the future.  Now, if this changes in the future, my
module won't be the only one to break ;)


> >This is now an x_tables target, so other protocols need to be implemented.
> 
> Huh? You're not looking at packets, so why does it need proto-specific
> stuff?

I need to associate the timers with specific interfaces, because it's
the idle time of the interfaces that the userspace in interested in.  I
didn't find any other way to associate the timers with them, except by
looking at the iniface and outiface values in ipt_ip (and eventually,
with IPv6 properly implemented, in ip6t_ip6).  This is what Patrick
suggested when he checked my previous patch [1] and triggered me to do a
major rework on my module ;)

Do you have any other suggestion on how I can associate the rules to
specific interfaces?


> >+static
> >+struct utimer_t *__utimer_find_by_info(const struct xt_idletimer_info *info)
> >+{
> >+	struct utimer_t *entry;
> >+
> >+	list_for_each_entry(entry, &active_utimer_head, entry) {
> >+		if (info == entry->info) {
> >+			return entry;
> >+		}
> >+	}
> >+
> >+	return NULL;
> >+}
> 
> Can do with less braces.

Sure.  These remained there after I removed some traces.  I'll clean
this up.


> >+static void utimer_expired(unsigned long data)
> >+{
> >+	struct utimer_t *timer = (struct utimer_t *) data;
> >+
> >+	pr_debug("xt_idletimer: timer '%s' ns %p expired\n",
> >+		 timer->name, timer->net);
> >+
> >+	schedule_work(&timer->work);
> >+}
> 
> You don't need xt_idletimer, because pr_debug already prints
> that (with #define pr_fmt(fmt) KBUILD_MODNAME ": " as many
> other modules do)

Ok, I'll change it.  Thanks for pointing out.


> >+
> >+static struct utimer_t *utimer_create(const char *name,
> >+				      struct net *net,
> >+				      const struct xt_idletimer_info *info)
> >+{
> >+	struct utimer_t *timer;
> >+
> >+	timer = kmalloc(sizeof(struct utimer_t), GFP_ATOMIC);
> >+	if (timer == NULL)
> >+		return NULL;
> 
> This is called from user context, so GFP_KERNEL will perfectly suffice.

Yup, will change.


> >+static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
> >+{
> >+	const struct xt_idletimer_info *info = par->targinfo;
> >+	const struct ipt_entry *entryinfo = par->entryinfo;
> >+	const struct ipt_ip *ip = &entryinfo->ip;
> 
> I'm not sure spying on ipt_ip is a long-term viable solution.

Do you have any other suggestions on how I could get an interface
associated with the rule? I thought about having the userspace pass the
interface as an option to the rule (like I already do for the timeout
value), but that looked ugly to me, since the interface can already be
defined as part of the ruleset.


> >+	pr_debug("xt_idletimer: checkentry targinfo %p\n", par->targinfo);
> >+
> >+	if (info->timeout == 0) {
> >+		pr_debug("xt_idletimer: timeout value is zero\n");
> >+		return -EINVAL;
> >+	}
> >+
> >+	/* FIXME: implement support for other protocol families */
> >+	switch (par->family) {
> >+	case NFPROTO_IPV4   :
> >+		pr_debug("xt_idletimer: NFPROTO_IPV4\n");
> >+		break;
> >+
> >+	default:
> >+		pr_debug("xt_idletimer: Unsupported protocol family family!\n");
> >+		return -EINVAL;
> >+	}
> >+
> >+	if (strlen(ip->iniface) == 0 && strlen(ip->outiface) == 0) {
> >+		pr_debug("xt_idletimer: in or out interface must "
> >+			 "be specified\n");
> >+		return -EINVAL;
> >+	}
> >+
> >+	if (utimer_create(ip->iniface, par->net, info) == NULL) {
> >+		pr_debug("xt_idletimer: failed to create timer\n");
> >+		return -ENOMEM;
> >+	}
> 
> What about outiface?
> What blows up when iniface is empty?

Ooops! I've redone this part of the code so many times and in this
version I completely forgot to include the outiface.  I'll fix it.


> >+static void xt_idletimer_destroy(const struct xt_tgdtor_param *par)
> >+{
> >+	const struct xt_idletimer_info *info = par->targinfo;
> >+
> >+	pr_debug("xt_idletimer: destroy targinfo %p\n", par->targinfo);
> >+
> >+	utimer_delete(info);
> >+}
> >+
> >+static int __init init(void)
> >+{
> >+	int ret;
> >+
> >+	ret = utimer_init();
> >+	if (ret)
> >+		return ret;
> >+
> >+	ret =  xt_register_target(&xt_idletimer);
> >+	if (ret < 0) {
> >+		utimer_fini();
> >+		return ret;
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static void __exit fini(void)
> >+{
> >+	xt_unregister_target(&xt_idletimer);
> >+	utimer_fini();
> >+}
> >+
> >+module_init(init);
> >+module_exit(fini);
> 
> Call it just exit?

Yes.


> Also give the functions better names (see other modules), that is going
> to be unrecognizable in stacktraces otherwise.

I agree.  These names are coming from the original code.  I thought
about changing them to something clearer, but didn't bother to do it
yet, because I was focusing on the actual functionality.  I'll change
the names.

Again, thanks for your comments.  I'll rework and submit v2 soon.

Ah, and please excuse my lameness of mistyping the netdev email address
when I submitted the patch.  I fixed it now.

[1]
http://article.gmane.org/gmane.comp.security.firewalls.netfilter.devel/33934


-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Jan Engelhardt]

On Friday 2010-05-28 07:25, Luciano Coelho wrote:
>
>Do you have any other suggestion on how I can associate the rules to
>specific interfaces?

-A INPUT -i foo -j do
-A do -j idletimer

A little funny, but actually this would allow me to keep a timer
for a group of interfaces rather than just per-if.

>> >+static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
>> >+{
>> >+	const struct xt_idletimer_info *info = par->targinfo;
>> >+	const struct ipt_entry *entryinfo = par->entryinfo;
>> >+	const struct ipt_ip *ip = &entryinfo->ip;
>> 
>> I'm not sure spying on ipt_ip is a long-term viable solution.
>
>Do you have any other suggestions on how I could get an interface
>associated with the rule? I thought about having the userspace pass the
>interface as an option to the rule (like I already do for the timeout
>value), but that looked ugly to me, since the interface can already be
>defined as part of the ruleset.

I have patches ready since a while that decouple ipt_ip
from a rule, so there is no guarantee that such will exist.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Fri, 2010-05-28 at 10:05 +0200, ext Jan Engelhardt wrote:
> On Friday 2010-05-28 07:25, Luciano Coelho wrote:
> >
> >Do you have any other suggestion on how I can associate the rules to
> >specific interfaces?
> 
> -A INPUT -i foo -j do
> -A do -j idletimer
> 
> A little funny, but actually this would allow me to keep a timer
> for a group of interfaces rather than just per-if.

Yes, this is what our userspace apps are doing.  I've formulated my
question in an unclear way.  If you check the rest of the code, I create
sysfs files under the interface's directory and use it as an attribute
to notify the userspace when the timer has expired.

In short, I need to figure out a way to associate each rule with an
interface in sysfs, so I can notify the userspace when the timer has
expired.  I couldn't figure out another way to do it.  Any suggestions?


> >> >+static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
> >> >+{
> >> >+	const struct xt_idletimer_info *info = par->targinfo;
> >> >+	const struct ipt_entry *entryinfo = par->entryinfo;
> >> >+	const struct ipt_ip *ip = &entryinfo->ip;
> >> 
> >> I'm not sure spying on ipt_ip is a long-term viable solution.
> >
> >Do you have any other suggestions on how I could get an interface
> >associated with the rule? I thought about having the userspace pass the
> >interface as an option to the rule (like I already do for the timeout
> >value), but that looked ugly to me, since the interface can already be
> >defined as part of the ruleset.
> 
> I have patches ready since a while that decouple ipt_ip
> from a rule, so there is no guarantee that such will exist.

Okay, if that's the case, then I don't know how to associate the rule
with a specific net object in the kobject tree.  Maybe I have to figure
out a different way to notify the userspace, unless I add the target
option I mentioned above. :/


-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Patrick McHardy]

Luciano Coelho wrote:
> On Fri, 2010-05-28 at 10:05 +0200, ext Jan Engelhardt wrote:
>> On Friday 2010-05-28 07:25, Luciano Coelho wrote:
>>> Do you have any other suggestion on how I can associate the rules to
>>> specific interfaces?
>> -A INPUT -i foo -j do
>> -A do -j idletimer
>>
>> A little funny, but actually this would allow me to keep a timer
>> for a group of interfaces rather than just per-if.
> 
> Yes, this is what our userspace apps are doing.  I've formulated my
> question in an unclear way.  If you check the rest of the code, I create
> sysfs files under the interface's directory and use it as an attribute
> to notify the userspace when the timer has expired.
> 
> In short, I need to figure out a way to associate each rule with an
> interface in sysfs, so I can notify the userspace when the timer has
> expired.  I couldn't figure out another way to do it.  Any suggestions?

How about just using an arbitrary user-supplied name? People can
name them after interfaces, or anything else.

>>>>> +static int xt_idletimer_checkentry(const struct xt_tgchk_param *par)
>>>>> +{
>>>>> +	const struct xt_idletimer_info *info = par->targinfo;
>>>>> +	const struct ipt_entry *entryinfo = par->entryinfo;
>>>>> +	const struct ipt_ip *ip = &entryinfo->ip;
>>>> I'm not sure spying on ipt_ip is a long-term viable solution.
>>> Do you have any other suggestions on how I could get an interface
>>> associated with the rule? I thought about having the userspace pass the
>>> interface as an option to the rule (like I already do for the timeout
>>> value), but that looked ugly to me, since the interface can already be
>>> defined as part of the ruleset.
>> I have patches ready since a while that decouple ipt_ip
>> from a rule, so there is no guarantee that such will exist.
> 
> Okay, if that's the case, then I don't know how to associate the rule
> with a specific net object in the kobject tree.  Maybe I have to figure
> out a different way to notify the userspace, unless I add the target
> option I mentioned above. :/
> 
> 

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Mon, 2010-05-31 at 17:59 +0200, ext Patrick McHardy wrote:
> Luciano Coelho wrote:
> > On Fri, 2010-05-28 at 10:05 +0200, ext Jan Engelhardt wrote:
> >> On Friday 2010-05-28 07:25, Luciano Coelho wrote:
> >>> Do you have any other suggestion on how I can associate the rules to
> >>> specific interfaces?
> >> -A INPUT -i foo -j do
> >> -A do -j idletimer
> >>
> >> A little funny, but actually this would allow me to keep a timer
> >> for a group of interfaces rather than just per-if.
> > 
> > Yes, this is what our userspace apps are doing.  I've formulated my
> > question in an unclear way.  If you check the rest of the code, I create
> > sysfs files under the interface's directory and use it as an attribute
> > to notify the userspace when the timer has expired.
> > 
> > In short, I need to figure out a way to associate each rule with an
> > interface in sysfs, so I can notify the userspace when the timer has
> > expired.  I couldn't figure out another way to do it.  Any suggestions?
> 
> How about just using an arbitrary user-supplied name? People can
> name them after interfaces, or anything else.

I considered this option, but then I didn't find a proper place where to
include the attribute in sysfs, since I cannot add it as part of the
interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.
The other option would be to make the idletimer as part of the
xt_IDLETIMER module object in sysfs
(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
place.  And I think adding it as /sys/class/net/idletimer is most likely
out of the question.

The latest "solution" I came up with, is to associate the idletimer with
every interface that it hits.  Whenever a packet arrives, I check which
interface it came from and add the timer to it (eg.
in /sys/class/net/wlan0/idletimer if the packet came via wlan0).  This
causes a bit extra processing per packet, but in most cases there
shouldn't be too many interfaces in the list, so the search should be
fairly quick.  And if performance becomes a problem, it can be worked
around by adding only one interface per ruleset, so the list will never
grow bigger than one node.

I think these two solutions would work.  I prefer the second one,
because we don't need to add the idletimer attribute in an artificial
place in sysfs.

The problem that remains with either solution is if the interface is
already idle when the rule created.  In that case, the timer won't start
(or at least will not be associated with that interface).  It will only
start when the first packet hits.  The only solution I see for this is
to add the interface name as an option to the target.  Maybe something
like "--autostart=wlan0"?

I'll send a new RFC patch soon with this ideas implemented, to better
express what I mean (C code can be easier to read than English :P)


-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Jan Engelhardt]

On Monday 2010-05-31 21:12, Luciano Coelho wrote:
>
>I considered this option, but then I didn't find a proper place where to
>include the attribute in sysfs, since I cannot add it as part of the
>interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.

You couldn't have done that before either, because the interface name
in ipt_ip may refer to an interface that does not exist at all times.

>The other option would be to make the idletimer as part of the
>xt_IDLETIMER module object in sysfs
>(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
>place.

I like it. It follows /proc/net/xt_{hashlimit,recent}/<user_supplied_name>.

>And I think adding it as /sys/class/net/idletimer is most likely
>out of the question.

It follows /sys/class/leds/...


I'm impartial though.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Mon, 2010-05-31 at 21:51 +0200, ext Jan Engelhardt wrote:
> On Monday 2010-05-31 21:12, Luciano Coelho wrote:
> >
> >I considered this option, but then I didn't find a proper place where to
> >include the attribute in sysfs, since I cannot add it as part of the
> >interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.
> 
> You couldn't have done that before either, because the interface name
> in ipt_ip may refer to an interface that does not exist at all times.

True.  That's why I was using netdevice_notifiers , so that I would
monitor the interface state and add the idletimer attribute when a timer
was associated with the interface that went up.  But now the rules are
not interface specific, so it cannot be done like that anymore.


> >The other option would be to make the idletimer as part of the
> >xt_IDLETIMER module object in sysfs
> >(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
> >place.
> 
> I like it. It follows /proc/net/xt_{hashlimit,recent}/<user_supplied_name>.
> 
> >And I think adding it as /sys/class/net/idletimer is most likely
> >out of the question.
> 
> It follows /sys/class/leds/...
> 
> 
> I'm impartial though.

Okay, so this can be done in either place.  I tend to
prefer /sys/class/net/idletimer.

What about my other proposal of creating generic timers and associating
them with certain interfaces whenever we get a hit? I mean, to add the
idletimer attribute to eg. /sys/class/net/wlan0/idletimer when a packet
reaches the target from wlan0?


-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Mon, 2010-05-31 at 22:11 +0200, Luciano Coelho wrote:
> What about my other proposal of creating generic timers and associating
> them with certain interfaces whenever we get a hit? I mean, to add the
> idletimer attribute to eg. /sys/class/net/wlan0/idletimer when a packet
> reaches the target from wlan0?

Let's forget about this other proposal.  It's not going to be efficient
at all.  It's worse than I thought at first, because we need search all
the associated_ifs in all timers whenever a packet is received.

I'll go for the other solution, which will make things much simpler and
I'll be able to remove the dependency on netdevice_notifiers
completely. :D

-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Mon, 2010-05-31 at 21:51 +0200, ext Jan Engelhardt wrote:
> On Monday 2010-05-31 21:12, Luciano Coelho wrote:
> >
> >I considered this option, but then I didn't find a proper place where to
> >include the attribute in sysfs, since I cannot add it as part of the
> >interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.
> 
> You couldn't have done that before either, because the interface name
> in ipt_ip may refer to an interface that does not exist at all times.
> 
> >The other option would be to make the idletimer as part of the
> >xt_IDLETIMER module object in sysfs
> >(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
> >place.
> 
> I like it. It follows /proc/net/xt_{hashlimit,recent}/<user_supplied_name>.

I'm starting to like this more and more too, as my code is getting much
smaller ;)

One quick question, though.  Do you have any ideas on how I can make
sure that the user doesn't supply the same name twice (ie. two rules
with the same user_supplied_name)?

The problem is that the userspace always re-adds all the existing rules
before inserting a new one and only later deletes the old rules. :\


-- 
Cheers,
Luca.

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Jan Engelhardt]

On Tuesday 2010-06-01 20:33, Luciano Coelho wrote:
>On Mon, 2010-05-31 at 21:51 +0200, ext Jan Engelhardt wrote:
>> On Monday 2010-05-31 21:12, Luciano Coelho wrote:
>> >
>> >I considered this option, but then I didn't find a proper place where to
>> >include the attribute in sysfs, since I cannot add it as part of the
>> >interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.
>> 
>> You couldn't have done that before either, because the interface name
>> in ipt_ip may refer to an interface that does not exist at all times.
>> 
>> >The other option would be to make the idletimer as part of the
>> >xt_IDLETIMER module object in sysfs
>> >(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
>> >place.
>> 
>> I like it. It follows /proc/net/xt_{hashlimit,recent}/<user_supplied_name>.
>
>I'm starting to like this more and more too, as my code is getting much
>smaller ;)
>
>One quick question, though.  Do you have any ideas on how I can make
>sure that the user doesn't supply the same name twice (ie. two rules
>with the same user_supplied_name)?

What's so bad about multiple rules being able to reset the timer?

Re: [RFC] netfilter: WIP: Xtables idletimer target implementation

[Luciano Coelho]

On Tue, 2010-06-01 at 20:38 +0200, ext Jan Engelhardt wrote:
> On Tuesday 2010-06-01 20:33, Luciano Coelho wrote:
> >On Mon, 2010-05-31 at 21:51 +0200, ext Jan Engelhardt wrote:
> >> On Monday 2010-05-31 21:12, Luciano Coelho wrote:
> >> >
> >> >I considered this option, but then I didn't find a proper place where to
> >> >include the attribute in sysfs, since I cannot add it as part of the
> >> >interface (eg. /sys/class/net/wlan0/idletimer) as I was doing before.
> >> 
> >> You couldn't have done that before either, because the interface name
> >> in ipt_ip may refer to an interface that does not exist at all times.
> >> 
> >> >The other option would be to make the idletimer as part of the
> >> >xt_IDLETIMER module object in sysfs
> >> >(ie. /sys/module/xt_IDLETIMER/<user_supplied_name>), but it looks out of
> >> >place.
> >> 
> >> I like it. It follows /proc/net/xt_{hashlimit,recent}/<user_supplied_name>.
> >
> >I'm starting to like this more and more too, as my code is getting much
> >smaller ;)
> >
> >One quick question, though.  Do you have any ideas on how I can make
> >sure that the user doesn't supply the same name twice (ie. two rules
> >with the same user_supplied_name)?
> 
> What's so bad about multiple rules being able to reset the timer?

Ahhmm... Nothing! Thank you, I think I'm getting code-blind :)

-- 
Cheers,
Luca.