Avoiding multiple calls to xt_target.checkentry

Avoiding multiple calls to xt_target.checkentry

[Adam Nielsen]

Hi all,

I've just discovered a small bug in the xt_LED target I submitted a couple of
months back, but I'm not sure of the best way of fixing it.

When iptables adds a rule with the LED target, it calls the "checkentry"
function (led_tg_check) to confirm that the rule conditions are valid.  This
target doesn't care what the rule is, so the code returns success after
creating a new LED trigger elsewhere in the kernel.

The problem is that if you create a new chain, add a rule to it with the LED
target, then add *other* rules that point to the new chain, the "checkentry"
function gets called multiple times (to make sure the new rules are valid)
which means the led_tg_check function tries to create the same trigger
multiple times (which fails.)

For example:

$ iptables -N scroll_lock

$ iptables -A scroll_lock -j LED --led-trigger-id http
// led_tg_check() called and registers the "netfilter-http" LED trigger

$ iptables -I INPUT 1 -p tcp --sport 80 -j scroll_lock
iptables: Invalid argument. Run `dmesg' for more information.
// led_tg_check() was called again and failed

$ dmesg
xt_LED: led_trigger_register() failed
xt_LED: Trigger name is already in use.

In other words, is there a function only called the first time the rule is
added by iptables?  Or should I be keeping track of that myself?

Thanks,
Adam.

Re: Avoiding multiple calls to xt_target.checkentry

[Jan Engelhardt]

On Sunday 2009-05-24 03:46, Adam Nielsen wrote:
>
>In other words, is there a function only called the first time the rule is
>added by iptables?  Or should I be keeping track of that myself?

You forget that iptables does not add rules. It replaces entire tables,
and to make that atomic, the new table is checked before the old one
is released. And yes, you usually try to lookup a led trigger first
before creating one, because there can be a table that calls -j LED
twice.

Re: Avoiding multiple calls to xt_target.checkentry

[Adam Nielsen]

>> In other words, is there a function only called the first time the rule is
>> added by iptables?  Or should I be keeping track of that myself?
> 
> You forget that iptables does not add rules. It replaces entire tables,
> and to make that atomic, the new table is checked before the old one
> is released. And yes, you usually try to lookup a led trigger first
> before creating one, because there can be a table that calls -j LED
> twice.

Thanks for the explanation!  So - to get it straight in my mind - the
checkentry function will be called multiple times while the trigger exists,
but is the destroy function also called multiple times?  Or is checkentry
called whenever tables are created, but destroy only ever called once when the
table is removed for the last time?

Just trying to work out whether I need to avoid removing the LED trigger in
the destroy function as well.

Thanks again,
Adam.

Re: Avoiding multiple calls to xt_target.checkentry

[Jan Engelhardt]

On Thursday 2009-05-28 01:07, Adam Nielsen wrote:

>>> In other words, is there a function only called the first time the rule is
>>> added by iptables?  Or should I be keeping track of that myself?
>> 
>> You forget that iptables does not add rules. It replaces entire tables,
>> and to make that atomic, the new table is checked before the old one
>> is released. And yes, you usually try to lookup a led trigger first
>> before creating one, because there can be a table that calls -j LED
>> twice.
>
>Thanks for the explanation!  So - to get it straight in my mind - the
>checkentry function will be called multiple times while the trigger exists,
>but is the destroy function also called multiple times?  Or is checkentry
>called whenever tables are created, but destroy only ever called once when the
>table is removed for the last time?

old table         new table           new table 2
==================================================
(is in place)
                  checkentry
                  (put into place)
destroy
                                      checkentry
                                      (put into place)
                  destroy

>
>Just trying to work out whether I need to avoid removing the LED trigger in
>the destroy function as well.

Yes, as destroy can - and usually is - called after a new table has
ran through checkentry. With the use of some sort of refcounting, it's
easy: checkentry upps it, and destroy downs it. If the refcount goes to zero,
remove the trigger.

Re: Avoiding multiple calls to xt_target.checkentry

[Patrick McHardy]

Adam Nielsen wrote:
>>> In other words, is there a function only called the first time the rule is
>>> added by iptables?  Or should I be keeping track of that myself?
>> You forget that iptables does not add rules. It replaces entire tables,
>> and to make that atomic, the new table is checked before the old one
>> is released. And yes, you usually try to lookup a led trigger first
>> before creating one, because there can be a table that calls -j LED
>> twice.
> 
> Thanks for the explanation!  So - to get it straight in my mind - the
> checkentry function will be called multiple times while the trigger exists,
> but is the destroy function also called multiple times?  Or is checkentry
> called whenever tables are created, but destroy only ever called once when the
> table is removed for the last time?

They will always be called an equal amount of times - each one
once per target instance.

Re: Avoiding multiple calls to xt_target.checkentry

[Adam Nielsen]

>> Thanks for the explanation!  So - to get it straight in my mind - the
>> checkentry function will be called multiple times while the trigger
>> exists,
>> but is the destroy function also called multiple times?  Or is checkentry
>> called whenever tables are created, but destroy only ever called once
>> when the
>> table is removed for the last time?
> 
> They will always be called an equal amount of times - each one
> once per target instance.

Great, thanks for the info (thanks Jan too!)  I'm working on a patch so I'll
post it when I think I've solved the issue correctly.

Cheers,
Adam.

Re: Avoiding multiple calls to xt_target.checkentry

[Patrick McHardy]

Adam Nielsen wrote:
>>> Thanks for the explanation!  So - to get it straight in my mind - the
>>> checkentry function will be called multiple times while the trigger
>>> exists,
>>> but is the destroy function also called multiple times?  Or is checkentry
>>> called whenever tables are created, but destroy only ever called once
>>> when the
>>> table is removed for the last time?
>> They will always be called an equal amount of times - each one
>> once per target instance.
> 
> Great, thanks for the info (thanks Jan too!)  I'm working on a patch so I'll
> post it when I think I've solved the issue correctly.

Just wondering whether there's still hope for this issue to get fixed.

I haven't added the LED extension to iptables yet, so if there's no
interest in fixing this, we can remove the LED target again.

Re: Avoiding multiple calls to xt_target.checkentry

[Jan Engelhardt]

On Thursday 2009-11-05 16:00, Patrick McHardy wrote:
>Adam Nielsen wrote in summer 2009:
>>>> Thanks for the explanation!  So - to get it straight in my mind - the
>>>> checkentry function will be called multiple times while the trigger
>>>> exists,
>>>> but is the destroy function also called multiple times?  Or is checkentry
>>>> called whenever tables are created, but destroy only ever called once
>>>> when the
>>>> table is removed for the last time?
>>> They will always be called an equal amount of times - each one
>>> once per target instance.
>> 
>> Great, thanks for the info (thanks Jan too!)  I'm working on a patch so I'll
>> post it when I think I've solved the issue correctly.
>
>Just wondering whether there's still hope for this issue to get fixed.
>
>I haven't added the LED extension to iptables yet, so if there's no
>interest in fixing this, we can remove the LED target again.

An increasing number of modules want to keep a global state.
Currently, xt_recent and xt_RATEEST do that in the kernel; xt_quota3
(Xt-a) also does its own management — basically just a
name=>personal_struct mapping —, and xt_LED would now need such state
keeping too. It seems preferable to somehow join that together and
funnel that into xtables.c. What do you think?
Meanwhile, let me prep some commit.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Avoiding multiple calls to xt_target.checkentry

[Patrick McHardy]

Jan Engelhardt wrote:
> On Thursday 2009-11-05 16:00, Patrick McHardy wrote:
>> Adam Nielsen wrote in summer 2009:
>>>>> Thanks for the explanation!  So - to get it straight in my mind - the
>>>>> checkentry function will be called multiple times while the trigger
>>>>> exists,
>>>>> but is the destroy function also called multiple times?  Or is checkentry
>>>>> called whenever tables are created, but destroy only ever called once
>>>>> when the
>>>>> table is removed for the last time?
>>>> They will always be called an equal amount of times - each one
>>>> once per target instance.
>>> Great, thanks for the info (thanks Jan too!)  I'm working on a patch so I'll
>>> post it when I think I've solved the issue correctly.
>> Just wondering whether there's still hope for this issue to get fixed.
>>
>> I haven't added the LED extension to iptables yet, so if there's no
>> interest in fixing this, we can remove the LED target again.
> 
> An increasing number of modules want to keep a global state.
> Currently, xt_recent and xt_RATEEST do that in the kernel; xt_quota3
> (Xt-a) also does its own management — basically just a
> name=>personal_struct mapping —, and xt_LED would now need such state
> keeping too. It seems preferable to somehow join that together and
> funnel that into xtables.c. What do you think?
> Meanwhile, let me prep some commit.

If this can be done clean and simple, sure. I have my doubt though,
since it needs a mapping of the old rule to the new rule.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Avoiding multiple calls to xt_target.checkentry

[Adam Nielsen]

> Just wondering whether there's still hope for this issue to get fixed.
>
> I haven't added the LED extension to iptables yet, so if there's no
> interest in fixing this, we can remove the LED target again.

Sorry Patrick, it's still on my to-do list I just haven't had a chance to get 
back to it yet!  But yes I still fully intend to fix the issue.  I'll see if I 
can spend some more time on it this weekend.

Cheers,
Adam.

Re: Avoiding multiple calls to xt_target.checkentry

[Patrick McHardy]

Adam Nielsen wrote:
>> Just wondering whether there's still hope for this issue to get fixed.
>>
>> I haven't added the LED extension to iptables yet, so if there's no
>> interest in fixing this, we can remove the LED target again.
> 
> Sorry Patrick, it's still on my to-do list I just haven't had a chance
> to get back to it yet!  But yes I still fully intend to fix the issue. 
> I'll see if I can spend some more time on it this weekend.

OK thanks.

[PATCH] Add refcounts to LED target

[Adam Nielsen]

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

>>> Just wondering whether there's still hope for this issue to get fixed.
>>>
>>> I haven't added the LED extension to iptables yet, so if there's no
>>> interest in fixing this, we can remove the LED target again.

Sorry again it has taken me so long to get around to this, but hopefully the 
attached patch is acceptable.  It fixes the issue for me, however I would just 
like to confirm one thing will work as expected.

I am relying on the xt_tgchk_param passed to the .checkentry function being 
zero'd out the first time.  One of the members is a pointer which becomes 
valid when the LED trigger is created, and I'm assuming if that pointer is 
NULL then the LED trigger hasn't yet been created.  Providing that iptables 
sets this field to NULL before the first call, and leaves it unchanged when 
replacing rules/tables then it will work fine.

The patch is against the latest git version of nf-next-2.6.

Thanks,
Adam.

[-- Attachment #2: led_target_refcount.patch --]
[-- Type: text/plain, Size: 3585 bytes --]

Add reference counting to the netfilter LED target, to fix errors when
multiple rules point to the same target ("LED trigger already exists").

Signed-off-by: Adam Nielsen <a.nielsen@shikadi.net>

---

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index 8ff7843..1877d6b 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -37,6 +37,7 @@ MODULE_DESCRIPTION("Xtables: trigger LED devices on packet match");
  * points to.
  */
 struct xt_led_info_internal {
+	atomic_t refcount;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -83,43 +84,52 @@ static void led_timeout_callback(unsigned long data)
 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;
 
-	if (ledinfo->id[0] == '\0') {
-		printk(KERN_ERR KBUILD_MODNAME ": No 'id' parameter given.\n");
-		return false;
-	}
-
-	ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
-	if (!ledinternal) {
-		printk(KERN_CRIT KBUILD_MODNAME ": out of memory\n");
-		return false;
-	}
-
-	ledinternal->netfilter_led_trigger.name = ledinfo->id;
-
-	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
-	if (err) {
-		printk(KERN_CRIT KBUILD_MODNAME
-			": led_trigger_register() failed\n");
-		if (err == -EEXIST)
-			printk(KERN_ERR KBUILD_MODNAME
-				": Trigger name is already in use.\n");
-		goto exit_alloc;
+	if (ledinternal) {
+		/* Rule already exists */
+		atomic_inc(&ledinternal->refcount);
+	} else {
+		/* New rule, check everything is OK */
+		if (ledinfo->id[0] == '\0') {
+			printk(KERN_ERR KBUILD_MODNAME ": No 'id' parameter given.\n");
+			return false;
+		}
+
+		ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
+		if (!ledinternal) {
+			printk(KERN_CRIT KBUILD_MODNAME ": out of memory\n");
+			return false;
+		}
+
+		ledinternal->netfilter_led_trigger.name = ledinfo->id;
+
+		err = led_trigger_register(&ledinternal->netfilter_led_trigger);
+		if (err) {
+			printk(KERN_CRIT KBUILD_MODNAME
+				": led_trigger_register() failed\n");
+			if (err == -EEXIST)
+				printk(KERN_ERR KBUILD_MODNAME
+					": Trigger name is already in use.\n");
+			goto exit_alloc;
+		}
+
+		/* See if we need to set up a timer */
+		if (ledinfo->delay > 0)
+			setup_timer(&ledinternal->timer, led_timeout_callback,
+						(unsigned long)ledinfo);
+
+		atomic_set(&ledinternal->refcount, 1);
+
+		ledinfo->internal_data = ledinternal;
 	}
 
-	/* See if we need to set up a timer */
-	if (ledinfo->delay > 0)
-		setup_timer(&ledinternal->timer, led_timeout_callback,
-			    (unsigned long)ledinfo);
-
-	ledinfo->internal_data = ledinternal;
-
 	return true;
 
 exit_alloc:
 	kfree(ledinternal);
+	ledinfo->internal_data = NULL;
 
 	return false;
 }
@@ -129,11 +139,14 @@ 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 (ledinfo->delay > 0)
-		del_timer_sync(&ledinternal->timer);
+	if (atomic_dec_and_test(&ledinternal->refcount)) {
+		/* No more rules use this LED trigger, so remove it */
+		if (ledinfo->delay > 0)
+			del_timer_sync(&ledinternal->timer);
 
-	led_trigger_unregister(&ledinternal->netfilter_led_trigger);
-	kfree(ledinternal);
+		led_trigger_unregister(&ledinternal->netfilter_led_trigger);
+		kfree(ledinternal);
+	}
 }
 
 static struct xt_target led_tg_reg __read_mostly = {

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

On Sunday 2009-11-29 02:43, Adam Nielsen wrote:
>
>>>> Just wondering whether there's still hope for this issue to get fixed.
>>>>
>>>> I haven't added the LED extension to iptables yet, so if there's no
>>>> interest in fixing this, we can remove the LED target again.
>
> I am relying on the xt_tgchk_param passed to the .checkentry function being
> zero'd out the first time.  One of the members is a pointer which becomes valid
> when the LED trigger is created, and I'm assuming if that pointer is NULL then
> the LED trigger hasn't yet been created.  Providing that iptables sets this
> field to NULL before the first call, and leaves it unchanged when replacing
> rules/tables then it will work fine.
>
>@@ -83,43 +84,52 @@ static void led_timeout_callback(unsigned long data)
> 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.

1. iptables1 loads ruleset (-j LED exists somewhere)
2. someone does -A INPUT something
3. iptables1 blocks for some reason (say, DNS..)
		4. iptables2 loads ruleset (-j LED exists)
		5. someone does -D .. -j LED
		6. iptables2 submits ruleset
7. iptables1 becomes runnable again and submits ruleset

 -> ledinfo->internal_data now points to a freed area.

Re: [PATCH] Add refcounts to LED target

[Adam Nielsen]

>> 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?

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?

Thanks,
Adam.

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

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.

Re: [PATCH] Add refcounts to LED target

[Patrick McHardy]

[-- 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);
 

Re: [PATCH] Add refcounts to LED target

[Adam Nielsen]

> 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.

Well I'm afraid I can't see why, but for some reason once I add a second 
iptables rule with this target my kernel often locks up, hard (audio starts 
skipping, magic SysRq keys don't work) even if I don't have any actual LED 
devices on the system.  The first rule, even when attached to a LED device, 
seems to work fine.

I'll have to do some more debugging to see if I can figure out where it's 
locking up.

Cheers,
Adam.

Re: [PATCH] Add refcounts to LED target

[Patrick McHardy]

Adam Nielsen wrote:
>> 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.
> 
> Well I'm afraid I can't see why, but for some reason once I add a second
> iptables rule with this target my kernel often locks up, hard (audio
> starts skipping, magic SysRq keys don't work) even if I don't have any
> actual LED devices on the system.  The first rule, even when attached to
> a LED device, seems to work fine.
> 
> I'll have to do some more debugging to see if I can figure out where
> it's locking up.

Its probably the timer, which uses the ledinfo of the first rule as
private data. Try passing ledinternal to the timer instead of ledinfo,
that should fix it.

Re: [PATCH] Add refcounts to LED target

[Patrick McHardy]

Patrick McHardy wrote:
> Adam Nielsen wrote:
>>> 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.
>> Well I'm afraid I can't see why, but for some reason once I add a second
>> iptables rule with this target my kernel often locks up, hard (audio
>> starts skipping, magic SysRq keys don't work) even if I don't have any
>> actual LED devices on the system.  The first rule, even when attached to
>> a LED device, seems to work fine.
>>
>> I'll have to do some more debugging to see if I can figure out where
>> it's locking up.
> 
> Its probably the timer, which uses the ledinfo of the first rule as
> private data. Try passing ledinternal to the timer instead of ledinfo,
> that should fix it.

I've waited patiently for about a year for this simple problem to
get fixed. Unless I see a patch in time for 2.6.34, I'll remove
the LED module again.

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

On Thursday 2010-03-25 15:01, Patrick McHardy wrote:
>Patrick McHardy wrote:
>> Adam Nielsen wrote:
>>>> 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.
>>> Well I'm afraid I can't see why, but for some reason once I add a second
>>> iptables rule with this target my kernel often locks up, hard (audio
>>> starts skipping, magic SysRq keys don't work) even if I don't have any
>>> actual LED devices on the system.  The first rule, even when attached to
>>> a LED device, seems to work fine.
>>>
>>> I'll have to do some more debugging to see if I can figure out where
>>> it's locking up.
>> 
>> Its probably the timer, which uses the ledinfo of the first rule as
>> private data. Try passing ledinternal to the timer instead of ledinfo,
>> that should fix it.
>
>I've waited patiently for about a year for this simple problem to
>get fixed. Unless I see a patch in time for 2.6.34, I'll remove
>the LED module again.

I want to give this a look, but I notice there's no iptables module
files for xt_LED yet either.

Re: [PATCH] Add refcounts to LED target

[Patrick McHardy]

Jan Engelhardt wrote:
> On Thursday 2010-03-25 15:01, Patrick McHardy wrote:
>> Patrick McHardy wrote:
>>> Adam Nielsen wrote:
>>>>> 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.
>>>> Well I'm afraid I can't see why, but for some reason once I add a second
>>>> iptables rule with this target my kernel often locks up, hard (audio
>>>> starts skipping, magic SysRq keys don't work) even if I don't have any
>>>> actual LED devices on the system.  The first rule, even when attached to
>>>> a LED device, seems to work fine.
>>>>
>>>> I'll have to do some more debugging to see if I can figure out where
>>>> it's locking up.
>>> Its probably the timer, which uses the ledinfo of the first rule as
>>> private data. Try passing ledinternal to the timer instead of ledinfo,
>>> that should fix it.
>> I've waited patiently for about a year for this simple problem to
>> get fixed. Unless I see a patch in time for 2.6.34, I'll remove
>> the LED module again.
> 
> I want to give this a look, but I notice there's no iptables module
> files for xt_LED yet either.

Correct, I didn't add it while this was still unfixed to keep
the possibility of removing the module again. You should be able
to find it in the mailing list archives.

Re: [PATCH] Add refcounts to LED target

[Adam Nielsen]

>>> I've waited patiently for about a year for this simple problem to
>>> get fixed. Unless I see a patch in time for 2.6.34, I'll remove
>>> the LED module again.

I'm really sorry about this, I'll try my best to get this problem fixed in the
next couple of days so it can be finally done and dusted.

Which git repo should I base my patch off?  I was using nf-next-2.6 but this
seems like it's older than the latest 2.6.33.1 release (nf-next-2.6 is missing
an XT_ALIGN at the end which is in 2.6.33.1.)

>> I want to give this a look, but I notice there's no iptables module
>> files for xt_LED yet either.
> 
> Correct, I didn't add it while this was still unfixed to keep
> the possibility of removing the module again. You should be able
> to find it in the mailing list archives.

I can post this again if you'd like to take a look.  I also have a kernel
module that makes your keyboard LEDs available as LED devices if you want to
test it out too.  (I could certainly use some feedback on that!)

Thanks,
Adam.

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

On Saturday 2010-03-27 05:05, Adam Nielsen wrote:

>>>> I've waited patiently for about a year for this simple problem to
>>>> get fixed. Unless I see a patch in time for 2.6.34, I'll remove
>>>> the LED module again.
>
>I'm really sorry about this, I'll try my best to get this problem fixed in the
>next couple of days so it can be finally done and dusted.
>
>Which git repo should I base my patch off?  I was using nf-next-2.6 but this
>seems like it's older than the latest 2.6.33.1 release (nf-next-2.6 is missing
>an XT_ALIGN at the end which is in 2.6.33.1.)

I removed that XT_ALIGN, as it is not necessary on the kernel side.

Re: [PATCH] Add refcounts to LED target

[Adam Nielsen]

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

Add reference counting to the LED target so that multiple targets sharing the
same trigger don't cause any problems.

Signed-off-by: Adam Nielsen <a.nielsen@shikadi.net>

--

> I removed that XT_ALIGN, as it is not necessary on the kernel side.

Thanks Jan, in that case I have attached a patch based on nf-next-2.6.  It
adds the reference counting that was discussed and fixes a bug where it
referenced a null string once the original table/rule was destroyed.

I've tested it and it works well for me, so subject to any feedback I believe
it is ready to be included.

Thanks,
Adam.

[-- Attachment #2: netfilter-leds-add_refcount.patch --]
[-- Type: text/plain, Size: 3479 bytes --]

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index f511bea..bb6e533 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -31,12 +31,17 @@ 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;
+	char *trigger_id;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -53,7 +58,7 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 	 */
 	if ((ledinfo->delay > 0) && ledinfo->always_blink &&
 	    timer_pending(&ledinternal->timer))
-		led_trigger_event(&ledinternal->netfilter_led_trigger,LED_OFF);
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
 
 	led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
 
@@ -74,12 +79,22 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 
 static void led_timeout_callback(unsigned long data)
 {
-	struct xt_led_info *ledinfo = (struct xt_led_info *)data;
-	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
+	struct xt_led_info_internal *ledinternal = (struct xt_led_info_internal *)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,11 +106,20 @@ 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)
 		return false;
 
-	ledinternal->netfilter_led_trigger.name = ledinfo->id;
+	ledinternal->refcnt = 1;
+	ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);
+	strcpy(ledinternal->trigger_id, ledinfo->id);
+	ledinternal->netfilter_led_trigger.name = ledinternal->trigger_id;
 
 	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
 	if (err) {
@@ -108,8 +132,10 @@ static bool led_tg_check(const struct xt_tgchk_param *par)
 	/* See if we need to set up a timer */
 	if (ledinfo->delay > 0)
 		setup_timer(&ledinternal->timer, led_timeout_callback,
-			    (unsigned long)ledinfo);
+			    (unsigned long)ledinternal);
 
+	list_add_tail(&ledinternal->list, &xt_led_triggers);
+out:
 	ledinfo->internal_data = ledinternal;
 
 	return true;
@@ -125,10 +151,15 @@ 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);
 
 	led_trigger_unregister(&ledinternal->netfilter_led_trigger);
+	kfree(ledinternal->trigger_id);
 	kfree(ledinternal);
 }
 

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

On Saturday 2010-03-27 12:39, Adam Nielsen wrote:

>Add reference counting to the LED target so that multiple targets sharing the
>same trigger don't cause any problems.
>
>I have attached a patch based on nf-next-2.6.  It
>adds the reference counting that was discussed and fixes a bug where it
>referenced a null string once the original table/rule was destroyed.
>
>I've tested it and it works well for me, so subject to any feedback I believe
>it is ready to be included.

You need to protect the list_add/list_del from concurrent operation.

You need to check the return value of

ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);

Re: [PATCH v2] Add refcounts to LED target

[Adam Nielsen]

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

Add reference counting to the LED target so that multiple targets sharing the
same trigger don't cause any problems.

Signed-off-by: Adam Nielsen <a.nielsen@shikadi.net>

--

>> I've tested it and it works well for me, so subject to any feedback I believe
>> it is ready to be included.
> 
> You need to protect the list_add/list_del from concurrent operation.
> 
> You need to check the return value of
> 
> ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);

Thanks for looking at it so quickly!  I've attached an updated patch which
hopefully addresses these issues.

Cheers,
Adam.

[-- Attachment #2: netfilter-leds-add_refcount_v2.patch --]
[-- Type: text/plain, Size: 3916 bytes --]

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index f511bea..fece049 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -31,12 +31,18 @@ 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);
+static DEFINE_MUTEX(xt_led_mutex);
+
 /*
  * 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;
+	char *trigger_id;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -53,7 +59,7 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 	 */
 	if ((ledinfo->delay > 0) && ledinfo->always_blink &&
 	    timer_pending(&ledinternal->timer))
-		led_trigger_event(&ledinternal->netfilter_led_trigger,LED_OFF);
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
 
 	led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
 
@@ -74,12 +80,26 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 
 static void led_timeout_callback(unsigned long data)
 {
-	struct xt_led_info *ledinfo = (struct xt_led_info *)data;
-	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
+	struct xt_led_info_internal *ledinternal = (struct xt_led_info_internal *)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;
+
+	mutex_lock(&xt_led_mutex);
+	list_for_each_entry(ledinternal, &xt_led_triggers, list) {
+		if (!strcmp(name, ledinternal->netfilter_led_trigger.name)) {
+			mutex_unlock(&xt_led_mutex);
+			return ledinternal;
+		}
+	}
+	mutex_unlock(&xt_led_mutex);
+	return NULL;
+}
+
 static bool led_tg_check(const struct xt_tgchk_param *par)
 {
 	struct xt_led_info *ledinfo = par->targinfo;
@@ -91,11 +111,23 @@ 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)
 		return false;
 
-	ledinternal->netfilter_led_trigger.name = ledinfo->id;
+	ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);
+	if (!ledinternal->trigger_id)
+		goto exit_internal_alloc;
+
+	ledinternal->refcnt = 1;
+	strcpy(ledinternal->trigger_id, ledinfo->id);
+	ledinternal->netfilter_led_trigger.name = ledinternal->trigger_id;
 
 	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
 	if (err) {
@@ -108,13 +140,21 @@ static bool led_tg_check(const struct xt_tgchk_param *par)
 	/* See if we need to set up a timer */
 	if (ledinfo->delay > 0)
 		setup_timer(&ledinternal->timer, led_timeout_callback,
-			    (unsigned long)ledinfo);
+			    (unsigned long)ledinternal);
 
+	mutex_lock(&xt_led_mutex);
+	list_add_tail(&ledinternal->list, &xt_led_triggers);
+	mutex_unlock(&xt_led_mutex);
+
+out:
 	ledinfo->internal_data = ledinternal;
 
 	return true;
 
 exit_alloc:
+	kfree(ledinternal->trigger_id);
+
+exit_internal_alloc:
 	kfree(ledinternal);
 
 	return false;
@@ -125,10 +165,18 @@ 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;
+
+	mutex_lock(&xt_led_mutex);
+	list_del(&ledinternal->list);
+	mutex_unlock(&xt_led_mutex);
+
 	if (ledinfo->delay > 0)
 		del_timer_sync(&ledinternal->timer);
 
 	led_trigger_unregister(&ledinternal->netfilter_led_trigger);
+	kfree(ledinternal->trigger_id);
 	kfree(ledinternal);
 }
 

Re: [PATCH v2] Add refcounts to LED target

[Jan Engelhardt]

On Sunday 2010-03-28 03:25, Adam Nielsen wrote:

>Add reference counting to the LED target so that multiple targets sharing the
>same trigger don't cause any problems.

I also noticed one another thing: you don't increase the refcount while 
xt_led_mutex is held. That means it is theoretically possible that you 
do a lookup, then a destructor runs and frees the object, leading to 
++ledinternal->refcnt dereference an illegal ledinternal.

Overall, it works pretty much ok.

Re: [PATCH v2] Add refcounts to LED target

[Patrick McHardy]

Jan Engelhardt wrote:
> On Sunday 2010-03-28 03:25, Adam Nielsen wrote:
> 
>> Add reference counting to the LED target so that multiple targets sharing the
>> same trigger don't cause any problems.
> 
> I also noticed one another thing: you don't increase the refcount while 
> xt_led_mutex is held. That means it is theoretically possible that you 
> do a lookup, then a destructor runs and frees the object, leading to 
> ++ledinternal->refcnt dereference an illegal ledinternal.

Indeed, I also noticed this. Basically, you need to make sure that

- the lookup and refcnt increase is atomic,
- the refcnt decrease and list deletion is atomic
- the lookup and list insertion is atomic (in case no trigger exists)

The remaining parts look fine to me, thanks.

Re: [PATCH v3] Add refcounts to LED target

[Adam Nielsen]

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

>> I also noticed one another thing: you don't increase the refcount while 
>> xt_led_mutex is held. That means it is theoretically possible that you 
>> do a lookup, then a destructor runs and frees the object, leading to 
>> ++ledinternal->refcnt dereference an illegal ledinternal.

Thanks both for your comments and explanations.  I've attached an updated
patch, I hope this one addresses these issues.

> Indeed, I also noticed this. Basically, you need to make sure that
> 
> - the lookup and refcnt increase is atomic,
> - the refcnt decrease and list deletion is atomic
> - the lookup and list insertion is atomic (in case no trigger exists)

I've moved the mutex around so that hopefully all these operations are now atomic.

> The remaining parts look fine to me, thanks.

Great, I hope you're happy with this one!

Cheers,
Adam.

[-- Attachment #2: netfilter-leds-add_refcount_v3.patch --]
[-- Type: text/plain, Size: 3998 bytes --]

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index efcf56d..196bf26 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -31,12 +31,18 @@ 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);
+static DEFINE_MUTEX(xt_led_mutex);
+
 /*
  * 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;
+	char *trigger_id;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -53,7 +59,7 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 	 */
 	if ((ledinfo->delay > 0) && ledinfo->always_blink &&
 	    timer_pending(&ledinternal->timer))
-		led_trigger_event(&ledinternal->netfilter_led_trigger,LED_OFF);
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
 
 	led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
 
@@ -74,12 +80,24 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 
 static void led_timeout_callback(unsigned long data)
 {
-	struct xt_led_info *ledinfo = (struct xt_led_info *)data;
-	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
+	struct xt_led_info_internal *ledinternal = (struct xt_led_info_internal *)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)) {
+			mutex_unlock(&xt_led_mutex);
+			return ledinternal;
+		}
+	}
+	return NULL;
+}
+
 static int led_tg_check(const struct xt_tgchk_param *par)
 {
 	struct xt_led_info *ledinfo = par->targinfo;
@@ -91,11 +109,26 @@ static int led_tg_check(const struct xt_tgchk_param *par)
 		return -EINVAL;
 	}
 
+	mutex_lock(&xt_led_mutex);
+
+	ledinternal = led_trigger_lookup(ledinfo->id);
+	if (ledinternal) {
+		ledinternal->refcnt++;
+		goto out;
+	}
+
+	err = -ENOMEM;
 	ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
 	if (!ledinternal)
-		return -ENOMEM;
+		goto exit_mutex_only;
+
+	ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);
+	if (!ledinternal->trigger_id)
+		goto exit_internal_alloc;
 
-	ledinternal->netfilter_led_trigger.name = ledinfo->id;
+	ledinternal->refcnt = 1;
+	strcpy(ledinternal->trigger_id, ledinfo->id);
+	ledinternal->netfilter_led_trigger.name = ledinternal->trigger_id;
 
 	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
 	if (err) {
@@ -108,13 +141,26 @@ static int led_tg_check(const struct xt_tgchk_param *par)
 	/* See if we need to set up a timer */
 	if (ledinfo->delay > 0)
 		setup_timer(&ledinternal->timer, led_timeout_callback,
-			    (unsigned long)ledinfo);
+			    (unsigned long)ledinternal);
+
+	list_add_tail(&ledinternal->list, &xt_led_triggers);
+
+out:
+	mutex_unlock(&xt_led_mutex);
 
 	ledinfo->internal_data = ledinternal;
+
 	return 0;
 
 exit_alloc:
+	kfree(ledinternal->trigger_id);
+
+exit_internal_alloc:
 	kfree(ledinternal);
+
+exit_mutex_only:
+	mutex_unlock(&xt_led_mutex);
+
 	return err;
 }
 
@@ -123,10 +169,23 @@ 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;
 
+	mutex_lock(&xt_led_mutex);
+
+	if (--ledinternal->refcnt) {
+		mutex_unlock(&xt_led_mutex);
+		return;
+	}
+
+	list_del(&ledinternal->list);
+
 	if (ledinfo->delay > 0)
 		del_timer_sync(&ledinternal->timer);
 
 	led_trigger_unregister(&ledinternal->netfilter_led_trigger);
+
+	mutex_unlock(&xt_led_mutex);
+
+	kfree(ledinternal->trigger_id);
 	kfree(ledinternal);
 }
 

Re: [PATCH v3] Add refcounts to LED target

[Patrick McHardy]

Adam Nielsen wrote:
>>> I also noticed one another thing: you don't increase the refcount while 
>>> xt_led_mutex is held. That means it is theoretically possible that you 
>>> do a lookup, then a destructor runs and frees the object, leading to 
>>> ++ledinternal->refcnt dereference an illegal ledinternal.
> 
> Thanks both for your comments and explanations.  I've attached an updated
> patch, I hope this one addresses these issues.
> 
>> Indeed, I also noticed this. Basically, you need to make sure that
>>
>> - the lookup and refcnt increase is atomic,
>> - the refcnt decrease and list deletion is atomic
>> - the lookup and list insertion is atomic (in case no trigger exists)
> 
> I've moved the mutex around so that hopefully all these operations are now atomic.
> 
>> The remaining parts look fine to me, thanks.
> 
> Great, I hope you're happy with this one!

Looks good to me, thanks. Jan, any further issues from your side?
Otherwise I'll apply it.

Re: [PATCH v3] Add refcounts to LED target

[Jan Engelhardt]

On Thursday 2010-04-08 13:33, Patrick McHardy wrote:
>Adam Nielsen wrote:
>>>> I also noticed one another thing: you don't increase the refcount while 
>>>> xt_led_mutex is held. That means it is theoretically possible that you 
>>>> do a lookup, then a destructor runs and frees the object, leading to 
>>>> ++ledinternal->refcnt dereference an illegal ledinternal.
>> 
>> Thanks both for your comments and explanations.  I've attached an updated
>> patch, I hope this one addresses these issues.
>> 
>>> Indeed, I also noticed this. Basically, you need to make sure that
>>>
>>> - the lookup and refcnt increase is atomic,
>>> - the refcnt decrease and list deletion is atomic
>>> - the lookup and list insertion is atomic (in case no trigger exists)
>> 
>> I've moved the mutex around so that hopefully all these operations are now atomic.
>> 
>>> The remaining parts look fine to me, thanks.
>> 
>> Great, I hope you're happy with this one!
>
>Looks good to me, thanks. Jan, any further issues from your side?

None at this time.

Re: [PATCH v3] Add refcounts to LED target

[Patrick McHardy]

Jan Engelhardt wrote:
> On Thursday 2010-04-08 13:33, Patrick McHardy wrote:
>> Adam Nielsen wrote:
>>> I've moved the mutex around so that hopefully all these operations are now atomic.
>>>
>>>> The remaining parts look fine to me, thanks.
>>> Great, I hope you're happy with this one!
>> Looks good to me, thanks. Jan, any further issues from your side?
> 
> None at this time.
> 

Thanks. Adam, please send a formal submission, including a changelog and
a Signed-off-by.

Re: [PATCH v4] Add refcounts to LED target

[Adam Nielsen]

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

Add reference counting to the netfilter LED target, to fix errors when
multiple rules point to the same target ("LED trigger already exists").

Signed-off-by: Adam Nielsen <a.nielsen@shikadi.net>

--

Hopefully this is all you need.  Things like kstrdup() and the mutexes are a
result of needing to remember LED trigger names, which in turn is due to the
introduction of reference counting, so the description above should cover all
changes.

I have attached the patch as my mail client likes to reformat inline patches.

Cheers,
Adam.

[-- Attachment #2: netfilter-leds-add_refcount_v4.patch --]
[-- Type: text/plain, Size: 3905 bytes --]

diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index efcf56d..bd102c7 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -31,12 +31,18 @@ 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);
+static DEFINE_MUTEX(xt_led_mutex);
+
 /*
  * 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;
+	char *trigger_id;
 	struct led_trigger netfilter_led_trigger;
 	struct timer_list timer;
 };
@@ -53,7 +59,7 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 	 */
 	if ((ledinfo->delay > 0) && ledinfo->always_blink &&
 	    timer_pending(&ledinternal->timer))
-		led_trigger_event(&ledinternal->netfilter_led_trigger,LED_OFF);
+		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_OFF);
 
 	led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
 
@@ -74,12 +80,23 @@ led_tg(struct sk_buff *skb, const struct xt_target_param *par)
 
 static void led_timeout_callback(unsigned long data)
 {
-	struct xt_led_info *ledinfo = (struct xt_led_info *)data;
-	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
+	struct xt_led_info_internal *ledinternal = (struct xt_led_info_internal *)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 int led_tg_check(const struct xt_tgchk_param *par)
 {
 	struct xt_led_info *ledinfo = par->targinfo;
@@ -91,11 +108,25 @@ static int led_tg_check(const struct xt_tgchk_param *par)
 		return -EINVAL;
 	}
 
+	mutex_lock(&xt_led_mutex);
+
+	ledinternal = led_trigger_lookup(ledinfo->id);
+	if (ledinternal) {
+		ledinternal->refcnt++;
+		goto out;
+	}
+
+	err = -ENOMEM;
 	ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
 	if (!ledinternal)
-		return -ENOMEM;
+		goto exit_mutex_only;
+
+	ledinternal->trigger_id = kstrdup(ledinfo->id, GFP_KERNEL);
+	if (!ledinternal->trigger_id)
+		goto exit_internal_alloc;
 
-	ledinternal->netfilter_led_trigger.name = ledinfo->id;
+	ledinternal->refcnt = 1;
+	ledinternal->netfilter_led_trigger.name = ledinternal->trigger_id;
 
 	err = led_trigger_register(&ledinternal->netfilter_led_trigger);
 	if (err) {
@@ -108,13 +139,26 @@ static int led_tg_check(const struct xt_tgchk_param *par)
 	/* See if we need to set up a timer */
 	if (ledinfo->delay > 0)
 		setup_timer(&ledinternal->timer, led_timeout_callback,
-			    (unsigned long)ledinfo);
+			    (unsigned long)ledinternal);
+
+	list_add_tail(&ledinternal->list, &xt_led_triggers);
+
+out:
+	mutex_unlock(&xt_led_mutex);
 
 	ledinfo->internal_data = ledinternal;
+
 	return 0;
 
 exit_alloc:
+	kfree(ledinternal->trigger_id);
+
+exit_internal_alloc:
 	kfree(ledinternal);
+
+exit_mutex_only:
+	mutex_unlock(&xt_led_mutex);
+
 	return err;
 }
 
@@ -123,10 +167,23 @@ 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;
 
+	mutex_lock(&xt_led_mutex);
+
+	if (--ledinternal->refcnt) {
+		mutex_unlock(&xt_led_mutex);
+		return;
+	}
+
+	list_del(&ledinternal->list);
+
 	if (ledinfo->delay > 0)
 		del_timer_sync(&ledinternal->timer);
 
 	led_trigger_unregister(&ledinternal->netfilter_led_trigger);
+
+	mutex_unlock(&xt_led_mutex);
+
+	kfree(ledinternal->trigger_id);
 	kfree(ledinternal);
 }
 

Re: [PATCH v4] Add refcounts to LED target

[Patrick McHardy]

Adam Nielsen wrote:
> Add reference counting to the netfilter LED target, to fix errors when
> multiple rules point to the same target ("LED trigger already exists").

Applied, thanks everyone.

Re: [PATCH v3] Add refcounts to LED target

[Florian Westphal]

Adam Nielsen <a.nielsen@shikadi.net> wrote:
> +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)) {
> +			mutex_unlock(&xt_led_mutex);

This mutex_unlock looks out of place...

>  static int led_tg_check(const struct xt_tgchk_param *par)
>  {
>  	struct xt_led_info *ledinfo = par->targinfo;
> @@ -91,11 +109,26 @@ static int led_tg_check(const struct xt_tgchk_param *par)
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&xt_led_mutex);
> +
> +	ledinternal = led_trigger_lookup(ledinfo->id);
> +	if (ledinternal) {
> +		ledinternal->refcnt++;
> +		goto out;

...its unlocked again here via goto.

> +	err = -ENOMEM;
>  	ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
>  	if (!ledinternal)
> -		return -ENOMEM;
> +		goto exit_mutex_only;
> +
> +	ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);

kstrdup()?

Re: [PATCH v3] Add refcounts to LED target

[Adam Nielsen]

>> +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)) {
>> +			mutex_unlock(&xt_led_mutex);
> 
> This mutex_unlock looks out of place...

Oh well spotted, I forgot to take that out when moving the mutex out of this
function.

>> +	ledinternal->trigger_id = kzalloc(strlen(ledinfo->id) + 1, GFP_KERNEL);
> 
> kstrdup()?

Thanks for the suggestion, I will change this to use kstrdup().

Cheers,
Adam.

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

On Saturday 2010-03-27 05:05, Adam Nielsen wrote:
>>> I want to give this a look, but I notice there's no iptables module
>>> files for xt_LED yet either.
>> 
>> Correct, I didn't add it while this was still unfixed to keep
>> the possibility of removing the module again. You should be able
>> to find it in the mailing list archives.
>
>I can post this again if you'd like to take a look.  I also have a kernel
>module that makes your keyboard LEDs available as LED devices if you want to
>test it out too.  (I could certainly use some feedback on that!)

Yes that would be nice, as I don't have any other LED hardware around
other than a cramped WRT54 device.

Re: [PATCH] Add refcounts to LED target

[Adam Nielsen]

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

>>> Correct, I didn't add it while this was still unfixed to keep
>>> the possibility of removing the module again. You should be able
>>> to find it in the mailing list archives.
>>
>> I can post this again if you'd like to take a look.  I also have a kernel
>> module that makes your keyboard LEDs available as LED devices if you want to
>> test it out too.  (I could certainly use some feedback on that!)
> 
> Yes that would be nice, as I don't have any other LED hardware around
> other than a cramped WRT54 device.

I've attached the iptables patch (against the latest iptables git), no change
since last time (other than ignoring xt_LED.h, which is already in git.)

I've uploaded the kernel module to make your keyboard LEDs accessible as LED
devices here: http://www.shikadi.net/files/kernel/leds-input-20100328.tar.bz2

It's still a bit rough, but if you can see any obvious mistakes I've made
please let me know!  There are a couple of minor issues I'm aware of listed at
the top of the .c file.

Cheers,
Adam.

[-- Attachment #2: iptables-LED_target.patch --]
[-- Type: text/plain, Size: 5958 bytes --]

diff --git a/extensions/libxt_LED.c b/extensions/libxt_LED.c
new file mode 100644
index 0000000..6e0566c
--- /dev/null
+++ b/extensions/libxt_LED.c
@@ -0,0 +1,158 @@
+/*
+ * libxt_LED.c - shared library add-on to iptables to add customized LED
+ *               trigger support.
+ *
+ * (C) 2008 Adam Nielsen <a.nielsen@shikadi.net>
+ *
+ * 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.
+ *
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <getopt.h>
+#include <stddef.h>
+
+#include <xtables.h>
+
+#include <linux/netfilter/xt_LED.h>
+
+static const struct option LED_opts[] = {
+	{ .name = "led-trigger-id",	.has_arg = 1, .val = 'i' },
+	{ .name = "led-delay",		.has_arg = 1, .val = 'd' },
+	{ .name = "led-always-blink",	.has_arg = 0, .val = 'a' },
+	{ .name = NULL }
+};
+
+static void LED_help(void)
+{
+	printf(
+"LED target options:\n"
+"--led-trigger-id name           suffix for led trigger name\n"
+"--led-delay ms                  leave the LED on for this number of\n"
+"                                milliseconds after triggering.\n"
+"--led-always-blink              blink on arriving packets, even if\n"
+"                                the LED is already on.\n"
+	);
+}
+
+static int LED_parse(int c, char **argv, int invert, unsigned int *flags,
+                     const void *entry, struct xt_entry_target **target)
+{
+	struct xt_led_info *led = (struct xt_led_info *)(*target)->data;
+
+	switch (c) {
+	case 'i':
+		xtables_param_act(XTF_NO_INVERT, "LED", "--led-trigger-id", invert);
+
+		if (strlen("netfilter-") + strlen(optarg) > sizeof(led->id))
+			xtables_error(PARAMETER_PROBLEM,
+				"--led-trigger-id must be 16 chars or less");
+
+		if (optarg[0] == '\0')
+			xtables_error(PARAMETER_PROBLEM,
+				"--led-trigger-id cannot be blank");
+
+		/* "netfilter-" + 16 char id == 26 == sizeof(led->id) */
+		strcpy(led->id, "netfilter-");
+		strcat(led->id, optarg);
+		*flags = 1;
+		return 1;
+
+	case 'd':
+		xtables_param_act(XTF_NO_INVERT, "LED", "--led-delay", invert);
+
+		if (strncasecmp(optarg, "inf", 3) == 0)
+			led->delay = -1;
+		else
+			led->delay = strtoul(optarg, NULL, 0);
+
+		return 1;
+
+	case 'a':
+		if (!invert)
+			led->always_blink = 1;
+
+		return 1;
+
+	}
+	return 0;
+}
+
+static void LED_final_check(unsigned int flags)
+{
+	if (!flags)
+		xtables_error(PARAMETER_PROBLEM,
+			"--led-trigger-id must be specified");
+}
+
+static void LED_print(const void *ip, const struct xt_entry_target *target,
+                      int numeric)
+{
+	const struct xt_led_info *led = (const struct xt_led_info *)target->data;
+	const char *id = led->id + strlen("netfilter-"); /* trim off prefix */
+
+	printf("led-trigger-id:\"");
+	/* Escape double quotes and backslashes in the ID */
+	while (*id) {
+		if ((*id == '"') || (*id == '\\'))
+			printf("\\");
+		printf("%c", *id++);
+	}
+	printf("\" ");
+
+	if (led->delay == -1)
+		printf("led-delay:inf ");
+	else
+		printf("led-delay:%dms ", led->delay);
+
+	if (led->always_blink)
+		printf("led-always-blink ");
+}
+
+static void LED_save(const void *ip, const struct xt_entry_target *target)
+{
+	const struct xt_led_info *led = (const struct xt_led_info *)target->data;
+	const char *id = led->id + strlen("netfilter-"); /* trim off prefix */
+
+	printf("--led-trigger-id \"");
+	/* Escape double quotes and backslashes in the ID */
+	while (*id) {
+		if ((*id == '"') || (*id == '\\'))
+			printf("\\");
+		printf("%c", *id++);
+	}
+	printf("\" ");
+
+	/* Only print the delay if it's not zero (the default) */
+	if (led->delay > 0)
+		printf("--led-delay %d ", led->delay);
+	else if (led->delay == -1)
+		printf("--led-delay inf ");
+
+	/* Only print always_blink if it's not set to the default */
+	if (led->always_blink)
+		printf("--led-always-blink ");
+}
+
+static struct xtables_target led_tg_reg = {
+	.family		= PF_UNSPEC,
+	.name		= "LED",
+	.version	= XTABLES_VERSION,
+	.size		= XT_ALIGN(sizeof(struct xt_led_info)),
+	.userspacesize	= offsetof(struct xt_led_info, internal_data),
+	.help		= LED_help,
+	.parse		= LED_parse,
+	.final_check	= LED_final_check,
+	.extra_opts	= LED_opts,
+	.print		= LED_print,
+	.save		= LED_save,
+};
+
+void _init(void)
+{
+	xtables_register_target(&led_tg_reg);
+}
diff --git a/extensions/libxt_LED.man b/extensions/libxt_LED.man
new file mode 100644
index 0000000..b65b3c2
--- /dev/null
+++ b/extensions/libxt_LED.man
@@ -0,0 +1,30 @@
+This creates an LED-trigger that can then be attached to system indicator
+lights, to blink or illuminate them when certain packets pass through the
+system.  One example might be to light up an LED for a few minutes every time
+an SSH connection is made to the local machine.  The following options control
+the trigger behaviour:
+.TP
+\fB--led-trigger-id\fP \fIname\fP
+This is the name given to the LED trigger.  The actual name of the trigger
+will be prefixed with "netfilter-".
+.TP
+\fB--led-delay\fP \fIms\fP
+This indicates how long (in milliseconds) the LED should be left illuminated
+when a packet arrives before being switched off again.  The default is 0
+(blink as fast as possible.)  The special value \fIinf\fP can be given to
+leave the LED on permanently once activated.  (In this case the trigger will
+need to be manually detached and reattached to the LED device to switch it
+off again.)
+.TP
+\fB--led-always-blink\fP
+Always make the LED blink on packet arrival, even if the LED is already on.
+This allows notification of new packets even with long delay values (which
+otherwise would result in a silent prolonging of the delay time.)
+.TP
+Example:
+.TP
+Create an LED trigger for incoming SSH traffic:
+iptables -A INPUT -p tcp --dport 22 -j LED --led-trigger-id ssh
+.TP
+Then attach the new trigger to an LED:
+echo netfilter-ssh > /sys/class/leds/<ledname>/trigger

Re: [PATCH] Add refcounts to LED target

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1556 bytes --]


On Sunday 2010-03-28 03:58, Adam Nielsen wrote:
>>>
>>> I can post this again if you'd like to take a look.  I also have a kernel
>>> module that makes your keyboard LEDs available as LED devices if you want to
>>> test it out too.  (I could certainly use some feedback on that!)
>> 
>> Yes that would be nice, as I don't have any other LED hardware around
>> other than a cramped WRT54 device.
>
>I've uploaded the kernel module to make your keyboard LEDs accessible as LED
>devices here: http://www.shikadi.net/files/kernel/leds-input-20100328.tar.bz2

I've got patches for that. (attached)

>It's still a bit rough, but if you can see any obvious mistakes I've made
>please let me know!  There are a couple of minor issues I'm aware of listed at
>the top of the .c file.

>Due to the limited bandwidth available to most input devices, it's
>possible for a fast enough trigger (e.g. netfilter) to saturate the
>link with LED on/off messages, causing a delay between pressing a
>key and having it appear on the screen. With netfilter the
>"--led-delay 1" option solves the problem, but it might be worth
>implementing a delay here too so that it works with other LED
>trigger sources too.

And what I observed is that I get duplicated keypresses when there is
network activity. (UP with CONFIG_PREEMPT=y.) Most likely because the
i8042 keyboard is such a piece of legacy hardware that it is not
worth investigating. I don't have any USB keyboard handy right now,
so testing that needs to wait until after the holidays.

But other than that, does its job.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-patch; name=kbuild.diff, Size: 473 bytes --]

---
 Makefile |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: leds-input/Makefile
===================================================================
--- leds-input.orig/Makefile
+++ leds-input/Makefile
@@ -1,4 +1,9 @@
+KERNEL_DIR := /lib/modules/$(shell uname -r)/build
+
 obj-m := leds-input.o
 
 all:
-	$(MAKE) -C /usr/src/linux M=`pwd` modules
+	$(MAKE) -C ${KERNEL_DIR} M=$$PWD
+
+clean:
+	$(MAKE) -C ${KERNEL_DIR} M=$$PWD $@

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: TEXT/x-patch; name=leds.diff, Size: 2532 bytes --]

---
 leds-input.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Index: leds-input/leds-input.c
===================================================================
--- leds-input.orig/leds-input.c
+++ leds-input/leds-input.c
@@ -55,12 +55,8 @@ struct leddev {
 	char led_name[20]; /* As per led_classdev.name */
 };
 
-/* Figure out if a bit is set in an array of longs */
-#define IS_BIT_SET(buffer, bit) \
-	((((buffer)[BITS_TO_LONGS((bit) + 1) - 1] >> ((bit) % BITS_PER_LONG))) & 1)
-
 /* The order of these mappings must match the LED_* constants in input.h */
-static const char *lednames[] = {
+static const char *const lednames[] = {
 	"num",      /* LED_NUML */
 	"caps",     /* LED_CAPSL */
 	"scroll",   /* LED_SCROLL */
@@ -84,7 +80,7 @@ static const char *lednames[] = {
 static void leds_input_set(struct led_classdev *led_cdev,
 	enum led_brightness value)
 {
-	struct leddev *leddev;
+	const struct leddev *leddev;
 
 	leddev = container_of(led_cdev, struct leddev, led_cdev);
 	input_inject_event(&leddev->handler_data->handle,
@@ -98,14 +94,17 @@ static void leds_input_set(struct led_cl
 /* Called when the LED class driver wants to know this LED's brightness */
 static enum led_brightness leds_input_get(struct led_classdev *led_cdev)
 {
-	struct leddev *leddev = container_of(led_cdev, struct leddev, led_cdev);
+	const struct leddev *leddev =
+		container_of(led_cdev, struct leddev, led_cdev);
 	struct input_dev *indev;
 
-	if (!leddev->handler_data) return LED_OFF; /* too early */
+	if (!leddev->handler_data)
+		return LED_OFF; /* too early */
 	indev = leddev->handler_data->handle.dev;
-	if (!indev) return LED_OFF;
+	if (!indev)
+		return LED_OFF;
 
-	if (IS_BIT_SET(indev->led, leddev->evdev_led_id))
+	if (test_bit(leddev->evdev_led_id, indev->led))
 		return LED_FULL;
 
 	return LED_OFF;
@@ -195,7 +194,7 @@ static int leddev_connect(struct input_h
 	/* Register a device in the LED class for each LED reported by the input layer */
 	newleds = 0;
 	for (i = 0; i < LED_CNT; i++) {
-		if (IS_BIT_SET(dev->ledbit, i)) {
+		if (test_bit(i, dev->ledbit)) {
 			//printk(KERN_DEBUG DRVMSG "has led %d (%s)\n", i, lednames[i]);
 			error = leddev_create(handler_data, handler, dev, i);
 			if (!error)
@@ -283,7 +282,6 @@ static int __init leds_input_init(void)
 static void __exit leds_input_exit(void)
 {
 	input_unregister_handler(&leddev_handler);
-	return;
 }
 
 module_init(leds_input_init);

Re: input-layer LEDs as LED-class devices (was: Add refcounts to LED target)

[Adam Nielsen]

Hi Jan,

>> I've uploaded the kernel module to make your keyboard LEDs accessible as LED
>> devices here: http://www.shikadi.net/files/kernel/leds-input-20100328.tar.bz2
> 
> I've got patches for that. (attached)

Thanks very much for taking the time to review it!  I've included your
changes, apparently I should look harder to see if the kernel already provides
a function before implementing it myself :-)

> And what I observed is that I get duplicated keypresses when there is
> network activity. (UP with CONFIG_PREEMPT=y.) Most likely because the
> i8042 keyboard is such a piece of legacy hardware that it is not
> worth investigating. I don't have any USB keyboard handy right now,
> so testing that needs to wait until after the holidays.

Did you try it with the iptables --led-delay option?  I've only tried it with
USB keyboards but that seems to be required for it to work smoothly
(--led-delay 1 is fine.)  Perhaps a higher delay value is needed for i8042
keyboards?

I'm wondering whether I should implement the delay in the module itself so
that the user doesn't have to worry about it, it just becomes a "slow" LED device.

> But other than that, does its job.

Glad to hear it!  Thanks again for the feedback.

Cheers,
Adam.

Re: input-layer LEDs as LED-class devices (was: Add refcounts to LED target)

[Jan Engelhardt]

On Thursday 2010-04-08 05:15, Adam Nielsen wrote:
>
>> And what I observed is that I get duplicated keypresses when there is
>> network activity. (UP with CONFIG_PREEMPT=y.) Most likely because the
>> i8042 keyboard is such a piece of legacy hardware that it is not
>> worth investigating. I don't have any USB keyboard handy right now,
>> so testing that needs to wait until after the holidays.
>
>Did you try it with the iptables --led-delay option?

Yes.

I would assume that the key repeat happens whenever the LED is
changed *and* I happen to press a key at the same time. This event
happening is IMO independent of the led-delay option, as the LED
state could change either by a delay-induced LED_OFF, or by a new
packet arriving.

> I've only tried it with
>USB keyboards but that seems to be required for it to work smoothly
>(--led-delay 1 is fine.)  Perhaps a higher delay value is needed for i8042
>keyboards?
>
>I'm wondering whether I should implement the delay in the module
>itself so that the user doesn't have to worry about it, it just
>becomes a "slow" LED device.

As long as you don't sleep in interrupt context ;-)