ulogd2 and nflog prefix

ulogd2 and nflog prefix

[Philip Craig]

The log message prefix contained in the netlink message is
not null terminated, but nflog_get_prefix() returns a pointer
to the data in the netlink message, and ulogd2 treats it as
null terminated.

I can create a patch to fix this if I know how it is meant to work:
- null terminate in kernel?
- nflog_get_prefix() should copy and null terminate?
- add nflog_get_prefix_len() and handle in ulogd?

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Philip Craig wrote:
> The log message prefix contained in the netlink message is
> not null terminated, but nflog_get_prefix() returns a pointer
> to the data in the netlink message, and ulogd2 treats it as
> null terminated.
> 
> I can create a patch to fix this if I know how it is meant to work:
> - null terminate in kernel?
> - nflog_get_prefix() should copy and null terminate?
> - add nflog_get_prefix_len() and handle in ulogd?


Most other netlink users in the kernel dealing with strings use
rtattr_strlcpy these days, which automatically terminates strings.
I think we should do the same for consistency.

Re: ulogd2 and nflog prefix

[Philip Craig]

On 05/15/2006 03:36 PM, Patrick McHardy wrote:
> Most other netlink users in the kernel dealing with strings use
> rtattr_strlcpy these days, which automatically terminates strings.
> I think we should do the same for consistency.

rtattr_strlcpy seems to be for getting the string out of a message,
not for creating a message.  It copies the string and adds a null
terminator.  The terminator in the message is optional.

Or do you mean that we should use rtattr_strlcpy in userspace?

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Philip Craig wrote:
> On 05/15/2006 03:36 PM, Patrick McHardy wrote:
> 
>>Most other netlink users in the kernel dealing with strings use
>>rtattr_strlcpy these days, which automatically terminates strings.
>>I think we should do the same for consistency.
> 
> 
> rtattr_strlcpy seems to be for getting the string out of a message,
> not for creating a message.  It copies the string and adds a null
> terminator.  The terminator in the message is optional.


Right, I somehow though the prefix was also configured using netlink,
which is of course wrong. In that case I think nfnetlink_log should
just 0-terminate the string. BTW, Is there any reason why the prefix
is truncated to NFULNL_PREFIXLEN?

Re: ulogd2 and nflog prefix

[Philip Craig]

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

On 05/15/2006 05:06 PM, Patrick McHardy wrote:
> Right, I somehow though the prefix was also configured using netlink,
> which is of course wrong. In that case I think nfnetlink_log should
> just 0-terminate the string. BTW, Is there any reason why the prefix
> is truncated to NFULNL_PREFIXLEN?

It makes calculating the size to allocate simpler, but otherwise it
looks like a carryover from when log messages were passed via fixed
size structures.  Giving the full prefix is desirable I think,
especially for log messages generated internally by netfilter.
eg the sequence number check in ip_conntrack_proto_tcp.c

Here's a patch against 2.6.16, tested with ulogd.

[-- Attachment #2: nflog-prefix.patch --]
[-- Type: text/plain, Size: 2382 bytes --]

Null terminate the prefix in netfilter netlink log messages,
and remove the artificial limit on the prefix length.

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.x/include/linux/netfilter/nfnetlink_log.h
===================================================================
RCS file: /cvs/sw/linux-2.6.x/include/linux/netfilter/nfnetlink_log.h,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 nfnetlink_log.h
--- linux-2.6.x/include/linux/netfilter/nfnetlink_log.h	28 Oct 2005 04:39:21 -0000	1.1.1.1
+++ linux-2.6.x/include/linux/netfilter/nfnetlink_log.h	15 May 2006 07:16:22 -0000
@@ -32,8 +32,6 @@ struct nfulnl_msg_packet_timestamp {
 	aligned_u64	usec;
 } __attribute__ ((packed));
 
-#define NFULNL_PREFIXLEN	30	/* just like old log target */
-
 enum nfulnl_attr_type {
 	NFULA_UNSPEC,
 	NFULA_PACKET_HDR,
Index: linux-2.6.x/net/netfilter/nfnetlink_log.c
===================================================================
RCS file: /cvs/sw/linux-2.6.x/net/netfilter/nfnetlink_log.c,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 nfnetlink_log.c
--- linux-2.6.x/net/netfilter/nfnetlink_log.c	21 Mar 2006 01:35:42 -0000	1.1.1.3
+++ linux-2.6.x/net/netfilter/nfnetlink_log.c	15 May 2006 07:16:22 -0000
@@ -410,12 +410,8 @@ __build_packet_message(struct nfulnl_ins
 
 	NFA_PUT(inst->skb, NFULA_PACKET_HDR, sizeof(pmsg), &pmsg);
 
-	if (prefix) {
-		int slen = strlen(prefix);
-		if (slen > NFULNL_PREFIXLEN)
-			slen = NFULNL_PREFIXLEN;
-		NFA_PUT(inst->skb, NFULA_PREFIX, slen, prefix);
-	}
+	if (prefix)
+		NFA_PUT(inst->skb, NFULA_PREFIX, strlen(prefix) + 1, prefix);
 
 	if (indev) {
 		tmp_uint = htonl(indev->ifindex);
@@ -585,7 +581,7 @@ nfulnl_log_packet(unsigned int pf,
 		return;
 	}
 
-	/* all macros expand to constant values at compile time */
+	/* most macros expand to constant values at compile time */
 	/* FIXME: do we want to make the size calculation conditional based on
 	 * what is actually present?  way more branches and checks, but more
 	 * memory efficient... */
@@ -599,7 +595,7 @@ nfulnl_log_packet(unsigned int pf,
 #endif
 		+ NFA_SPACE(sizeof(u_int32_t))	/* mark */
 		+ NFA_SPACE(sizeof(u_int32_t))	/* uid */
-		+ NFA_SPACE(NFULNL_PREFIXLEN)	/* prefix */
+		+ NFA_SPACE(strlen(prefix) + 1)	/* prefix */
 		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_hw))
 		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_timestamp));
 

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Philip Craig wrote:
> On 05/15/2006 05:06 PM, Patrick McHardy wrote:
> 
>>Right, I somehow though the prefix was also configured using netlink,
>>which is of course wrong. In that case I think nfnetlink_log should
>>just 0-terminate the string. BTW, Is there any reason why the prefix
>>is truncated to NFULNL_PREFIXLEN?
> 
> 
> It makes calculating the size to allocate simpler, but otherwise it
> looks like a carryover from when log messages were passed via fixed
> size structures.  Giving the full prefix is desirable I think,
> especially for log messages generated internally by netfilter.
> eg the sequence number check in ip_conntrack_proto_tcp.c

I agree. A few nitpicks though ..

> Index: linux-2.6.x/include/linux/netfilter/nfnetlink_log.h
> ===================================================================
> RCS file: /cvs/sw/linux-2.6.x/include/linux/netfilter/nfnetlink_log.h,v
> retrieving revision 1.1.1.1
> diff -u -p -r1.1.1.1 nfnetlink_log.h
> --- linux-2.6.x/include/linux/netfilter/nfnetlink_log.h	28 Oct 2005 04:39:21 -0000	1.1.1.1
> +++ linux-2.6.x/include/linux/netfilter/nfnetlink_log.h	15 May 2006 07:16:22 -0000
> @@ -32,8 +32,6 @@ struct nfulnl_msg_packet_timestamp {
>  	aligned_u64	usec;
>  } __attribute__ ((packed));
>  
> -#define NFULNL_PREFIXLEN	30	/* just like old log target */
> -

Please keep this around, userspace might be using it.


>  enum nfulnl_attr_type {
>  	NFULA_UNSPEC,
>  	NFULA_PACKET_HDR,
> Index: linux-2.6.x/net/netfilter/nfnetlink_log.c
> ===================================================================
> RCS file: /cvs/sw/linux-2.6.x/net/netfilter/nfnetlink_log.c,v
> retrieving revision 1.1.1.3
> diff -u -p -r1.1.1.3 nfnetlink_log.c
> --- linux-2.6.x/net/netfilter/nfnetlink_log.c	21 Mar 2006 01:35:42 -0000	1.1.1.3
> +++ linux-2.6.x/net/netfilter/nfnetlink_log.c	15 May 2006 07:16:22 -0000
> @@ -410,12 +410,8 @@ __build_packet_message(struct nfulnl_ins
>  
>  	NFA_PUT(inst->skb, NFULA_PACKET_HDR, sizeof(pmsg), &pmsg);
>  
> -	if (prefix) {
> -		int slen = strlen(prefix);
> -		if (slen > NFULNL_PREFIXLEN)
> -			slen = NFULNL_PREFIXLEN;
> -		NFA_PUT(inst->skb, NFULA_PREFIX, slen, prefix);
> -	}
> +	if (prefix)
> +		NFA_PUT(inst->skb, NFULA_PREFIX, strlen(prefix) + 1, prefix);
>  
>  	if (indev) {
>  		tmp_uint = htonl(indev->ifindex);
> @@ -585,7 +581,7 @@ nfulnl_log_packet(unsigned int pf,
>  		return;
>  	}
>  
> -	/* all macros expand to constant values at compile time */
> +	/* most macros expand to constant values at compile time */
>  	/* FIXME: do we want to make the size calculation conditional based on
>  	 * what is actually present?  way more branches and checks, but more
>  	 * memory efficient... */
> @@ -599,7 +595,7 @@ nfulnl_log_packet(unsigned int pf,
>  #endif
>  		+ NFA_SPACE(sizeof(u_int32_t))	/* mark */
>  		+ NFA_SPACE(sizeof(u_int32_t))	/* uid */
> -		+ NFA_SPACE(NFULNL_PREFIXLEN)	/* prefix */
> +		+ NFA_SPACE(strlen(prefix) + 1)	/* prefix */
>  		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_hw))
>  		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_timestamp));
>  

Please try to avoid multiple strlen calls .. just pass it around as an
argument or something.

Re: ulogd2 and nflog prefix

[Philip Craig]

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

On 05/15/2006 05:39 PM, Patrick McHardy wrote:
> I agree. A few nitpicks though ..

Okay, here's a better version.


[-- Attachment #2: nflog-prefix.patch --]
[-- Type: text/plain, Size: 2381 bytes --]

Null terminate the prefix in netfilter netlink log messages,
and remove the artificial limit on the prefix length.

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.x/net/netfilter/nfnetlink_log.c
===================================================================
RCS file: /cvs/sw/linux-2.6.x/net/netfilter/nfnetlink_log.c,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 nfnetlink_log.c
--- linux-2.6.x/net/netfilter/nfnetlink_log.c	21 Mar 2006 01:35:42 -0000	1.1.1.3
+++ linux-2.6.x/net/netfilter/nfnetlink_log.c	15 May 2006 07:54:59 -0000
@@ -386,7 +386,8 @@ __build_packet_message(struct nfulnl_ins
 			const struct net_device *indev,
 			const struct net_device *outdev,
 			const struct nf_loginfo *li,
-			const char *prefix)
+			const char *prefix,
+			unsigned int prefix_len)
 {
 	unsigned char *old_tail;
 	struct nfulnl_msg_packet_hdr pmsg;
@@ -410,12 +411,8 @@ __build_packet_message(struct nfulnl_ins
 
 	NFA_PUT(inst->skb, NFULA_PACKET_HDR, sizeof(pmsg), &pmsg);
 
-	if (prefix) {
-		int slen = strlen(prefix);
-		if (slen > NFULNL_PREFIXLEN)
-			slen = NFULNL_PREFIXLEN;
-		NFA_PUT(inst->skb, NFULA_PREFIX, slen, prefix);
-	}
+	if (prefix)
+		NFA_PUT(inst->skb, NFULA_PREFIX, prefix_len, prefix);
 
 	if (indev) {
 		tmp_uint = htonl(indev->ifindex);
@@ -565,7 +562,7 @@ nfulnl_log_packet(unsigned int pf,
 		  const struct nf_loginfo *li_user,
 		  const char *prefix)
 {
-	unsigned int size, data_len;
+	unsigned int size, data_len, prefix_len;
 	struct nfulnl_instance *inst;
 	const struct nf_loginfo *li;
 	unsigned int qthreshold;
@@ -599,10 +596,15 @@ nfulnl_log_packet(unsigned int pf,
 #endif
 		+ NFA_SPACE(sizeof(u_int32_t))	/* mark */
 		+ NFA_SPACE(sizeof(u_int32_t))	/* uid */
-		+ NFA_SPACE(NFULNL_PREFIXLEN)	/* prefix */
 		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_hw))
 		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_timestamp));
 
+	if (prefix) {
+		prefix_len = strlen(prefix) + 1;
+		size += NFA_SPACE(prefix_len);
+	} else
+		prefix_len = 0;
+
 	UDEBUG("initial size=%u\n", size);
 
 	spin_lock_bh(&inst->lock);
@@ -665,7 +667,7 @@ nfulnl_log_packet(unsigned int pf,
 	inst->qlen++;
 
 	__build_packet_message(inst, skb, data_len, pf,
-				hooknum, in, out, li, prefix);
+				hooknum, in, out, li, prefix, prefix_len);
 
 	/* timer_pending always called within inst->lock, so there
 	 * is no chance of a race here */

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Philip Craig wrote:
> On 05/15/2006 05:39 PM, Patrick McHardy wrote:
> 
>>I agree. A few nitpicks though ..
> 
> 
> Okay, here's a better version.

Thanks. Harald, any objections to me applying this patch?

Re: ulogd2 and nflog prefix

[Philip Craig]

On 05/15/2006 05:06 PM, Patrick McHardy wrote:
> Right, I somehow though the prefix was also configured using netlink,
> which is of course wrong. In that case I think nfnetlink_log should
> just 0-terminate the string. BTW, Is there any reason why the prefix
> is truncated to NFULNL_PREFIXLEN?

I just noticed that net/netfilter/nf_log.c limits the prefix to
NF_LOG_PREFIXLEN=128.  Don't know if this affects the decision.

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Philip Craig wrote:
> On 05/15/2006 05:06 PM, Patrick McHardy wrote:
> 
>>Right, I somehow though the prefix was also configured using netlink,
>>which is of course wrong. In that case I think nfnetlink_log should
>>just 0-terminate the string. BTW, Is there any reason why the prefix
>>is truncated to NFULNL_PREFIXLEN?
> 
> 
> I just noticed that net/netfilter/nf_log.c limits the prefix to
> NF_LOG_PREFIXLEN=128.  Don't know if this affects the decision.


Not really - if there is a second limit the minimum will be effective,
but that alone is no reason to truncate it.

#define NFULNL_PREFIXLEN        30      /* just like old log target */

I guess this tells us why the limit exists, but since nfnetlink_log
is not compatible with the old ULOG structure anyway, this could
just as well be done by the compat library.

Re: ulogd2 and nflog prefix

[Harald Welte]

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

On Mon, May 15, 2006 at 10:31:03AM +0200, Patrick McHardy wrote:
> Philip Craig wrote:
> > On 05/15/2006 05:39 PM, Patrick McHardy wrote:
> > 
> >>I agree. A few nitpicks though ..
> > 
> > 
> > Okay, here's a better version.
> 
> Thanks. Harald, any objections to me applying this patch?

no, not at all. thanks.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ulogd2 and nflog prefix

[Patrick McHardy]

Harald Welte wrote:
> On Mon, May 15, 2006 at 10:31:03AM +0200, Patrick McHardy wrote:
> 
>>Philip Craig wrote:
>>
>>>Okay, here's a better version.
>>
>>Thanks. Harald, any objections to me applying this patch?
> 
> 
> no, not at all. thanks.

OK, queued for 2.6.18. Thanks.