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

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.