Re: [PATCH v2] netfilter: Xtables: idletimer target implementation

Previous thread: sysfs class/net/ problem by Johannes Berg on Wednesday, June 2, 2010 - 6:16 am. (58 messages)

Next thread: [PATCH v2] cls_u32: use skb_header_pointer() to dereference data safely by Changli Gao on Wednesday, June 2, 2010 - 7:00 am. (2 messages)
From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 6:41 am

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 module's object:

/sys/module/xt_IDLETIMER/idletimer/<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

 include/linux/netfilter/xt_IDLETIMER.h |   40 ++++
 net/netfilter/Kconfig                  |   12 +
 net/netfilter/Makefile                 |    1 +
 net/netfilter/xt_IDLETIMER.c           |  360 ++++++++++++++++++++++++++++++++
 4 files changed, 413 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..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 ...
From: Jan Engelhardt
Date: Wednesday, June 2, 2010 - 8:16 am

Isn't this going to oops when you compile this module as =y?

--

From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 11:37 am

Damn, that's true. :(

I'll investigate how to fix this.

-- 
Cheers,
Luca.

--

From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 12:05 pm

On Wed, 2010-06-02 at 20:37 +0200, Coelho Luciano (Nokia-D/Helsinki)

Would it be too hacky to force it to be a module (ie. add "depends on m"
in Kconfig)?

Besides /sys/module/xt_IDLETIMER and /sys/class/net, which we have
already discarded, I can't find any other place that would make sense to
add the idletimer in the kernel object hierarchy...


-- 
Cheers,
Luca.

--

From: Jan Engelhardt
Date: Wednesday, June 2, 2010 - 12:29 pm

While THIS_MODULE is NULL in =y mode, /sys/module/<xyz> can still exist 
(cf. /sys/module/printk). I just don't know how to get at the kobj for 
it, but the existence of it must mean it's there somewhere. Might ask 
sysfs authors.
--

From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 12:52 pm

Okay, good to know.  My initial theory was that /sys/module/xt_IDLETIMER
(pointed to by THIS_MODULE) would exist even if it was linked into the
kernel itself, but now it's obvious that it doesn't.

I'll investigate how printk does that.

-- 
Cheers,
Luca.

--

From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 1:04 pm

On Wed, 2010-06-02 at 21:52 +0200, Coelho Luciano (Nokia-D/Helsinki)

What causes printk to appear under /sys/module even when compiled in, is
that it uses a module param.  This line:

module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);

...is what triggers the printk directory to be created in sysfs.  If I
add a similar line in my module, it shows up there too.

I still don't know if there is an actual kobject associated with it,
I'll check that next.


-- 
Cheers,
Luca.

--

From: Luciano Coelho
Date: Wednesday, June 2, 2010 - 2:01 pm

On Wed, 2010-06-02 at 22:04 +0200, Coelho Luciano (Nokia-D/Helsinki)

Okay, so here is how it goes: if the module is linked into the kernel
and it has module parameters, the kernel creates a kobj for it as a
module_ktype without parent, which will cause it to show up
in /sys/modules.

I could do the same in the module initialization when THIS_MODULE ==
NULL, but I don't see any other module doing this.  In fact, I only see
the kernel itself creating kobjects of module_ktype (in load_module()
and in the case I just described).  Smells like a terrible hack to do
that in the module itself... :(

Adding bogus parameters to the module just to trig the kernel to create
the kobject also seems to be too hacky...

-- 
Cheers,
Luca.

--

From: Luciano Coelho
Date: Thursday, June 3, 2010 - 12:04 am

On Wed, 2010-06-02 at 23:01 +0200, Coelho Luciano (Nokia-D/Helsinki)

Looking closer, it seems that it makes a bit of sense to add a kernel
module to /sys/device/system.  I think it makes more sense than adding
to the module class or to the net class, actually.  The idletimer is not
a net device (so it doesn't fit in /sys/class/net) and it is not a
module, even though it may be handled by the xt_IDLETIMER module.

So we can look at the xt_idletimer as a system device, which is not a
peripheral device in itself, but a software timer device (there are
already similar components).

I'll add the kernel object we need as a system class device, so it will
go under /sys/devices/system/xt_idletimer.  Does that make sense to you?


-- 
Cheers,
Luca.

--

From: Jan Engelhardt
Date: Thursday, June 3, 2010 - 12:58 am

Mh.. somehow I'd pick /sys/devices/virtual/xt_idletimer.
Or even create a /sys/net/xt_idletimer. (/sys has conceptual
subsystems directly beneath it: devices, fs, kernel, ...)
--

From: Luciano Coelho
Date: Thursday, June 3, 2010 - 3:13 am

Yes, I think I'll use the /sys/device/virtual/misc class.  That seems to
be the place where, well, miscellaneous devices go. :) I think it fits
pretty nicely in that concept.

We could also have a /sys/net subsystem, but that's very high in the
sysfs hierarchy and adding it in the xt_IDLETIMER module wouldn't make
any sense.  This is something that should be added (if really needed) in
the net core subsystem, I guess.

I'll use the first option and resubmit the patch as v3.


-- 
Cheers,
Luca.

--

From: Luciano Coelho
Date: Thursday, June 3, 2010 - 6:17 am

On Thu, 2010-06-03 at 12:13 +0200, Coelho Luciano (Nokia-D/Helsinki)

Argh, it seems that I'll never end this.  Pros and cons of a few
different solutions:

1) /sys/devices/virtual/misc/xt_idletimer/<user_defined_label>

The misc class is a char device, so if I add the xt_idletimer there,
I'll get lots of useless things, like file operation functions etc.  And
a few attributes that make sense to char devices are also added
automatically, but will never be used with the xt_idletimer.

2) /sys/devices/virtual/xt_idletimer/

This solution seems to be okay, basically I create a new virtual class
called xt_idletimer.  This will automatically create a link
to /sys/class/xt_idletimer, so the user doesn't need to know that this
is a virtual device at all.  One problem is that we'll have a few more
device attributes than we need such as ./power/wakeup and ./uevent,
which we won't use.

    2a) /sys/devices/virtual/xt_idletimer/<user_defined_label>/timer

Each timer set by the user will be added as a new device named
<user_defined_label> under the xt_idletimer class.  Each of these
devices will have one more attribute called timer (plus the
autogenerated wakeup and uevent attributes).  The drawback is that we
use a more resources than we need, since we have more kobjects and more
attributes than we need.

    2b) /sys/devices/virtual/xt_idletimer/timers/<user_defined_label>

This solution is similar to 2a, but uses less resources, since we have
only one kobject with several attributes (one for each user defined
label).  We still have the extra attributes, though.

3) /sys/devices/system/xt_idletimer/<user_defined_label>/timer

This solution uses less resources, because the system device class
doesn't contain any implicit attributes.

4) /sys/devices/{system,virtual}/xt_idletimer/<user_defined_label>

I tried this one, by creating the xt_idletimer class and adding
attributes directly to it.  But due to restrictions in sysfs, I didn't
figure out a way to dynamically add ...
Previous thread: sysfs class/net/ problem by Johannes Berg on Wednesday, June 2, 2010 - 6:16 am. (58 messages)

Next thread: [PATCH v2] cls_u32: use skb_header_pointer() to dereference data safely by Changli Gao on Wednesday, June 2, 2010 - 7:00 am. (2 messages)