[PATCH 4/*] libnfnetlink fixes

[PATCH 4/*] libnfnetlink fixes

[Pablo Neira]

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

This fixes:

a) Check for bad file descriptor, otherwise this can result in a 
infinite loop during event display cancelation.

b) Return 0 on success. -100 is ugly and we this since we use explicit 
ACK confirmation (NLM_F_ACK) since it's cleaner.

c) nfnl_nest and nfnl_nest_end to build up nested attributes.

d) NLMSG_TAIL introduced in recent rtnetlinkv2 changes.

[-- Attachment #2: 04libnfnetlink.patch --]
[-- Type: text/x-patch, Size: 2305 bytes --]

Index: libnfnetlink.c
===================================================================
--- libnfnetlink.c	(revision 4067)
+++ libnfnetlink.c	(working copy)
@@ -108,10 +108,11 @@
  */
 int nfnl_close(struct nfnl_handle *nfnlh)
 {
-	if (nfnlh->fd)
-		close(nfnlh->fd);
+	int err;
+	
+	err = close(nfnlh->fd);
 
-	return 0;
+	return err;
 }
 
 /**
@@ -202,6 +203,9 @@
 		if (remain < 0) {
 			if (errno == EINTR)
 				continue;
+			/* Bad file descriptor */
+			if (errno == EBADF)
+				break;
 			nfnl_error("recvmsg overrun");
 			continue;
 		}
@@ -231,7 +235,7 @@
 
 			/* end of messages reached, let's return */
 			if (h->nlmsg_type == NLMSG_DONE)
-				return -100;
+				return 0;
 
 			/* Break the loop if success is explicitely
 			 * reported via NLM_F_ACK flag set */
@@ -400,12 +404,11 @@
 		return -1;
 	}
 
-	nfa = (struct nfattr *)(((char *)n) + NLMSG_ALIGN(n->nlmsg_len));
+	nfa = NLMSG_TAIL(n);
 	nfa->nfa_type = type;
 	nfa->nfa_len = len;
 	memcpy(NFA_DATA(nfa), data, alen);
-	n->nlmsg_len = (NLMSG_ALIGN(n->nlmsg_len) + len);
-
+	n->nlmsg_len = (NLMSG_ALIGN(n->nlmsg_len) + NFA_ALIGN(len));
 	return 0;
 }
 
@@ -482,7 +485,7 @@
 {
 	while (NFA_OK(nfa, len)) {
 		if (nfa->nfa_type <= max)
-			tb[nfa->nfa_type] = nfa;
+			tb[nfa->nfa_type-1] = nfa;
                 nfa = NFA_NEXT(nfa,len);
 	}
 	if (len)
Index: libnfnetlink.h
===================================================================
--- libnfnetlink.h	(revision 4067)
+++ libnfnetlink.h	(working copy)
@@ -10,6 +10,9 @@
 #include <linux/netlink.h>
 #include <linux/netfilter/nfnetlink.h>
 
+#define NLMSG_TAIL(nlh) \
+	((void *) (nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)
+
 #define NFNL_BUFFSIZE		8192
 
 struct nfnl_handle {
@@ -45,6 +48,12 @@
 extern int nfnl_nfa_addattr_l(struct nfattr *, int, int, void *, int);
 extern int nfnl_nfa_addattr32(struct nfattr *, int, int, u_int32_t);
 extern int nfnl_parse_attr(struct nfattr **, int, struct nfattr *, int);
+#define nfnl_nest(nlh, bufsize, type) 				\
+({	struct nfattr *__start = NLMSG_TAIL(nlh);		\
+	nfnl_addattr_l(nlh, bufsize, type, NULL, 0); 		\
+	__start; })
+#define nfnl_nest_end(nlh, tail) 				\
+({	(tail)->nfa_len = (void *) NLMSG_TAIL(nlh) - (void *) tail; })
 
 extern void nfnl_dump_packet(struct nlmsghdr *, int, char *);
 #endif /* __LIBNFNETLINK_H */

Re: [PATCH 4/*] libnfnetlink fixes

[Amin Azez]

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

Here is a patch I propose for libnfnetlink.c

It allows the application-supplied callback handler to signifiy in its
return code whether or not the nfnl_listen read-loop should terminate.

If the handler returns a negative code, then nfnl_listen returns
immediately with the same return code.

If the handler returns a postitive code, then nfnl_listen returns at the
next iteration of the while loop, which is when it finishes handing all
netlink messages in the current packet.

This avois the need to duplicate most of nfnl_listen in a non-loop
context where the application needs control over the read and packet
decode process.

I use it to break out of the loop when a signal handler sets a flag,
once the loop has broken I then do some per-signal processing and
re-enter the loop.

Sam


[-- Attachment #2: libnfnetlink.c.diff --]
[-- Type: text/x-patch, Size: 924 bytes --]

Index: /opt/KERNEL/SVN/libnfnetlink/libnfnetlink.c
===================================================================
--- /opt/KERNEL/SVN/libnfnetlink/libnfnetlink.c	(revision 3897)
+++ /opt/KERNEL/SVN/libnfnetlink/libnfnetlink.c	(working copy)
@@ -184,6 +193,7 @@
 	int remain;
 	struct nlmsghdr *h;
 	struct nlmsgerr *msgerr;
+	int quit=0;
 
 	struct msghdr msg = {
 		(void *)&nladdr, sizeof(nladdr),
@@ -197,7 +207,7 @@
 	iov.iov_base = buf;
 	iov.iov_len = sizeof(buf);
 
-	while (1) {
+	while (! quit) {
 		remain = recvmsg(nfnlh->fd, &msg, 0);
 		if (remain < 0) {
 			if (errno == EINTR)
@@ -243,6 +253,7 @@
 			err = handler(&nladdr, h, jarg);
 			if (err < 0)
 				return err;
+			quit |= err;
 		
 			/* FIXME: why not _NEXT macros, etc.? */
 			//h = NLMSG_NEXT(h, remain);
@@ -259,7 +270,7 @@
 		}
 	}
 
-	return 0;
+	return quit;
 }
 
 int nfnl_talk(struct nfnl_handle *nfnlh, struct nlmsghdr *n, pid_t peer,

Re: [PATCH 4/*] libnfnetlink fixes

[Harald Welte]

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

On Wed, Jul 13, 2005 at 09:23:56AM +0100, Amin Azez wrote:

> It allows the application-supplied callback handler to signifiy in its
> return code whether or not the nfnl_listen read-loop should terminate.
> 
> If the handler returns a negative code, then nfnl_listen returns
> immediately with the same return code.
> 
> If the handler returns a postitive code, then nfnl_listen returns at the
> next iteration of the while loop, which is when it finishes handing all
> netlink messages in the current packet.
> 
> This avois the need to duplicate most of nfnl_listen in a non-loop
> context where the application needs control over the read and packet
> decode process.

I think this change is fine.  Would you mind to re-submit the patch
against current svn?  Please also document the meaning of the return
values, probably with a comment in the code or the header file?

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: [PATCH 4/*] libnfnetlink fixes

[Amin Azez]

Harald Welte wrote:

>On Wed, Jul 13, 2005 at 09:23:56AM +0100, Amin Azez wrote:
>
>  
>
>I think this change is fine.  Would you mind to re-submit the patch
>against current svn?  Please also document the meaning of the return
>values, probably with a comment in the code or the header file?
>
>  
>
Will do (Monday)

thanks for the feedback.

Azez

Re: [PATCH 4/*] libnfnetlink fixes

[Harald Welte]

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

On Fri, Sep 02, 2005 at 05:12:38PM +0100, Amin Azez wrote:
> Harald Welte wrote:
> 
> >On Wed, Jul 13, 2005 at 09:23:56AM +0100, Amin Azez wrote:
> >
> >  
> >
> >I think this change is fine.  Would you mind to re-submit the patch
> >against current svn?  Please also document the meaning of the return
> >values, probably with a comment in the code or the header file?
> >
> >  
> >
> Will do (Monday)
> 
> thanks for the feedback.

I haven't received an updated patch so far. Did I miss it somehow?

-- 
- 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: [PATCH 4/*] libnfnetlink fixes

[Amin Azez]

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

Harald Welte wrote:

>On Fri, Sep 02, 2005 at 05:12:38PM +0100, Amin Azez wrote:
>  
>
>>Harald Welte wrote:
>>
>>    
>>
>>>On Wed, Jul 13, 2005 at 09:23:56AM +0100, Amin Azez wrote:
>>>
>>> 
>>>
>>>I think this change is fine.  Would you mind to re-submit the patch
>>>against current svn?  Please also document the meaning of the return
>>>values, probably with a comment in the code or the header file?
>>>
>>> 
>>>
>>>      
>>>
>>Will do (Monday)
>>
>>thanks for the feedback.
>>    
>>
>
>I haven't received an updated patch so far. Did I miss it somehow?
>
>  
>
err... you must have, I sent it on 5th Sept.

Attached again.

Azez


[-- Attachment #2: libnfnetlink.c.diff --]
[-- Type: text/x-patch, Size: 1691 bytes --]

Index: src/libnfnetlink.c
===================================================================
--- src/libnfnetlink.c	(revision 4254)
+++ src/libnfnetlink.c	(working copy)
@@ -257,6 +257,16 @@
  *
  * nfnhl: libnfnetlink handle
  * handler: callback function to be called for every netlink message
+ *          - the callback handler should normally return 0
+ *          - but may return a negative error code which will cause
+ *            nfnl_listen to return immediately with the same error code
+ *          - or return a postivie error code which will cause 
+ *            nfnl_listen to return after it has finished processing all
+ *            the netlink messages in the current packet
+ *          Thus a positive error code will terminate nfnl_listen "soon"
+ *          without any loss of data, a negative error code will terminate
+ *          nfnl_listen "very soon" and throw away data already read from
+ *          the netlink socket.
  * jarg: opaque argument passed on to callback
  *
  */
@@ -270,6 +280,7 @@
 	int remain;
 	struct nlmsghdr *h;
 	struct nlmsgerr *msgerr;
+	int quit=0;
 
 	struct msghdr msg = {
 		(void *)&nladdr, sizeof(nladdr),
@@ -283,7 +294,7 @@
 	iov.iov_base = buf;
 	iov.iov_len = sizeof(buf);
 
-	while (1) {
+	while (! quit) {
 		remain = recvmsg(nfnlh->fd, &msg, 0);
 		if (remain < 0) {
 			if (errno == EINTR)
@@ -332,6 +343,7 @@
 			err = handler(&nladdr, h, jarg);
 			if (err < 0)
 				return err;
+			quit |= err;
 		
 			/* FIXME: why not _NEXT macros, etc.? */
 			//h = NLMSG_NEXT(h, remain);
@@ -348,7 +360,7 @@
 		}
 	}
 
-	return 0;
+	return quit;
 }
 
 int nfnl_talk(struct nfnl_handle *nfnlh, struct nlmsghdr *n, pid_t peer,

Re: [PATCH 4/*] libnfnetlink fixes

[Harald Welte]

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

On Mon, Sep 12, 2005 at 09:03:25AM +0100, Amin Azez wrote:

> err... you must have, I sent it on 5th Sept.

sorry.  I've applied it now (Committed revision 4258.)
-- 
- 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: [PATCH 4/*] libnfnetlink fixes

[Amin Azez]

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

Attached, with explanatory notes in the function comment.

Sam

Harald Welte wrote:
> On Wed, Jul 13, 2005 at 09:23:56AM +0100, Amin Azez wrote:
> 
> 
>>It allows the application-supplied callback handler to signifiy in its
>>return code whether or not the nfnl_listen read-loop should terminate.
>>
>>If the handler returns a negative code, then nfnl_listen returns
>>immediately with the same return code.
>>
>>If the handler returns a postitive code, then nfnl_listen returns at the
>>next iteration of the while loop, which is when it finishes handing all
>>netlink messages in the current packet.
>>
>>This avois the need to duplicate most of nfnl_listen in a non-loop
>>context where the application needs control over the read and packet
>>decode process.
> 
> 
> I think this change is fine.  Would you mind to re-submit the patch
> against current svn?  Please also document the meaning of the return
> values, probably with a comment in the code or the header file?
> 
> Thanks!
> 


[-- Attachment #2: libnfnetlink.c.diff --]
[-- Type: text/x-patch, Size: 1691 bytes --]

Index: src/libnfnetlink.c
===================================================================
--- src/libnfnetlink.c	(revision 4254)
+++ src/libnfnetlink.c	(working copy)
@@ -257,6 +257,16 @@
  *
  * nfnhl: libnfnetlink handle
  * handler: callback function to be called for every netlink message
+ *          - the callback handler should normally return 0
+ *          - but may return a negative error code which will cause
+ *            nfnl_listen to return immediately with the same error code
+ *          - or return a postivie error code which will cause 
+ *            nfnl_listen to return after it has finished processing all
+ *            the netlink messages in the current packet
+ *          Thus a positive error code will terminate nfnl_listen "soon"
+ *          without any loss of data, a negative error code will terminate
+ *          nfnl_listen "very soon" and throw away data already read from
+ *          the netlink socket.
  * jarg: opaque argument passed on to callback
  *
  */
@@ -270,6 +280,7 @@
 	int remain;
 	struct nlmsghdr *h;
 	struct nlmsgerr *msgerr;
+	int quit=0;
 
 	struct msghdr msg = {
 		(void *)&nladdr, sizeof(nladdr),
@@ -283,7 +294,7 @@
 	iov.iov_base = buf;
 	iov.iov_len = sizeof(buf);
 
-	while (1) {
+	while (! quit) {
 		remain = recvmsg(nfnlh->fd, &msg, 0);
 		if (remain < 0) {
 			if (errno == EINTR)
@@ -332,6 +343,7 @@
 			err = handler(&nladdr, h, jarg);
 			if (err < 0)
 				return err;
+			quit |= err;
 		
 			/* FIXME: why not _NEXT macros, etc.? */
 			//h = NLMSG_NEXT(h, remain);
@@ -348,7 +360,7 @@
 		}
 	}
 
-	return 0;
+	return quit;
 }
 
 int nfnl_talk(struct nfnl_handle *nfnlh, struct nlmsghdr *n, pid_t peer,

Re: [PATCH 4/*] libnfnetlink fixes

[Harald Welte]

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

On Tue, Jul 12, 2005 at 10:37:13PM +0200, Pablo Neira wrote:
> This fixes:

Thanks, applied with some minor cosmetic changes.
-- 
- 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 --]