[PATCH 0/5] ulogd2 patches

[PATCH 0/5] ulogd2 patches

[Peter Warasin]

Hi guys

This patchset is against ulogd2 subversion repository.

The actual code in subversion did not work for me with the
following stack:
stack=log1:NFLOG,base1:BASE,ifi2:IFINDEX,print1:PRINTPKT,emu1:SYSLOG
stack=log2:ULOG,base2:BASE,print1:PRINTPKT,emu2:SYSLOG

The patch set fixes some problems after which ulogd2 works for me with
this plugin stack.


Patch #4 is a workaround of the following problem,
of which i am not quite sure is really no problem of
ulogd2, but i think so:

After every successful received packet coming from the ULOG
netlink, namely in the next iteration of ipulog_read() in
ulog_read_cb(), ipulog_read() returns -1 and thus logs an
error.
This happens because the recvfrom() within ipulog_netlink_recvfrom()
fails with EAGAIN, which in reality should'nt, since the
select() should only return when packets are present.

I think it is probably this kernel bug:
http://bugzilla.kernel.org/show_bug.cgi?id=5498

I'm not sure if it is really that bug, but i think so, because
libipulog had virtually no changes between ulogd1 and ulogd2
and ulogd1 does not have this problem. The only thing which
changed is that the netlink socket is now O_NONBLOCK, which
IMHO indicates that it must be that kernel bug.

Probably it is already solved with newer kernel versions.
I tested on 2.6.22.15

BTW: finally managed to set up quilt correctly, so these patches
should now be inline :)

kind regards,
Peter

--

[PATCH 1/5] openlog() to syslog for global ulogd log messages

[Peter Warasin]

[-- Attachment #1: ulogd_general_syslog_fix.patch --]
[-- Type: text/plain, Size: 793 bytes --]

When [global]'s logfile is syslog, ulogd should log it's own
mesasages (not the firewall log lines) to syslog, which it
does'nt because openlog() is missing. This patch adds openlog()

Signed-off-by: Peter Warasin <peter@endian.com>

---
 src/ulogd.c |    1 +
 1 file changed, 1 insertion(+)

Index: ulogd-2.0.0beta1/src/ulogd.c
===================================================================
--- ulogd-2.0.0beta1.orig/src/ulogd.c	2008-01-11 12:44:53.000000000 +0100
+++ ulogd-2.0.0beta1/src/ulogd.c	2008-01-11 12:44:56.000000000 +0100
@@ -738,6 +738,7 @@
 	if (!strcmp(name, "stdout")) {
 		logfile = stdout;
 	} else if (!strcmp(name, "syslog")) {
+                openlog("ulogd", LOG_PID, LOG_DAEMON);
 		logfile = &syslog_dummy;
 	} else {
 		logfile = fopen(ulogd_logfile, "a");

--

Re: [PATCH 1/5] openlog() to syslog for global ulogd log messages

[Patrick McHardy]

Peter Warasin wrote:

 > When [global]'s logfile is syslog, ulogd should log it's own
 > mesasages (not the firewall log lines) to syslog, which it
 > does'nt because openlog() is missing. This patch adds openlog()
 >
 > Signed-off-by: Peter Warasin <peter@endian.com>


Applied, but I changed the spaces used for intending to tabs.

[PATCH 2/5] Dont ntohs() mac_len of ULOG, since it is not in network byte order

[Peter Warasin]

[-- Attachment #1: ulogd_inppkt_ULOG_fix.patch --]
[-- Type: text/plain, Size: 882 bytes --]

Stores mac_len correctly, since within ULOG structire it is not stored
in network byte order.

Signed-off-by: Peter Warasin <peter@endian.com>

---
 input/packet/ulogd_inppkt_ULOG.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c
===================================================================
--- ulogd-2.0.0beta1.orig/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 12:43:49.000000000 +0100
+++ ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 12:58:50.000000000 +0100
@@ -167,7 +167,7 @@
 	if (pkt->mac_len) {
 		ret[ULOG_KEY_RAW_MAC].u.value.ptr = pkt->mac;
 		ret[ULOG_KEY_RAW_MAC].flags |= ULOGD_RETF_VALID;
-		ret[ULOG_KEY_RAW_MAC_LEN].u.value.ui16 = ntohs(pkt->mac_len);
+		ret[ULOG_KEY_RAW_MAC_LEN].u.value.ui16 = pkt->mac_len;
 		ret[ULOG_KEY_RAW_MAC_LEN].flags |= ULOGD_RETF_VALID;
 	}
 

--

Re: [PATCH 2/5] Dont ntohs() mac_len of ULOG, since it is not in network byte order

[Patrick McHardy]

Peter Warasin wrote:

 > Stores mac_len correctly, since within ULOG structire it is not stored
 > in network byte order.
 >
 > Signed-off-by: Peter Warasin <peter@endian.com>

Applied, thanks Peter.

[PATCH 3/5] Logs also ipulog_strerror() and strerror() within ULOG plugin

[Peter Warasin]

[-- Attachment #1: ulogd_inppkt_ULOG_strerror.patch --]
[-- Type: text/plain, Size: 1054 bytes --]

This patch logs also the string representations ipulog:_strerror()
and strerror() when an error occurred during receivement of packets
within the ULOG plugin

Signed-off-by: Peter Warasin <peter@endian.com>

---
 input/packet/ulogd_inppkt_ULOG.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c
===================================================================
--- ulogd-2.0.0beta1.orig/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 12:58:50.000000000 +0100
+++ ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 13:10:44.000000000 +0100
@@ -227,8 +227,11 @@
 		if (len <= 0) {
 			/* this is not supposed to happen */
 			ulogd_log(ULOGD_ERROR, "ipulog_read = %d! "
-				  "ipulog_errno = %d, errno = %d\n",
-				  len, ipulog_errno, errno);
+				  "ipulog_errno = %d (%s), "
+				  "errno = %d (%s)\n",
+				  len, ipulog_errno,
+				  ipulog_strerror(ipulog_errno),
+				  errno, strerror(errno));
 			break;
 		}
 		while ((upkt = ipulog_get_packet(u->libulog_h,

--

Re: [PATCH 3/5] Logs also ipulog_strerror() and strerror() within ULOG plugin

[Patrick McHardy]

Peter Warasin wrote:

 > This patch logs also the string representations ipulog:_strerror()
 > and strerror() when an error occurred during receivement of packets
 > within the ULOG plugin
 >
 > Signed-off-by: Peter Warasin <peter@endian.com>

Applied, thanks.

[PATCH 4/5] Workaround of recvfrom() EAGAIN bug

[Peter Warasin]

[-- Attachment #1: ulogd_inppkt_ULOG_recvfrom_EAGAIN_workaround.patch --]
[-- Type: text/plain, Size: 1207 bytes --]

This is a workaround which prevents ulogd from logging each
time when recvfrom() returns error because of EAGAIN.
Since the netlink socket is now O_NONBLOCK, we probably run
into the following bug:

http://bugzilla.kernel.org/show_bug.cgi?id=5498

which causes recvfrom() get an error when select() had a good
return, whenever select() receives a packet with a bad checksum.

ipulog_read() always has this problem once after every successful
ipulog_read().

Signed-off-by: Peter Warasin <peter@endian.com>

---
 input/packet/ulogd_inppkt_ULOG.c |    2 ++
 1 file changed, 2 insertions(+)

Index: ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c
===================================================================
--- ulogd-2.0.0beta1.orig/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 13:11:20.000000000 +0100
+++ ulogd-2.0.0beta1/input/packet/ulogd_inppkt_ULOG.c	2008-01-11 13:11:54.000000000 +0100
@@ -225,6 +225,8 @@
 	while ((len = ipulog_read(u->libulog_h, u->libulog_buf,
 				 upi->config_kset->ces[0].u.value, 1))) {
 		if (len <= 0) {
+			if (errno == EAGAIN)
+				break;
 			/* this is not supposed to happen */
 			ulogd_log(ULOGD_ERROR, "ipulog_read = %d! "
 				  "ipulog_errno = %d (%s), "

--

Re: [PATCH 4/5] Workaround of recvfrom() EAGAIN bug

[Patrick McHardy]

Peter Warasin wrote:

 > This is a workaround which prevents ulogd from logging each
 > time when recvfrom() returns error because of EAGAIN.
 > Since the netlink socket is now O_NONBLOCK, we probably run
 > into the following bug:
 > ...

This is not a workaround, but correct handling of EAGAIN on
a non-blocking socket. This happens because we loop until
there is no more data in the receive-queue.

Applied, thanks.

[PATCH 5/5] stores the converted syslog parameters set within config file

[Peter Warasin]

[-- Attachment #1: ulogd_output_SYSLOG_configure_fix.patch --]
[-- Type: text/plain, Size: 1000 bytes --]

This patch stores the converted values from the configuration file
into the syslog instance structure.
Otherwise configuration parameters are senseless and only the
default values will be used.

Signed-off-by: Peter Warasin <peter@endian.com>

---
 output/ulogd_output_SYSLOG.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: ulogd-2.0.0beta1/output/ulogd_output_SYSLOG.c
===================================================================
--- ulogd-2.0.0beta1.orig/output/ulogd_output_SYSLOG.c	2008-01-11 13:12:27.000000000 +0100
+++ ulogd-2.0.0beta1/output/ulogd_output_SYSLOG.c	2008-01-11 13:24:18.000000000 +0100
@@ -87,6 +87,7 @@
 {
 	int syslog_facility, syslog_level;
 	char *facility, *level;
+	struct syslog_instance *li = (struct syslog_instance *) &pi->private;
 
 	/* FIXME: error handling */
 	config_parse_file(pi->id, pi->config_kset);
@@ -144,6 +145,9 @@
 		return -EINVAL;
 	}
 
+	li->syslog_level = syslog_level;
+	li->syslog_facility = syslog_facility;
+
 	return 0;
 }
 

--

Re: [PATCH 5/5] stores the converted syslog parameters set within config file

[Patrick McHardy]

Peter Warasin wrote:

 > This patch stores the converted values from the configuration file
 > into the syslog instance structure.
 > Otherwise configuration parameters are senseless and only the
 > default values will be used.
 >
 > Signed-off-by: Peter Warasin <peter@endian.com>

Applied, thanks.