[PATCH 1/2] New netfilter target to trigger LED devices

[PATCH 1/2] New netfilter target to trigger LED devices

[Adam Nielsen]

Add a new "LED" target to iptables, which allows LEDs to blink in
response to matching rules.

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

---

Hi all,

This is my first attempt at a proper patch, so please be nice :-)

The patch below (and the kernel module coming shortly) adds support
for a LED target in netfilter.  This allows netfilter to blink LEDs
on your system in response to certain packets passing through the
machine.  For example, you could have an LED flashing to indicate
how much HTTP traffic is coming into a server, or you might have an
LED light up for a few minutes whenever someone connects to your
machine via SSH.

It uses the "led" class of hardware to provide the interface to the
LEDs themselves, so you'll need at least one device that appears in
/sys/class/leds/ to test this (and unfortunately keyboard LEDs don't
appear there...yet.)

The patch here is against iptables git as of a couple of hours ago,
but it only adds two new files so it should apply without problems.

I'll post the kernel module in the next e-mail.

There are however a couple of outstanding issues I'd appreciate some
advice with:

Firstly, I've declared xt_led_info in both the iptables library and the
kernel module.  This probably isn't ideal, but I wanted to avoid having
to create another header file, which presumably needs to be installed on
the user's system before iptables can be compiled.  Please let me know
what the preferred solution to this might be.

Secondly, I'm not sure whether setting xtables_target.family to
PF_UNSPEC is correct.  Given that the process of blinking lights isn't
related to any particular network protocol, I wanted the LED target to
be available in as many cases as possible.  I think PF_UNSPEC does this,
but please advise if there is a more suitable alternative.

I hope these patches are at least suitable for your consideration!

Cheers,
Adam.

diff --git a/extensions/libxt_LED.c b/extensions/libxt_LED.c
new file mode 100644
index 0000000..f59c42e
--- /dev/null
+++ b/extensions/libxt_LED.c
@@ -0,0 +1,182 @@
+/*
+ * libxt_LED.c - shared library add-on to iptables to add customized LED
+ *               trigger support.
+ *
+ * (C) 2008 Adam Nielsen <a.nielsen@shikadi.net>
+ *
+ * This allows you to blink LEDs on your system in response to incoming
+ * network packets.
+ *
+ * Create a LED trigger for incoming SSH traffic:
+ *   iptables -A INPUT -p tcp --dport 22 -j LED --led-trigger-id ssh
+ *
+ * Then attach the new trigger to a LED on your system:
+ *   echo netfilter-ssh > /sys/class/leds/<ledname>/trigger
+ *
+ * See /usr/src/linux/Documentation/leds-class.txt for LED details.
+ *
+ * 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 <xtables.h>
+
+struct xt_led_info {
+	char id[26];  /* Unique ID for this trigger in the LED class */
+	int delay;    /* Delay until LED is switched off again after trigger */
+
+	void *internal_data; /* Kernel data used in the module */
+};
+
+static const struct option LED_opts[] = {
+	{ .name = "led-trigger-id", .has_arg = 1, .val = 'i' },
+	{ .name = "led-delay", .has_arg = 1, .val = 'd' },
+	{ .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.  Defaults\n"
+"                                to 0ms (blink as fast as possible.)\n"
+"                                Set to \"inf\" to stay on.  Incoming\n"
+"                                packets while the LED is on will still\n"
+"                                cause it to briefly blink off.\n");
+}
+
+static void LED_init(struct xt_entry_target *t)
+{
+	struct xt_led_info *led = (struct xt_led_info *)t->data;
+
+	/* defaults */
+	led->id[0] = 0;
+	led->delay = 0;
+
+	return;
+}
+
+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':
+		if (check_inverse(optarg, &invert, NULL, 0))
+			exit_error(PARAMETER_PROBLEM,
+				"Unexpected `!' after --led-trigger-id");
+
+		if (strlen(optarg) > 15)
+			exit_error(PARAMETER_PROBLEM,
+				"--led-trigger-id must be 16 chars or less");
+
+		if (optarg[0] == 0)
+			exit_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':
+		if (check_inverse(optarg, &invert, NULL, 0))
+			exit_error(PARAMETER_PROBLEM,
+				"Unexpected `!' after --led-delay");
+
+		if (strncasecmp(optarg, "inf", 3) == 0)
+			led->delay = -1;
+		else
+			led->delay = atoi(optarg);
+
+		return 1;
+
+	default:
+		break;
+
+	}
+	return 0;
+}
+
+static void LED_final_check(unsigned int flags)
+{
+	if (!flags)
+		exit_error(PARAMETER_PROBLEM, "--led-trigger-id must be specified");
+	return;
+}
+
+/* Print our config data, e.g. with iptables -L -v */
+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 + 10; /* trim off "netfilter-" prefix */
+	printf("led-trigger-id:'");
+	/* Some not so great code to escape single quotes in the ID */
+	while (*id) {
+		if (*id == '\'')
+			printf("\\'");
+		else
+			printf("%c", *id);
+		id++;
+	}
+	if (led->delay == -1) printf("' led-delay:inf ");
+	else printf("' led-delay:%dms ", led->delay);
+	return;
+}
+
+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 + 10; /* trim off "netfilter-" prefix */
+
+	/* The same not so great code to escape single quotes in the ID */
+	printf("--led-trigger-id '");
+	while (*id) {
+		if (*id == '\'')
+			printf("\\'");
+		else
+			printf("%c", *id);
+		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 ");
+
+	return;
+}
+
+static struct xtables_target led_tg_reg = {
+	.family		= PF_UNSPEC,
+	.name		= "LED",
+	.version	= XTABLES_VERSION,
+	.size		= XT_ALIGN(sizeof(struct xt_led_info)),
+	.userspacesize	= XT_ALIGN(sizeof(struct xt_led_info)),
+	.help		= LED_help,
+	.init		= LED_init,
+	.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..96dd17c
--- /dev/null
+++ b/extensions/libxt_LED.man
@@ -0,0 +1,19 @@
+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
+.BI "--led-trigger-id " "name"
+This is the name given to the LED trigger.  The actual name of the trigger
+will have the prefix "netfilter-".
+.TP
+.BI "--led-delay " "ms"
+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
+.I inf
+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.)
+.PP

Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Jan Engelhardt]

On Sunday 2008-11-09 14:19, Adam Nielsen wrote:

> Firstly, I've declared xt_led_info in both the iptables library and the
> kernel module.  This probably isn't ideal, but I wanted to avoid having
> to create another header file, which presumably needs to be installed on
> the user's system before iptables can be compiled.  Please let me know
> what the preferred solution to this might be.

One solution is to have a file <linux/netfilter/xt_LED.h>
in the kernel tree that is copied to the iptables tree
at iptables/include/linux/netfilter/xt_LED.h.

But you just gave me something to consider, so keep it as it is right now.

> Secondly, I'm not sure whether setting xtables_target.family to
> PF_UNSPEC is correct.  Given that the process of blinking lights isn't
> related to any particular network protocol, I wanted the LED target to
> be available in as many cases as possible.  I think PF_UNSPEC does this,
> but please advise if there is a more suitable alternative.

It is correct.

> @@ -0,0 +1,182 @@
> +/*
> + * libxt_LED.c - shared library add-on to iptables to add customized LED
> + *               trigger support.
> + *
> + * (C) 2008 Adam Nielsen <a.nielsen@shikadi.net>
> + *
> + * This allows you to blink LEDs on your system in response to incoming
> + * network packets.
> + *
> + * Create a LED trigger for incoming SSH traffic:
> + *   iptables -A INPUT -p tcp --dport 22 -j LED --led-trigger-id ssh
> + *
> + * Then attach the new trigger to a LED on your system:
> + *   echo netfilter-ssh > /sys/class/leds/<ledname>/trigger
> + *
> + * See /usr/src/linux/Documentation/leds-class.txt for LED details.

I do not think the help text is needed in code (either iptables
or kernel). It should be in the iptables manpage, i.e.
libxt_LED.man.

> +struct xt_led_info {
> +	char id[26];  /* Unique ID for this trigger in the LED class */
> +	int delay;    /* Delay until LED is switched off again after trigger */
> +
> +	void *internal_data; /* Kernel data used in the module */
> +};

Move it to xt_LED.h.

> +static void LED_init(struct xt_entry_target *t)
> +{
> +	struct xt_led_info *led = (struct xt_led_info *)t->data;
> +
> +	/* defaults */
> +	led->id[0] = 0;
> +	led->delay = 0;
> +
> +	return;
> +}

It is by default initialized to zero, no need to do it yourself again.
(As such the entire function cen be gotten rid of.)

> +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':
> +		if (check_inverse(optarg, &invert, NULL, 0))
> +			exit_error(PARAMETER_PROBLEM,
> +				"Unexpected `!' after --led-trigger-id");

Just remove check_inverse - no more intraposition negation support.
Inversion is indicated by the "invert" variable already.

> +		if (strlen(optarg) > 15)
> +			exit_error(PARAMETER_PROBLEM,
> +				"--led-trigger-id must be 16 chars or less");

Why 15, if struct xt_led_info->name is 26 in size? Oh 'tis confusing!
Try using
	strlen("netfilter-") + strlen(optarg) >= sizeof(led->name) instead.

> +	case 'd':
> +		if (check_inverse(optarg, &invert, NULL, 0))
> +			exit_error(PARAMETER_PROBLEM,
> +				"Unexpected `!' after --led-delay");

Remove check_inverse.

> +		if (strncasecmp(optarg, "inf", 3) == 0)
> +			led->delay = -1;
> +		else
> +			led->delay = atoi(optarg);

strto(u)l, for great justice :)

> +	default:
> +		break;
> +

No need for a blank default.

> +static void LED_final_check(unsigned int flags)
> +{
> +	if (!flags)
> +		exit_error(PARAMETER_PROBLEM, "--led-trigger-id must be
> specified");
> +	return;
> +}

-return

> +/* Print our config data, e.g. with iptables -L -v */
> +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 + 10; /* trim off "netfilter-" prefix */
> +	printf("led-trigger-id:'");
> +	/* Some not so great code to escape single quotes in the ID */

iptables-restore does not recognize single-quoted words, you will
have to use double quotes.

Or just forbid quote characters altogether?

> +	while (*id) {
> +		if (*id == '\'')
> +			printf("\\'");
> +		else
> +			printf("%c", *id);
> +		id++;
> +	}
> +	if (led->delay == -1) printf("' led-delay:inf ");
> +	else printf("' led-delay:%dms ", led->delay);
> +	return;
> +}
> +
> +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 + 10; /* trim off "netfilter-" prefix */

strlen("netfilter-");  Compiler is smart enough to optimize it.

> +
> +	/* The same not so great code to escape single quotes in the ID */
> +	printf("--led-trigger-id '");
> +	while (*id) {
> +		if (*id == '\'')
> +			printf("\\'");
> +		else
> +			printf("%c", *id);
> +		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 ");
> +
> +	return;
> +}
> +
> +static struct xtables_target led_tg_reg = {
> +	.family		= PF_UNSPEC,
> +	.name		= "LED",
> +	.version	= XTABLES_VERSION,
> +	.size		= XT_ALIGN(sizeof(struct xt_led_info)),
> +	.userspacesize	= XT_ALIGN(sizeof(struct xt_led_info)),

Wow did you actually try this? Since you need...

	.userspacesize = offsetof(struct xt_led_info, internal_data)

> +	.help		= LED_help,
> +	.init		= LED_init,
> +	.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..96dd17c
> --- /dev/null
> +++ b/extensions/libxt_LED.man
> @@ -0,0 +1,19 @@
> +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
> +.BI "--led-trigger-id " "name"
> +This is the name given to the LED trigger.  The actual name of the trigger
> +will have the prefix "netfilter-".
> +.TP
> +.BI "--led-delay " "ms"
> +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
> +.I inf
> +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.)
> +.PP

I prefer \fB/\fI over .B/.I because they read more like inline HTML,
but that's me.

No trailing .PP.

Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Adam Nielsen]

>> Firstly, I've declared xt_led_info in both the iptables library and the
>> kernel module.  This probably isn't ideal, but I wanted to avoid having
>> to create another header file, which presumably needs to be installed on
>> the user's system before iptables can be compiled.  Please let me know
>> what the preferred solution to this might be.
> 
> One solution is to have a file <linux/netfilter/xt_LED.h>
> in the kernel tree that is copied to the iptables tree
> at iptables/include/linux/netfilter/xt_LED.h.
> 
> But you just gave me something to consider, so keep it as it is right now.

Let me know what you decide!

> I do not think the help text is needed in code (either iptables
> or kernel). It should be in the iptables manpage, i.e.
> libxt_LED.man.

That's true, it's just that as a user I often go looking in the code when
I can't find any other documentation.  Are those example commands suitable
for inclusion in the manpage?  I think they're worth keeping somewhere.

>> +		if (check_inverse(optarg, &invert, NULL, 0))
>> +			exit_error(PARAMETER_PROBLEM,
>> +				"Unexpected `!' after --led-trigger-id");
> 
> Just remove check_inverse - no more intraposition negation support.
> Inversion is indicated by the "invert" variable already.

Does this mean I can remove those whole three lines?  Or do I still need to
check the value of "invert" and complain if it's specified?

>> +		if (strlen(optarg) > 15)
> 
> Why 15, if struct xt_led_info->name is 26 in size? Oh 'tis confusing!
> Try using
> 	strlen("netfilter-") + strlen(optarg) >= sizeof(led->name) instead.

Ah ok, I didn't realise that the compiler can optimise away the strlen()
call.  In that case I don't have any problem with doing it that way.

>> +			led->delay = atoi(optarg);
> 
> strto(u)l, for great justice :)

No problem.  What's the benefit of using strto(u)l over atoi()?

> iptables-restore does not recognize single-quoted words, you will
> have to use double quotes.
> 
> Or just forbid quote characters altogether?

I was going to forbid quote characters, but then I thought that people
may want to use extended characters (dollar signs perhaps, or other
characters that could be interpreted by the shell) so I thought
quoting the output would be less restrictive.

Maybe I should only allow alphanumerics and basic punctuation?

>> +	.size		= XT_ALIGN(sizeof(struct xt_led_info)),
>> +	.userspacesize	= XT_ALIGN(sizeof(struct xt_led_info)),
> 
> Wow did you actually try this? Since you need...
> 
> 	.userspacesize = offsetof(struct xt_led_info, internal_data)

Ah ok, I found a PDF of yours that explains this properly, I
misinterpreted the purpose of the field (I thought it was comparing
existing valid structures together, rather than comparing a new
structure against an existing one.)  I think this worked for me because
I'm in the habit of removing rules by index (iptables -D INPUT 1)

>> +.BI "--led-delay " "ms"
> 
> I prefer \fB/\fI over .B/.I because they read more like inline HTML,
> but that's me.
> 
> No trailing .PP.

That's fine, I've never used whatever that markup is before (I don't
even know what it's called...!)  I just edited one of the other target
pages.

Thanks again for your feedback.  I'll wait a day or two in case
anyone else has any other comments and then resubmit.

Cheers,
Adam.

Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Jan Engelhardt]

On Monday 2008-11-10 11:34, Adam Nielsen wrote:
>
> That's true, it's just that as a user I often go looking in the code when
> I can't find any other documentation.  Are those example commands suitable
> for inclusion in the manpage?  I think they're worth keeping somewhere.

Yes. These commands should be made available to iptables users --
not developers (or those wanting to be one) to find it by chance
in net/netfilter/Kconfig.

>> > +		if (check_inverse(optarg, &invert, NULL, 0))
>> > +			exit_error(PARAMETER_PROBLEM,
>> > +				"Unexpected `!' after --led-trigger-id");
>> 
>> Just remove check_inverse - no more intraposition negation support.
>> Inversion is indicated by the "invert" variable already.
>
> Does this mean I can remove those whole three lines?  Or do I still need to
> check the value of "invert" and complain if it's specified?

You still need to check for it. If you grep for P_NO_INVERT in all .c
files you will find the shortest possible line to do so.

>
>> > +		if (strlen(optarg) > 15)
>> 
>> Why 15, if struct xt_led_info->name is 26 in size? Oh 'tis confusing!
>> Try using
>>  strlen("netfilter-") + strlen(optarg) >= sizeof(led->name) instead.
>
> Ah ok, I didn't realise that the compiler can optimise away the strlen()
> call.  In that case I don't have any problem with doing it that way.

Even if the compiler could not, you could use sizeof("foo")-1 to get "3".
That is a property of the C language.

>> > +			led->delay = atoi(optarg);
>> 
>> strto(u)l, for great justice :)
>
> No problem.  What's the benefit of using strto(u)l over atoi()?

Error detection, says the manual page. It is actually a good thing
to go with the consistency even if you do not care about errors.

>> iptables-restore does not recognize single-quoted words, you will
>> have to use double quotes.
>> 
>> Or just forbid quote characters altogether?
>
> I was going to forbid quote characters, but then I thought that people
> may want to use extended characters (dollar signs perhaps, or other
> characters that could be interpreted by the shell) so I thought
> quoting the output would be less restrictive.
>
> Maybe I should only allow alphanumerics and basic punctuation?

I thought about that - but I have come to the conclusion that most
character should be allowed so that people can use all the
extended characters like CJK.

>> I prefer \fB/\fI over .B/.I because they read more like inline HTML,
>> but that's me.
>> 
>> No trailing .PP.
>
> That's fine, I've never used whatever that markup is before

>(I don't even know what it's called...!)

Read the PDF to the end and you will find out ;-)

Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Adam Nielsen]

>> Does this mean I can remove those whole three lines?  Or do I still need to
>> check the value of "invert" and complain if it's specified?
> 
> You still need to check for it. If you grep for P_NO_INVERT in all .c
> files you will find the shortest possible line to do so.

Ah ok, that makes sense.  On a related note, am I doing something wrong
here?  Even when I'm not listing my target explicitly, I can get an odd
error:

$ iptables -A INPUT -j ACCEPT --test
iptables v1.4.2: Unknown arg `(null)'

> Even if the compiler could not, you could use sizeof("foo")-1 to get "3".
> That is a property of the C language.

Ha, been programming in it for years and never thought of doing that :-)

>> Maybe I should only allow alphanumerics and basic punctuation?
> 
> I thought about that - but I have come to the conclusion that most
> character should be allowed so that people can use all the
> extended characters like CJK.

So if I switch to using double-quotes then, is there anything else I'll
need to escape? (such as '$')

>> That's fine, I've never used whatever that markup is before
>> (I don't even know what it's called...!)
> 
> Read the PDF to the end and you will find out ;-)

Ah, I see now :-)  That's a very useful and well written PDF too, btw.

Cheers,
Adam.

Re: [PATCH 1/2] New netfilter target to trigger LED devices

[Jan Engelhardt]

On Monday 2008-11-10 12:55, Adam Nielsen wrote:

>> > Does this mean I can remove those whole three lines?  Or do I still need to
>> > check the value of "invert" and complain if it's specified?
>> 
>> You still need to check for it. If you grep for P_NO_INVERT in all .c
>> files you will find the shortest possible line to do so.
>
> Ah ok, that makes sense.  On a related note, am I doing something wrong
> here?  Even when I'm not listing my target explicitly, I can get an odd
> error:
>
> $ iptables -A INPUT -j ACCEPT --test
> iptables v1.4.2: Unknown arg `(null)'

This is a known one. IIRC Pablo addressed this with a patch (but I don't 
see it in the git repo!?)


>> > Maybe I should only allow alphanumerics and basic punctuation?
>> 
>> I thought about that - but I have come to the conclusion that most
>> character should be allowed so that people can use all the
>> extended characters like CJK.
>
> So if I switch to using double-quotes then, is there anything else I'll
> need to escape? (such as '$')

Nope. Just '"', and obviously '\'.