| Message ID | 1275592445-15555-1-git-send-email-luciano.coelho@nokia.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello, On Thu, 2010-06-03 at 21:14 +0200, Coelho Luciano (Nokia-D/Helsinki) wrote: > From: Luciano Coelho <luciano.coelho@nokia.com> > > This patch implements an idletimer Xtables target that can be used to > identify when interfaces have been idle for a certain period of time. > > Timers are identified by labels and are created when a rule is set with a new > label. The rules also take a timeout value (in seconds) as an option. If > more than one rule uses the same timer label, the timer will be restarted > whenever any of the rules get a hit. > > One entry for each timer is created in sysfs. This attribute contains the > timer remaining for the timer to expire. The attributes are located under > the xt_idletimer class: > > /sys/class/xt_idletimer/timers/<label> > > When the timer expires, the target module sends a sysfs notification to the > userspace, which can then decide what to do (eg. disconnect to save power). > > Cc: Timo Teras <timo.teras@iki.fi> > Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com> > --- > v2: Fixed according to Jan's comments > v3: Changed to a device class in the virtual bus in sysfs > Removed unnecessary attribute group > Fixed missing deallocation in some error cases Does this patch look fine now or is there something more I need to do to get it applied? If it's okay already, are you waiting for the merge window to close before applying it? Sorry to bother you with this, but we're depending on this feature and I need to know if there's something more to be doen. I'll provide the extension for the iptables tool pretty soon.
luciano.coelho@nokia.com wrote: > From: Luciano Coelho <luciano.coelho@nokia.com> > > This patch implements an idletimer Xtables target that can be used to > identify when interfaces have been idle for a certain period of time. > > Timers are identified by labels and are created when a rule is set with a new > label. The rules also take a timeout value (in seconds) as an option. If > more than one rule uses the same timer label, the timer will be restarted > whenever any of the rules get a hit. > > One entry for each timer is created in sysfs. This attribute contains the > timer remaining for the timer to expire. The attributes are located under > the xt_idletimer class: > > /sys/class/xt_idletimer/timers/<label> > > When the timer expires, the target module sends a sysfs notification to the > userspace, which can then decide what to do (eg. disconnect to save power). > Basically this seems fine to me, some minor comments below. > +++ b/include/linux/netfilter/xt_IDLETIMER.h > @@ -0,0 +1,40 @@ > +#ifndef _XT_IDLETIMER_H > +#define _XT_IDLETIMER_H > + > +#define MAX_LABEL_SIZE 32 > This seems a bit generic, maybe better use MAX_IDLETIMER_LABER_SIZE. > + > +struct idletimer_tg_info { > + __u32 timeout; > + > + char label[MAX_LABEL_SIZE]; > +}; > + > +#endif > new file mode 100644 > index 0000000..65c195e > --- /dev/null > +++ b/net/netfilter/xt_IDLETIMER.c > @@ -0,0 +1,356 @@ > +/* > + * 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 > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/module.h> > +#include <linux/timer.h> > +#include <linux/list.h> > +#include <linux/spinlock.h> > +#include <linux/netfilter.h> > +#include <linux/netfilter/x_tables.h> > +#include <linux/netfilter/xt_IDLETIMER.h> > +#include <linux/kobject.h> > +#include <linux/workqueue.h> > +#include <linux/sysfs.h> > + > +struct idletimer_tg { > + struct list_head entry; > + struct timer_list timer; > + struct work_struct work; > + > + struct kobject *kobj; > + struct idletimer_tg_attr *attr; > + > + unsigned int refcnt; > +}; > + > +struct idletimer_tg_attr { > + struct attribute attr; > + ssize_t (*show)(struct kobject *kobj, > + struct attribute *attr, char *buf); > +}; > + > +static LIST_HEAD(idletimer_tg_list); > How does this work with multiple namespaces? It seems every namespace can bind to any timer. > +static DEFINE_SPINLOCK(list_lock); > + > +static struct kobject *idletimer_tg_kobj; > + > +static > +struct idletimer_tg *__idletimer_tg_find_by_label(const char *label) > +{ > + struct idletimer_tg *entry; > + > + BUG_ON(!label); > + > + list_for_each_entry(entry, &idletimer_tg_list, entry) { > + if (!strcmp(label, entry->attr->attr.name)) > + return entry; > + } > + > + return NULL; > +} > + > +static ssize_t idletimer_tg_show(struct kobject *kobj, struct attribute *attr, > + char *buf) > +{ > + struct idletimer_tg *timer; > + unsigned long expires = 0; > + > + spin_lock_bh(&list_lock); > + timer = __idletimer_tg_find_by_label(attr->name); > + if (timer) > + expires = timer->timer.expires; > + spin_unlock_bh(&list_lock); > + > + if (expires > jiffies) > time_after()? > + return sprintf(buf, "%u\n", > + jiffies_to_msecs(expires - jiffies) / 1000); > + > + return sprintf(buf, "0\n"); > +} > + > +static void idletimer_tg_delete(const struct idletimer_tg_info *info) > +{ > The only caller is the target cleanup function, why don't you just move everything there? > + struct idletimer_tg *timer; > + > + spin_lock_bh(&list_lock); > + timer = __idletimer_tg_find_by_label(info->label); > + if (!timer) { > + spin_unlock_bh(&list_lock); > + return; > + } > + > + if (--timer->refcnt == 0) { > + pr_debug("deleting timer %s\n", info->label); > + > + list_del(&timer->entry); > + del_timer_sync(&timer->timer); > + spin_unlock_bh(&list_lock); > + > + sysfs_remove_file(idletimer_tg_kobj, &timer->attr->attr); > + kfree(timer->attr->attr.name); > + kfree(timer->attr); > + kfree(timer); > + } else { > + spin_unlock_bh(&list_lock); > + pr_debug("decreased refcnt of timer %s to %u\n", > + info->label, timer->refcnt); > + } > +} > + > +static void idletimer_tg_work(struct work_struct *work) > +{ > + struct idletimer_tg *timer = container_of(work, struct idletimer_tg, > + work); > + > + sysfs_notify(idletimer_tg_kobj, NULL, > + timer->attr->attr.name); > +} > + > +static void idletimer_tg_expired(unsigned long data) > +{ > + struct idletimer_tg *timer = (struct idletimer_tg *) data; > + > + pr_debug("timer %s expired\n", > + timer->attr->attr.name); > + > + schedule_work(&timer->work); > +} > + > +static > +struct idletimer_tg *idletimer_tg_create(const struct idletimer_tg_info *info) > +{ > + struct idletimer_tg *timer; > + struct idletimer_tg_attr *attr; > + > + attr = kzalloc(sizeof(attr), GFP_KERNEL); > sizeof(*attr) > + if (!attr) { > + pr_debug("couldn't alloc attribute\n"); > + return NULL; > + } > + > + attr->attr.name = kstrdup(info->label, GFP_KERNEL); > + if (!attr->attr.name) { > + pr_debug("couldn't alloc attribute name\n"); > + goto out_free_attr; > + } > + attr->attr.mode = S_IRUGO; > + attr->show = idletimer_tg_show; > + > + if (sysfs_create_file(idletimer_tg_kobj, &attr->attr)) { > + pr_debug("couldn't add attr to sysfs\n"); > + goto out_free_name; > + } > + > + timer = kmalloc(sizeof(struct idletimer_tg), GFP_KERNEL); > + if (!timer) { > + pr_debug("couldn't alloc timer\n"); > + goto out_free_file; > + } > + > + spin_lock_bh(&list_lock); > + list_add(&timer->entry, &idletimer_tg_list); > + > + init_timer(&timer->timer); > + setup_timer(&timer->timer, idletimer_tg_expired, (unsigned long) timer); > + mod_timer(&timer->timer, > + msecs_to_jiffies(info->timeout * 1000) + jiffies); > + > + timer->attr = attr; > + timer->refcnt = 0; > Better fully set up the timer before activating it. > + > + INIT_WORK(&timer->work, idletimer_tg_work); > + spin_unlock_bh(&list_lock); > + > + return timer; > + > +out_free_file: > + sysfs_remove_file(idletimer_tg_kobj, &attr->attr); > +out_free_name: > + kfree(attr->attr.name); > +out_free_attr: > + kfree(attr); > + return NULL; > +} > + > +static void idletimer_tg_cleanup(void) > +{ > + struct idletimer_tg *timer; > + > + spin_lock(&list_lock); > + list_for_each_entry(timer, &idletimer_tg_list, entry) { > list_for_each_entry_safe(). This function seems unnecessary though, the module can't be unloaded while its still in use and cleanup will already happen when the rules are removed. > + pr_debug("deleting timer %s\n", timer->attr->attr.name); > + > + list_del(&timer->entry); > + del_timer_sync(&timer->timer); > + kfree(timer->attr->attr.name); > + kfree(timer->attr); > + kfree(timer); > + } > + spin_unlock(&list_lock); > +} > + > +/* > + * The actual xt_tables plugin. > + */ > +static unsigned int idletimer_tg_target(struct sk_buff *skb, > + const struct xt_action_param *par) > +{ > + const struct idletimer_tg_info *info = par->targinfo; > + struct idletimer_tg *timer; > + > + pr_debug("resetting timer %s, timeout period %u\n", > + info->label, info->timeout); > + > + spin_lock(&list_lock); > You need BH protection here as well for the output path. > + timer = __idletimer_tg_find_by_label(info->label); > + > + BUG_ON(!timer); > + > + mod_timer(&timer->timer, > + msecs_to_jiffies(info->timeout * 1000) + jiffies); > + spin_unlock(&list_lock); > + > + return XT_CONTINUE; > +} > + > +static int idletimer_tg_checkentry(const struct xt_tgchk_param *par) > +{ > + const struct idletimer_tg_info *info = par->targinfo; > + struct idletimer_tg *timer; > + > + pr_debug("checkentry targinfo %s\n", info->label); > + > + if (info->timeout == 0) { > + pr_debug("timeout value is zero\n"); > + return -EINVAL; > + } > + > + if (!info->label || strlen(info->label) == 0) { > !info->label is impossible. You should check for \0 termination instead. > + pr_debug("label is missing\n"); > + return -EINVAL; > + } > + > + spin_lock(&list_lock); > spin_lock_bh() > + timer = __idletimer_tg_find_by_label(info->label); > + if (!timer) { > + spin_unlock(&list_lock); > + timer = idletimer_tg_create(info); > How does this prevent creating the same timer twice? > + if (!timer) { > + pr_debug("failed to create timer\n"); > + return -ENOMEM; > + } > + spin_lock(&list_lock); > + } > + > + timer->refcnt++; > + mod_timer(&timer->timer, > + msecs_to_jiffies(info->timeout * 1000) + jiffies); > + spin_unlock(&list_lock); > + > + return 0; > +} > + > +static void idletimer_tg_destroy(const struct xt_tgdtor_param *par) > +{ > + const struct idletimer_tg_info *info = par->targinfo; > + > + pr_debug("destroy targinfo %s\n", info->label); > + > + idletimer_tg_delete(info); > +} > + > +static struct xt_target idletimer_tg __read_mostly = { > + .name = "IDLETIMER", > + .family = NFPROTO_UNSPEC, > + .target = idletimer_tg_target, > + .targetsize = sizeof(struct idletimer_tg_info), > + .checkentry = idletimer_tg_checkentry, > + .destroy = idletimer_tg_destroy, > + .me = THIS_MODULE, > +}; > + > +static struct class *idletimer_tg_class; > + > +static struct device *idletimer_tg_device; > + > +static int __init idletimer_tg_init(void) > +{ > + int err; > + > + idletimer_tg_class = class_create(THIS_MODULE, "xt_idletimer"); > + err = PTR_ERR(idletimer_tg_class); > + if (IS_ERR(idletimer_tg_class)) { > + pr_debug("couldn't register device class\n"); > + goto out; > + } > + > + idletimer_tg_device = device_create(idletimer_tg_class, NULL, > + MKDEV(0, 0), NULL, "timers"); > + err = PTR_ERR(idletimer_tg_device); > + if (IS_ERR(idletimer_tg_device)) { > + pr_debug("couldn't register system device\n"); > + goto out_class; > + } > + > + idletimer_tg_kobj = &idletimer_tg_device->kobj; > + > + err = xt_register_target(&idletimer_tg); > + if (err < 0) { > + pr_debug("couldn't register xt target\n"); > + goto out_dev; > + } > + > + return 0; > +out_dev: > + device_destroy(idletimer_tg_class, MKDEV(0, 0)); > +out_class: > + class_destroy(idletimer_tg_class); > +out: > + return err; > +} > + > +static void __exit idletimer_tg_exit(void) > +{ > + xt_unregister_target(&idletimer_tg); > + > + device_destroy(idletimer_tg_class, MKDEV(0, 0)); > + class_destroy(idletimer_tg_class); > + > + idletimer_tg_cleanup(); > +} > + > +module_init(idletimer_tg_init); > +module_exit(idletimer_tg_exit); > + > +MODULE_AUTHOR("Timo Teras <ext-timo.teras@nokia.com>"); > +MODULE_AUTHOR("Luciano Coelho <luciano.coelho@nokia.com>"); > +MODULE_DESCRIPTION("Xtables: idle time monitor"); > +MODULE_LICENSE("GPL v2"); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-06-09 at 15:45 +0200, ext Patrick McHardy wrote: > luciano.coelho@nokia.com wrote: > > From: Luciano Coelho <luciano.coelho@nokia.com> > > > > This patch implements an idletimer Xtables target that can be used to > > identify when interfaces have been idle for a certain period of time. > > > > Timers are identified by labels and are created when a rule is set with a new > > label. The rules also take a timeout value (in seconds) as an option. If > > more than one rule uses the same timer label, the timer will be restarted > > whenever any of the rules get a hit. > > > > One entry for each timer is created in sysfs. This attribute contains the > > timer remaining for the timer to expire. The attributes are located under > > the xt_idletimer class: > > > > /sys/class/xt_idletimer/timers/<label> > > > > When the timer expires, the target module sends a sysfs notification to the > > userspace, which can then decide what to do (eg. disconnect to save power). > > > > Basically this seems fine to me, some minor comments below. Thanks a lot for reviewing this again. My replies below. > > +++ b/include/linux/netfilter/xt_IDLETIMER.h > > @@ -0,0 +1,40 @@ > > +#ifndef _XT_IDLETIMER_H > > +#define _XT_IDLETIMER_H > > + > > +#define MAX_LABEL_SIZE 32 > > > > This seems a bit generic, maybe better use MAX_IDLETIMER_LABER_SIZE. True, I'll change it. > > +struct idletimer_tg { > > + struct list_head entry; > > + struct timer_list timer; > > + struct work_struct work; > > + > > + struct kobject *kobj; > > + struct idletimer_tg_attr *attr; > > + > > + unsigned int refcnt; > > +}; > > + > > +struct idletimer_tg_attr { > > + struct attribute attr; > > + ssize_t (*show)(struct kobject *kobj, > > + struct attribute *attr, char *buf); > > +}; > > + > > +static LIST_HEAD(idletimer_tg_list); > > > > How does this work with multiple namespaces? It seems every namespace > can bind to any timer. I was implementing this solution for multiple namespaces (see the previous versions of my patch), but the code started getting really complicated. Then I found out that sysfs and multiple namespaces are not working very well together yet and decided to drop it for the time being. Of course this doesn't matter anymore, since the timers belong to an independent class in sysfs, so I can easily add multiple namespace support by adding struct net *net as part of the list key together with the label. Do you think it's okay to leave it like this for now and extend it for multiple namespace support with a future patch? > > +static ssize_t idletimer_tg_show(struct kobject *kobj, struct attribute *attr, > > + char *buf) > > +{ > > + struct idletimer_tg *timer; > > + unsigned long expires = 0; > > + > > + spin_lock_bh(&list_lock); > > + timer = __idletimer_tg_find_by_label(attr->name); > > + if (timer) > > + expires = timer->timer.expires; > > + spin_unlock_bh(&list_lock); > > + > > + if (expires > jiffies) > > > > time_after()? Of course! > > + return sprintf(buf, "%u\n", > > + jiffies_to_msecs(expires - jiffies) / 1000); > > + > > + return sprintf(buf, "0\n"); > > +} > > + > > +static void idletimer_tg_delete(const struct idletimer_tg_info *info) > > +{ > > > > The only caller is the target cleanup function, why don't you just move > everything there? Good point. It probably remained as a separate function as vestigial code. > > +static > > +struct idletimer_tg *idletimer_tg_create(const struct idletimer_tg_info *info) > > +{ > > + struct idletimer_tg *timer; > > + struct idletimer_tg_attr *attr; > > + > > + attr = kzalloc(sizeof(attr), GFP_KERNEL); > > > > sizeof(*attr) Ugh! Nice catch, I'll fix it. > > + if (!attr) { > > + pr_debug("couldn't alloc attribute\n"); > > + return NULL; > > + } > > + > > + attr->attr.name = kstrdup(info->label, GFP_KERNEL); > > + if (!attr->attr.name) { > > + pr_debug("couldn't alloc attribute name\n"); > > + goto out_free_attr; > > + } > > + attr->attr.mode = S_IRUGO; > > + attr->show = idletimer_tg_show; > > + > > + if (sysfs_create_file(idletimer_tg_kobj, &attr->attr)) { > > + pr_debug("couldn't add attr to sysfs\n"); > > + goto out_free_name; > > + } > > + > > + timer = kmalloc(sizeof(struct idletimer_tg), GFP_KERNEL); I will also change this to kmalloc(sizeof(*timer), GFP_KERNEL) for consistency. > > + if (!timer) { > > + pr_debug("couldn't alloc timer\n"); > > + goto out_free_file; > > + } > > + > > + spin_lock_bh(&list_lock); > > + list_add(&timer->entry, &idletimer_tg_list); > > + > > + init_timer(&timer->timer); > > + setup_timer(&timer->timer, idletimer_tg_expired, (unsigned long) timer); > > + mod_timer(&timer->timer, > > + msecs_to_jiffies(info->timeout * 1000) + jiffies); > > + > > + timer->attr = attr; > > + timer->refcnt = 0; > > > > Better fully set up the timer before activating it. Ok. Also, I don't need init_timer() here, since setup_timer() already calls it. > > + > > + INIT_WORK(&timer->work, idletimer_tg_work); > > + spin_unlock_bh(&list_lock); > > + > > + return timer; > > + > > +out_free_file: > > + sysfs_remove_file(idletimer_tg_kobj, &attr->attr); > > +out_free_name: > > + kfree(attr->attr.name); > > +out_free_attr: > > + kfree(attr); > > + return NULL; > > +} > > + > > +static void idletimer_tg_cleanup(void) > > +{ > > + struct idletimer_tg *timer; > > + > > + spin_lock(&list_lock); > > + list_for_each_entry(timer, &idletimer_tg_list, entry) { > > > > list_for_each_entry_safe(). This function seems unnecessary though, the > module > can't be unloaded while its still in use and cleanup will already happen > when the > rules are removed. Right. I'll remove the cleanup function, since it's unnecessary. > > +static unsigned int idletimer_tg_target(struct sk_buff *skb, > > + const struct xt_action_param *par) > > +{ > > + const struct idletimer_tg_info *info = par->targinfo; > > + struct idletimer_tg *timer; > > + > > + pr_debug("resetting timer %s, timeout period %u\n", > > + info->label, info->timeout); > > + > > + spin_lock(&list_lock); > > > > You need BH protection here as well for the output path. True. I'll add it. > > +static int idletimer_tg_checkentry(const struct xt_tgchk_param *par) > > +{ > > + const struct idletimer_tg_info *info = par->targinfo; > > + struct idletimer_tg *timer; > > + > > + pr_debug("checkentry targinfo %s\n", info->label); > > + > > + if (info->timeout == 0) { > > + pr_debug("timeout value is zero\n"); > > + return -EINVAL; > > + } > > + > > + if (!info->label || strlen(info->label) == 0) { > > > > !info->label is impossible. You should check for \0 termination instead. Ooops! <blush> I'll fix it. > > > + pr_debug("label is missing\n"); > > + return -EINVAL; > > + } > > + > > + spin_lock(&list_lock); > > > spin_lock_bh() Yes, I'll add BH here too. > > + timer = __idletimer_tg_find_by_label(info->label); > > + if (!timer) { > > + spin_unlock(&list_lock); > > + timer = idletimer_tg_create(info); > > > > How does this prevent creating the same timer twice? The timer will only be created if __idletimer_tg_find_by_label() returns NULL, which means that no timer with that label has been found. "info" won't be the same if info->label is different, right? Or can it change on the fly? I'll submit v4 with these changes later this evening.
On Wednesday 2010-06-09 17:11, Luciano Coelho wrote: >> >> How does this work with multiple namespaces? It seems every namespace >> can bind to any timer. > >I was implementing this solution for multiple namespaces (see the >previous versions of my patch), but the code started getting really >complicated. Then I found out that sysfs and multiple namespaces are >not working very well together yet and decided to drop it for the time >being. Of course this doesn't matter anymore, since the timers belong >to an independent class in sysfs, so I can easily add multiple namespace >support by adding struct net *net as part of the list key together with >the label. > >Do you think it's okay to leave it like this for now and extend it for >multiple namespace support with a future patch? Yes. Least thing we need is one humongous patch. :) >> > + timer = __idletimer_tg_find_by_label(info->label); >> > + if (!timer) { >> > + spin_unlock(&list_lock); >> > + timer = idletimer_tg_create(info); >> > >> >> How does this prevent creating the same timer twice? > >The timer will only be created if __idletimer_tg_find_by_label() returns >NULL, which means that no timer with that label has been found. "info" >won't be the same if info->label is different, right? Or can it change >on the fly? One thing to be generally aware about is that things could potentially be instantiated by another entity between the time a label was looked up with negative result and the time one tries to add it. It may thus be required to extend keeping the lock until after idletimer_tg_create, in other words, lookup and create must be atomic to the rest of the world. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jan, On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: > On Wednesday 2010-06-09 17:11, Luciano Coelho wrote: > >Do you think it's okay to leave it like this for now and extend it for > >multiple namespace support with a future patch? > > Yes. Least thing we need is one humongous patch. :) Thanks! That was my concern too. One huge and very complex patch is not a nice thing to have. ;) > >> > + timer = __idletimer_tg_find_by_label(info->label); > >> > + if (!timer) { > >> > + spin_unlock(&list_lock); > >> > + timer = idletimer_tg_create(info); > >> > > >> > >> How does this prevent creating the same timer twice? > > > >The timer will only be created if __idletimer_tg_find_by_label() returns > >NULL, which means that no timer with that label has been found. "info" > >won't be the same if info->label is different, right? Or can it change > >on the fly? > > One thing to be generally aware about is that things could potentially > be instantiated by another entity between the time a label was looked up > with negative result and the time one tries to add it. > It may thus be required to extend keeping the lock until after > idletimer_tg_create, in other words, lookup and create must be atomic > to the rest of the world. Ahh, sure! I missed the actual point of Patrick's question. I had the idletimer_tg_create() inside the lock, but when I added the sysfs_create_file() there (which can sleep), I screwed up with the locking. I'll move the sysfs file creation to outside that function so I can keep the lock until after the timer is added to the list. Thanks for clarifying!
On Wed, 2010-06-09 at 19:48 +0200, Coelho Luciano (Nokia-D/Helsinki) wrote: > On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: > > >> > + timer = __idletimer_tg_find_by_label(info->label); > > >> > + if (!timer) { > > >> > + spin_unlock(&list_lock); > > >> > + timer = idletimer_tg_create(info); > > >> > > > >> > > >> How does this prevent creating the same timer twice? > > > > > >The timer will only be created if __idletimer_tg_find_by_label() returns > > >NULL, which means that no timer with that label has been found. "info" > > >won't be the same if info->label is different, right? Or can it change > > >on the fly? > > > > One thing to be generally aware about is that things could potentially > > be instantiated by another entity between the time a label was looked up > > with negative result and the time one tries to add it. > > It may thus be required to extend keeping the lock until after > > idletimer_tg_create, in other words, lookup and create must be atomic > > to the rest of the world. > > Ahh, sure! I missed the actual point of Patrick's question. I had the > idletimer_tg_create() inside the lock, but when I added the > sysfs_create_file() there (which can sleep), I screwed up with the > locking. > > I'll move the sysfs file creation to outside that function so I can keep > the lock until after the timer is added to the list. Thanks for > clarifying! Hmmm... after struggling with this for a while, I think it's not really possible to simply create the sysfs file outside of the lock, because if the sysfs creation fails, we will again risk a race condition. I think the only way is to delay the sysfs file creation and do it in a workqueue.
On Wed, 2010-06-09 at 20:42 +0200, Coelho Luciano (Nokia-D/Helsinki) wrote: > On Wed, 2010-06-09 at 19:48 +0200, Coelho Luciano (Nokia-D/Helsinki) > wrote: > > On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: > > > >> > + timer = __idletimer_tg_find_by_label(info->label); > > > >> > + if (!timer) { > > > >> > + spin_unlock(&list_lock); > > > >> > + timer = idletimer_tg_create(info); > > > >> > > > > >> > > > >> How does this prevent creating the same timer twice? > > > > > > > >The timer will only be created if __idletimer_tg_find_by_label() returns > > > >NULL, which means that no timer with that label has been found. "info" > > > >won't be the same if info->label is different, right? Or can it change > > > >on the fly? > > > > > > One thing to be generally aware about is that things could potentially > > > be instantiated by another entity between the time a label was looked up > > > with negative result and the time one tries to add it. > > > It may thus be required to extend keeping the lock until after > > > idletimer_tg_create, in other words, lookup and create must be atomic > > > to the rest of the world. > > > > Ahh, sure! I missed the actual point of Patrick's question. I had the > > idletimer_tg_create() inside the lock, but when I added the > > sysfs_create_file() there (which can sleep), I screwed up with the > > locking. > > > > I'll move the sysfs file creation to outside that function so I can keep > > the lock until after the timer is added to the list. Thanks for > > clarifying! > > Hmmm... after struggling with this for a while, I think it's not really > possible to simply create the sysfs file outside of the lock, because if > the sysfs creation fails, we will again risk a race condition. > > I think the only way is to delay the sysfs file creation and do it in a > workqueue. Okay, I think I found a way to do it without using an extra workqueue. The patch is ready, but I still want to run some tests before sending it out.
On Wednesday 2010-06-09 20:42, Luciano Coelho wrote: > >> I'll move the sysfs file creation to outside that function so I can keep >> the lock until after the timer is added to the list. Thanks for >> clarifying! > >Hmmm... after struggling with this for a while, I think it's not really >possible to simply create the sysfs file outside of the lock, because if >the sysfs creation fails, we will again risk a race condition. Well if sysfs_add can return an error code when a file already exists (instead of adding it again), it's much easier. Try checking. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-06-09 at 23:05 +0200, ext Jan Engelhardt wrote: > On Wednesday 2010-06-09 20:42, Luciano Coelho wrote: > > > >> I'll move the sysfs file creation to outside that function so I can keep > >> the lock until after the timer is added to the list. Thanks for > >> clarifying! > > > >Hmmm... after struggling with this for a while, I think it's not really > >possible to simply create the sysfs file outside of the lock, because if > >the sysfs creation fails, we will again risk a race condition. > > Well if sysfs_add can return an error code when a file already > exists (instead of adding it again), it's much easier. Try checking. Unfortunately sysfs_add and sysfs_create will throw a kernel warning if we try to create a file that already exists. But this was not really the problem. Now I just throw an WARN_ON if the sysfs file creation fails, the other sysfs functions I'm calling can handle this situation. This shouldn't happen in normal cases and, if it does, the timer will run normally, but there won't be a sysfs file associated with it (and there won't be any notification). As I just wrote in another email, I thought I had figured out a way to do it without a workqueue. But now that I looked into the code again, I think there might still be some race conditions... For example: If someone deletes the timer immediately after I released the lock and before I created the sysfs entry... The deletion won't cause problems if the file is not there, it will just nop. But the creation of the file after the timer has been deleted will cause the file to be dangling in sysfs without any timer associated with it... :(
Luciano Coelho wrote: > On Wed, 2010-06-09 at 19:48 +0200, Coelho Luciano (Nokia-D/Helsinki) > wrote: > >> On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: >> >>>>>> + timer = __idletimer_tg_find_by_label(info->label); >>>>>> + if (!timer) { >>>>>> + spin_unlock(&list_lock); >>>>>> + timer = idletimer_tg_create(info); >>>>>> >>>>>> >>>>> How does this prevent creating the same timer twice? >>>>> >>>> The timer will only be created if __idletimer_tg_find_by_label() returns >>>> NULL, which means that no timer with that label has been found. "info" >>>> won't be the same if info->label is different, right? Or can it change >>>> on the fly? >>>> >>> One thing to be generally aware about is that things could potentially >>> be instantiated by another entity between the time a label was looked up >>> with negative result and the time one tries to add it. >>> It may thus be required to extend keeping the lock until after >>> idletimer_tg_create, in other words, lookup and create must be atomic >>> to the rest of the world. >>> >> Ahh, sure! I missed the actual point of Patrick's question. I had the >> idletimer_tg_create() inside the lock, but when I added the >> sysfs_create_file() there (which can sleep), I screwed up with the >> locking. >> >> I'll move the sysfs file creation to outside that function so I can keep >> the lock until after the timer is added to the list. Thanks for >> clarifying! >> > > Hmmm... after struggling with this for a while, I think it's not really > possible to simply create the sysfs file outside of the lock, because if > the sysfs creation fails, we will again risk a race condition. > > I think the only way is to delay the sysfs file creation and do it in a > workqueue. > Why don't you simply use a mutex instead of the spinlock? It would be better to only do the lookup once and store the timer pointer in the target structure anyways. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-06-10 at 12:07 +0200, ext Patrick McHardy wrote: > Luciano Coelho wrote: > > On Wed, 2010-06-09 at 19:48 +0200, Coelho Luciano (Nokia-D/Helsinki) > > wrote: > > > >> On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: > >> > >>>>>> + timer = __idletimer_tg_find_by_label(info->label); > >>>>>> + if (!timer) { > >>>>>> + spin_unlock(&list_lock); > >>>>>> + timer = idletimer_tg_create(info); > >>>>>> > >>>>>> > >>>>> How does this prevent creating the same timer twice? > >>>>> > >>>> The timer will only be created if __idletimer_tg_find_by_label() returns > >>>> NULL, which means that no timer with that label has been found. "info" > >>>> won't be the same if info->label is different, right? Or can it change > >>>> on the fly? > >>>> > >>> One thing to be generally aware about is that things could potentially > >>> be instantiated by another entity between the time a label was looked up > >>> with negative result and the time one tries to add it. > >>> It may thus be required to extend keeping the lock until after > >>> idletimer_tg_create, in other words, lookup and create must be atomic > >>> to the rest of the world. > >>> > >> Ahh, sure! I missed the actual point of Patrick's question. I had the > >> idletimer_tg_create() inside the lock, but when I added the > >> sysfs_create_file() there (which can sleep), I screwed up with the > >> locking. > >> > >> I'll move the sysfs file creation to outside that function so I can keep > >> the lock until after the timer is added to the list. Thanks for > >> clarifying! > >> > > > > Hmmm... after struggling with this for a while, I think it's not really > > possible to simply create the sysfs file outside of the lock, because if > > the sysfs creation fails, we will again risk a race condition. > > > > I think the only way is to delay the sysfs file creation and do it in a > > workqueue. > > > > Why don't you simply use a mutex instead of the spinlock? It would be better > to only do the lookup once and store the timer pointer in the target > structure > anyways. Wow! Again I have been totally blind and focusing only in a solution for the spinlock problem, while using a mutex would ease things up quite a lot! Thanks for the suggestion, I'll re-spin my patch (pun intended?) with a mutex. I also agree that it makes more sense to lookup and store the timer in the targe.
On Thu, 2010-06-10 at 15:42 +0300, Luciano Coelho wrote: > On Thu, 2010-06-10 at 12:07 +0200, ext Patrick McHardy wrote: > > Luciano Coelho wrote: > > > On Wed, 2010-06-09 at 19:48 +0200, Coelho Luciano (Nokia-D/Helsinki) > > > wrote: > > > > > >> On Wed, 2010-06-09 at 17:18 +0200, ext Jan Engelhardt wrote: > > >> > > >>>>>> + timer = __idletimer_tg_find_by_label(info->label); > > >>>>>> + if (!timer) { > > >>>>>> + spin_unlock(&list_lock); > > >>>>>> + timer = idletimer_tg_create(info); > > >>>>>> > > >>>>>> > > >>>>> How does this prevent creating the same timer twice? > > >>>>> > > >>>> The timer will only be created if __idletimer_tg_find_by_label() returns > > >>>> NULL, which means that no timer with that label has been found. "info" > > >>>> won't be the same if info->label is different, right? Or can it change > > >>>> on the fly? > > >>>> > > >>> One thing to be generally aware about is that things could potentially > > >>> be instantiated by another entity between the time a label was looked up > > >>> with negative result and the time one tries to add it. > > >>> It may thus be required to extend keeping the lock until after > > >>> idletimer_tg_create, in other words, lookup and create must be atomic > > >>> to the rest of the world. > > >>> > > >> Ahh, sure! I missed the actual point of Patrick's question. I had the > > >> idletimer_tg_create() inside the lock, but when I added the > > >> sysfs_create_file() there (which can sleep), I screwed up with the > > >> locking. > > >> > > >> I'll move the sysfs file creation to outside that function so I can keep > > >> the lock until after the timer is added to the list. Thanks for > > >> clarifying! > > >> > > > > > > Hmmm... after struggling with this for a while, I think it's not really > > > possible to simply create the sysfs file outside of the lock, because if > > > the sysfs creation fails, we will again risk a race condition. > > > > > > I think the only way is to delay the sysfs file creation and do it in a > > > workqueue. > > > > > > > Why don't you simply use a mutex instead of the spinlock? It would be better > > to only do the lookup once and store the timer pointer in the target > > structure > > anyways. > > Wow! Again I have been totally blind and focusing only in a solution for > the spinlock problem, while using a mutex would ease things up quite a > lot! Thanks for the suggestion, I'll re-spin my patch (pun intended?) > with a mutex. Yep, now I think I (finally) got it right :) > I also agree that it makes more sense to lookup and store the timer in > the targe. One quick question about this: did you mean to put it in the target info structure, in this case struct idletimer_tg_info? So is it okay to have internal data in this structure even though it is mostly for passing data from the userspace to the kernel?
On Thursday 2010-06-10 15:32, Luciano Coelho wrote: > >> I also agree that it makes more sense to lookup and store the timer in >> the targe. > >One quick question about this: did you mean to put it in the target info >structure, in this case struct idletimer_tg_info? So is it okay to have >internal data in this structure even though it is mostly for passing >data from the userspace to the kernel? You put a kernel-private pointer inside idletimer_tginfo, similar to what other modules do. That would also allow you to get a reference to the timer so that you do not have to call find_by_label on every idletimer_target invocation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter/xt_IDLETIMER.h b/include/linux/netfilter/xt_IDLETIMER.h new file mode 100644 index 0000000..6e62224 --- /dev/null +++ b/include/linux/netfilter/xt_IDLETIMER.h @@ -0,0 +1,40 @@ +/* + * 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_IDLETIMER_H +#define _XT_IDLETIMER_H + +#define MAX_LABEL_SIZE 32 + +struct idletimer_tg_info { + __u32 timeout; + + char label[MAX_LABEL_SIZE]; +}; + +#endif diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig index 8593a77..413ed24 100644 --- a/net/netfilter/Kconfig +++ b/net/netfilter/Kconfig @@ -424,6 +424,18 @@ config NETFILTER_XT_TARGET_HL since you can easily create immortal packets that loop forever on the network. +config NETFILTER_XT_TARGET_IDLETIMER + tristate "IDLETIMER target support" + depends on NETFILTER_ADVANCED + help + + This option adds the `IDLETIMER' target. Each matching packet + resets the timer associated with label specified when the rule is + added. When the timer expires, it triggers a sysfs notification. + The remaining time for expiration can be read via sysfs. + + To compile it as a module, choose M here. If unsure, say N. + config NETFILTER_XT_TARGET_LED tristate '"LED" target support' depends on LEDS_CLASS && LEDS_TRIGGERS 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..65c195e --- /dev/null +++ b/net/netfilter/xt_IDLETIMER.c @@ -0,0 +1,356 @@ +/* + * 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 + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/module.h> +#include <linux/timer.h> +#include <linux/list.h> +#include <linux/spinlock.h> +#include <linux/netfilter.h> +#include <linux/netfilter/x_tables.h> +#include <linux/netfilter/xt_IDLETIMER.h> +#include <linux/kobject.h> +#include <linux/workqueue.h> +#include <linux/sysfs.h> + +struct idletimer_tg { + struct list_head entry; + struct timer_list timer; + struct work_struct work; + + struct kobject *kobj; + struct idletimer_tg_attr *attr; + + unsigned int refcnt; +}; + +struct idletimer_tg_attr { + struct attribute attr; + ssize_t (*show)(struct kobject *kobj, + struct attribute *attr, char *buf); +}; + +static LIST_HEAD(idletimer_tg_list); +static DEFINE_SPINLOCK(list_lock); + +static struct kobject *idletimer_tg_kobj; + +static +struct idletimer_tg *__idletimer_tg_find_by_label(const char *label) +{ + struct idletimer_tg *entry; + + BUG_ON(!label); + + list_for_each_entry(entry, &idletimer_tg_list, entry) { + if (!strcmp(label, entry->attr->attr.name)) + return entry; + } + + return NULL; +} + +static ssize_t idletimer_tg_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + struct idletimer_tg *timer; + unsigned long expires = 0; + + spin_lock_bh(&list_lock); + timer = __idletimer_tg_find_by_label(attr->name); + 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 void idletimer_tg_delete(const struct idletimer_tg_info *info) +{ + struct idletimer_tg *timer; + + spin_lock_bh(&list_lock); + timer = __idletimer_tg_find_by_label(info->label); + if (!timer) { + spin_unlock_bh(&list_lock); + return; + } + + if (--timer->refcnt == 0) { + pr_debug("deleting timer %s\n", info->label); + + list_del(&timer->entry); + del_timer_sync(&timer->timer); + spin_unlock_bh(&list_lock); + + sysfs_remove_file(idletimer_tg_kobj, &timer->attr->attr); + kfree(timer->attr->attr.name); + kfree(timer->attr); + kfree(timer); + } else { + spin_unlock_bh(&list_lock); + pr_debug("decreased refcnt of timer %s to %u\n", + info->label, timer->refcnt); + } +} + +static void idletimer_tg_work(struct work_struct *work) +{ + struct idletimer_tg *timer = container_of(work, struct idletimer_tg, + work); + + sysfs_notify(idletimer_tg_kobj, NULL, + timer->attr->attr.name); +} + +static void idletimer_tg_expired(unsigned long data) +{ + struct idletimer_tg *timer = (struct idletimer_tg *) data; + + pr_debug("timer %s expired\n", + timer->attr->attr.name); + + schedule_work(&timer->work); +} + +static +struct idletimer_tg *idletimer_tg_create(const struct idletimer_tg_info *info) +{ + struct idletimer_tg *timer; + struct idletimer_tg_attr *attr; + + attr = kzalloc(sizeof(attr), GFP_KERNEL); + if (!attr) { + pr_debug("couldn't alloc attribute\n"); + return NULL; + } + + attr->attr.name = kstrdup(info->label, GFP_KERNEL); + if (!attr->attr.name) { + pr_debug("couldn't alloc attribute name\n"); + goto out_free_attr; + } + attr->attr.mode = S_IRUGO; + attr->show = idletimer_tg_show; + + if (sysfs_create_file(idletimer_tg_kobj, &attr->attr)) { + pr_debug("couldn't add attr to sysfs\n"); + goto out_free_name; + } + + timer = kmalloc(sizeof(struct idletimer_tg), GFP_KERNEL); + if (!timer) { + pr_debug("couldn't alloc timer\n"); + goto out_free_file; + } + + spin_lock_bh(&list_lock); + list_add(&timer->entry, &idletimer_tg_list); + + init_timer(&timer->timer); + setup_timer(&timer->timer, idletimer_tg_expired, (unsigned long) timer); + mod_timer(&timer->timer, + msecs_to_jiffies(info->timeout * 1000) + jiffies); + + timer->attr = attr; + timer->refcnt = 0; + + INIT_WORK(&timer->work, idletimer_tg_work); + spin_unlock_bh(&list_lock); + + return timer; + +out_free_file: + sysfs_remove_file(idletimer_tg_kobj, &attr->attr); +out_free_name: + kfree(attr->attr.name); +out_free_attr: + kfree(attr); + return NULL; +} + +static void idletimer_tg_cleanup(void) +{ + struct idletimer_tg *timer; + + spin_lock(&list_lock); + list_for_each_entry(timer, &idletimer_tg_list, entry) { + pr_debug("deleting timer %s\n", timer->attr->attr.name); + + list_del(&timer->entry); + del_timer_sync(&timer->timer); + kfree(timer->attr->attr.name); + kfree(timer->attr); + kfree(timer); + } + spin_unlock(&list_lock); +} + +/* + * The actual xt_tables plugin. + */ +static unsigned int idletimer_tg_target(struct sk_buff *skb, + const struct xt_action_param *par) +{ + const struct idletimer_tg_info *info = par->targinfo; + struct idletimer_tg *timer; + + pr_debug("resetting timer %s, timeout period %u\n", + info->label, info->timeout); + + spin_lock(&list_lock); + timer = __idletimer_tg_find_by_label(info->label); + + BUG_ON(!timer); + + mod_timer(&timer->timer, + msecs_to_jiffies(info->timeout * 1000) + jiffies); + spin_unlock(&list_lock); + + return XT_CONTINUE; +} + +static int idletimer_tg_checkentry(const struct xt_tgchk_param *par) +{ + const struct idletimer_tg_info *info = par->targinfo; + struct idletimer_tg *timer; + + pr_debug("checkentry targinfo %s\n", info->label); + + if (info->timeout == 0) { + pr_debug("timeout value is zero\n"); + return -EINVAL; + } + + if (!info->label || strlen(info->label) == 0) { + pr_debug("label is missing\n"); + return -EINVAL; + } + + spin_lock(&list_lock); + timer = __idletimer_tg_find_by_label(info->label); + if (!timer) { + spin_unlock(&list_lock); + timer = idletimer_tg_create(info); + if (!timer) { + pr_debug("failed to create timer\n"); + return -ENOMEM; + } + spin_lock(&list_lock); + } + + timer->refcnt++; + mod_timer(&timer->timer, + msecs_to_jiffies(info->timeout * 1000) + jiffies); + spin_unlock(&list_lock); + + return 0; +} + +static void idletimer_tg_destroy(const struct xt_tgdtor_param *par) +{ + const struct idletimer_tg_info *info = par->targinfo; + + pr_debug("destroy targinfo %s\n", info->label); + + idletimer_tg_delete(info); +} + +static struct xt_target idletimer_tg __read_mostly = { + .name = "IDLETIMER", + .family = NFPROTO_UNSPEC, + .target = idletimer_tg_target, + .targetsize = sizeof(struct idletimer_tg_info), + .checkentry = idletimer_tg_checkentry, + .destroy = idletimer_tg_destroy, + .me = THIS_MODULE, +}; + +static struct class *idletimer_tg_class; + +static struct device *idletimer_tg_device; + +static int __init idletimer_tg_init(void) +{ + int err; + + idletimer_tg_class = class_create(THIS_MODULE, "xt_idletimer"); + err = PTR_ERR(idletimer_tg_class); + if (IS_ERR(idletimer_tg_class)) { + pr_debug("couldn't register device class\n"); + goto out; + } + + idletimer_tg_device = device_create(idletimer_tg_class, NULL, + MKDEV(0, 0), NULL, "timers"); + err = PTR_ERR(idletimer_tg_device); + if (IS_ERR(idletimer_tg_device)) { + pr_debug("couldn't register system device\n"); + goto out_class; + } + + idletimer_tg_kobj = &idletimer_tg_device->kobj; + + err = xt_register_target(&idletimer_tg); + if (err < 0) { + pr_debug("couldn't register xt target\n"); + goto out_dev; + } + + return 0; +out_dev: + device_destroy(idletimer_tg_class, MKDEV(0, 0)); +out_class: + class_destroy(idletimer_tg_class); +out: + return err; +} + +static void __exit idletimer_tg_exit(void) +{ + xt_unregister_target(&idletimer_tg); + + device_destroy(idletimer_tg_class, MKDEV(0, 0)); + class_destroy(idletimer_tg_class); + + idletimer_tg_cleanup(); +} + +module_init(idletimer_tg_init); +module_exit(idletimer_tg_exit); + +MODULE_AUTHOR("Timo Teras <ext-timo.teras@nokia.com>"); +MODULE_AUTHOR("Luciano Coelho <luciano.coelho@nokia.com>"); +MODULE_DESCRIPTION("Xtables: idle time monitor"); +MODULE_LICENSE("GPL v2");