Re: [PATCH v2] Add refcounts to LED target

[Adam Nielsen <a.nielsen@shikadi.net>]

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