Skip to contentSkip to conversation

libnfnetlink fixes

PATCH Applied Pablo Neira 3 participants 9 messages Jul 12, 2005 – Sep 13, 2005 netfilter

lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 1/1 libnfnetlink fixes

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.

libnfnetlink.c +11 -8
Index: libnfnetlink.c
===================================================================
@@ -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)
libnfnetlink.h +9 -0
Index: libnfnetlink.h
===================================================================
@@ -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 */
#1 lore raw

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

opt/KERNEL/SVN/libnfnetlink/libnfnetlink.c +4 -2
Index: /opt/KERNEL/SVN/libnfnetlink/libnfnetlink.c
===================================================================
@@ -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,
Applied
#8 lore raw
In reply to Pablo Neira
Quoted 1 line · Pablo Neira

This fixes:

Thanks, applied with some minor cosmetic changes.

Signature · 5 lines
- Harald Welte <[email protected]>                 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
#2 lore raw
In reply to Amin Azez
Quoted 6 lines · Amin Azez

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.

Quoted 7 lines

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!

Signature · 5 lines
- Harald Welte <[email protected]>                 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
#3 lore raw
Quoted 10 lines · Harald Welte

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

#7 lore raw
In reply to Harald Welte

Attached, with explanatory notes in the function comment.

Sam

Quoted 18 lines · Harald Welte
Quoted 6 lines · Amin Azez

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.

Quoted 7 lines

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.

Quoted 6 lines

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!

libnfnetlink.c +14 -2
Index: src/libnfnetlink.c
===================================================================
@@ -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,
#4 lore raw
In reply to Amin Azez
Quoted 12 lines · Amin Azez
Quoted 10 lines · Harald Welte

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?

Quoted 3 lines

Will do (Monday)

thanks for the feedback.

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

Signature · 5 lines
- Harald Welte <[email protected]>                 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
#5 lore raw
Quoted 19 lines · Harald Welte
Quoted 16 lines · Amin Azez
Quoted 12 lines · Harald Welte

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?

Quoted 10 lines
Quoted 5 lines

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

libnfnetlink.c +14 -2
Index: src/libnfnetlink.c
===================================================================
@@ -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,
#6 lore raw
Quoted 1 line · Amin Azez

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

sorry. I've applied it now (Committed revision 4258.)

Signature · 5 lines
- Harald Welte <[email protected]>                 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