Re: [PATCH] Add refcounts to LED target

[Patrick McHardy <kaber@trash.net>]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

Jan Engelhardt wrote:
> On Sunday 2009-11-29 12:33, Adam Nielsen wrote:
> 
>>>> static bool led_tg_check(const struct xt_tgchk_param *par)
>>>> {
>>>> 	struct xt_led_info *ledinfo = par->targinfo;
>>>> -	struct xt_led_info_internal *ledinternal;
>>>> +	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
>>>> 	int err;
>>> You cannot rely on ledinfo->internal_data having any meaningful
>>> value when iptables prepares the rule.
>> Hmm ok, so in led_tg_check (the .checkentry function) how do you tell whether
>> the xt_tgchk_param is pointing to an existing ruleset or not?  Or is it always
>> referring to a new ruleset and you have to handle it yourself?
> 
> You always have to do a lookup on some structure that has xt_LED.ko 
> lifetime (similar to what xt_recent/xt_rateest does).
> 
>> I guess my question comes from this point of view:
>>
>>  $ iptables -A scroll_lock -j LED --led-trigger-id http
>>
>> This calls led_tg_check() with a new xt_tgchk_param structure.
>>
>>  $ iptables -I INPUT 1 -p tcp --sport 80 -j scroll_lock
>>
>> Now led_tg_check() gets called again with an xt_tgchk_param structure
>> containing the trigger name etc. even though this was not specified on the
>> command line.  Where does that second xt_tgchk_param come from if it's not a
>> pointer to the first one?
> 
> Running two iptables instances concurrently. Even without races like 
> these, it would be a security violation to accept unknown pointers from 
> userspace.

Since this has already taken ages, I took the liberty of preparing
an example fix. Adam, please have a look at this and give it some
testing.

As usual when sharing state but not parameters between target
instances, there's a problem of potentially inconsistent parameters.
You can now create two rules refering to the same trigger, but
using different always_blink and delay parameters. You could
either catch this by storing a copy of the parameters in the
xt_led_info_internal struct and verifying their consistency, or ignore
it and have each rule modify the LED state using its own parameters.
Not sure which one makes more sense here.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2414 bytes --]

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index 8ff7843..1a6693f 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -31,12 +31,16 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Adam Nielsen <a.nielsen@shikadi.net>");
 MODULE_DESCRIPTION("Xtables: trigger LED devices on packet match");
 
+static LIST_HEAD(xt_led_triggers);
+
 /*
  * This is declared in here (the kernel module) only, to avoid having these
  * dependencies in userspace code.  This is what xt_led_info.internal_data
  * points to.
  */
 struct xt_led_info_internal {
+	struct list_head list;
+	int refcnt;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -80,6 +84,17 @@ static void led_timeout_callback(unsigned long data)
 	led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
 }
 
+static struct xt_led_info_internal *led_trigger_lookup(const char *name)
+{
+	struct xt_led_info_internal *ledinternal;
+
+	list_for_each_entry(ledinternal, &xt_led_triggers, list) {
+		if (!strcmp(name, ledinternal->netfilter_led_trigger.name))
+			return ledinternal;
+	}
+	return NULL;
+}
+
 static bool led_tg_check(const struct xt_tgchk_param *par)
 {
 	struct xt_led_info *ledinfo = par->targinfo;
@@ -91,12 +106,19 @@ static bool led_tg_check(const struct xt_tgchk_param *par)
 		return false;
 	}
 
+	ledinternal = led_trigger_lookup(ledinfo->id);
+	if (ledinternal) {
+		ledinternal->refcnt++;
+		goto out;
+	}
+
 	ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
 	if (!ledinternal) {
 		printk(KERN_CRIT KBUILD_MODNAME ": out of memory\n");
 		return false;
 	}
 
+	ledinternal->refcnt = 1;
 	ledinternal->netfilter_led_trigger.name = ledinfo->id;
 
 	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
@@ -114,6 +136,8 @@ static bool led_tg_check(const struct xt_tgchk_param *par)
 		setup_timer(&ledinternal->timer, led_timeout_callback,
 			    (unsigned long)ledinfo);
 
+	list_add_tail(&ledinternal->list, &xt_led_triggers);
+out:
 	ledinfo->internal_data = ledinternal;
 
 	return true;
@@ -129,6 +153,10 @@ static void led_tg_destroy(const struct xt_tgdtor_param *par)
 	const struct xt_led_info *ledinfo = par->targinfo;
 	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
 
+	if (--ledinternal->refcnt)
+		return;
+
+	list_del(&ledinternal->list);
 	if (ledinfo->delay > 0)
 		del_timer_sync(&ledinternal->timer);