[PATCH 0/5] [PATCHSET] Netlink Patches

[PATCH 0/5] [PATCHSET] Netlink Patches

[Thomas Graf]

Converts westwood and vegas netlink code to use the typesafe
interface, removes an unused varaible, and move some netlink
queue management code into the generic layer.

[PATCH 1/5] [TCP] vegas: Use type safe netlink interface

[Thomas Graf]

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

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/net/ipv4/tcp_vegas.c
===================================================================
--- net-2.6.22.orig/net/ipv4/tcp_vegas.c	2007-03-20 23:53:19.000000000 +0100
+++ net-2.6.22/net/ipv4/tcp_vegas.c	2007-03-21 01:10:15.000000000 +0100
@@ -341,16 +341,14 @@ static void tcp_vegas_get_info(struct so
 {
 	const struct vegas *ca = inet_csk_ca(sk);
 	if (ext & (1 << (INET_DIAG_VEGASINFO - 1))) {
-		struct tcpvegas_info *info;
+		struct tcpvegas_info info = {
+			.tcpv_enabled = ca->doing_vegas_now,
+			.tcpv_rttcnt = ca->cntRTT,
+			.tcpv_rtt = ca->baseRTT,
+			.tcpv_minrtt = ca->minRTT,
+		};
 
-		info = RTA_DATA(__RTA_PUT(skb, INET_DIAG_VEGASINFO,
-					  sizeof(*info)));
-
-		info->tcpv_enabled = ca->doing_vegas_now;
-		info->tcpv_rttcnt = ca->cntRTT;
-		info->tcpv_rtt = ca->baseRTT;
-		info->tcpv_minrtt = ca->minRTT;
-	rtattr_failure:	;
+		nla_put(skb, INET_DIAG_VEGASINFO, sizeof(info), &info);
 	}
 }
 

--

[PATCH 2/5] [TCP] westwood: Use type safe netlink interface

[Thomas Graf]

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

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/net/ipv4/tcp_westwood.c
===================================================================
--- net-2.6.22.orig/net/ipv4/tcp_westwood.c	2007-03-20 23:53:19.000000000 +0100
+++ net-2.6.22/net/ipv4/tcp_westwood.c	2007-03-21 01:10:16.000000000 +0100
@@ -260,16 +260,13 @@ static void tcp_westwood_info(struct soc
 {
 	const struct westwood *ca = inet_csk_ca(sk);
 	if (ext & (1 << (INET_DIAG_VEGASINFO - 1))) {
-		struct rtattr *rta;
-		struct tcpvegas_info *info;
+		struct tcpvegas_info info = {
+			.tcpv_enabled = 1,
+			.tcpv_rtt = jiffies_to_usecs(ca->rtt),
+			.tcpv_minrtt = jiffies_to_usecs(ca->rtt_min),
+		};
 
-		rta = __RTA_PUT(skb, INET_DIAG_VEGASINFO, sizeof(*info));
-		info = RTA_DATA(rta);
-		info->tcpv_enabled = 1;
-		info->tcpv_rttcnt = 0;
-		info->tcpv_rtt = jiffies_to_usecs(ca->rtt);
-		info->tcpv_minrtt = jiffies_to_usecs(ca->rtt_min);
-	rtattr_failure:	;
+		nla_put(skb, INET_DIAG_VEGASINFO, sizeof(info), &info);
 	}
 }
 

--

[PATCH 3/5] [NETLINK]: Remove unused groups variable

[Thomas Graf]

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

Leftover from dynamic multicast groups allocation work.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6/net/netlink/af_netlink.c
===================================================================
--- net-2.6.orig/net/netlink/af_netlink.c	2007-01-23 14:09:34.000000000 +0100
+++ net-2.6/net/netlink/af_netlink.c	2007-01-23 14:09:49.000000000 +0100
@@ -396,7 +396,6 @@ static int netlink_create(struct socket 
 {
 	struct module *module = NULL;
 	struct netlink_sock *nlk;
-	unsigned int groups;
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -418,7 +417,6 @@ static int netlink_create(struct socket 
 	if (nl_table[protocol].registered &&
 	    try_module_get(nl_table[protocol].module))
 		module = nl_table[protocol].module;
-	groups = nl_table[protocol].groups;
 	netlink_unlock_table();
 
 	if ((err = __netlink_create(sock, protocol)) < 0)

--

[PATCH 4/5] [NETLINK]: Ignore !NLM_F_REQUEST messages directly in netlink_run_queue()

[Thomas Graf]

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

netlink_rcv_skb() is changed to skip messages which don't have the
NLM_F_REQUEST bit to avoid every netlink family having to perform this
check on their own.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/net/netlink/af_netlink.c
===================================================================
--- net-2.6.22.orig/net/netlink/af_netlink.c	2007-03-21 01:16:39.000000000 +0100
+++ net-2.6.22/net/netlink/af_netlink.c	2007-03-21 01:17:13.000000000 +0100
@@ -1470,10 +1470,15 @@ static int netlink_rcv_skb(struct sk_buf
 
 	while (skb->len >= nlmsg_total_size(0)) {
 		nlh = nlmsg_hdr(skb);
+		err = 0;
 
 		if (nlh->nlmsg_len < NLMSG_HDRLEN || skb->len < nlh->nlmsg_len)
 			return 0;
 
+		/* Only requests are handled by the kernel */
+		if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
+			goto skip;
+
 		if (cb(skb, nlh, &err) < 0) {
 			/* Not an error, but we have to interrupt processing
 			 * here. Note: that in this case we do not pull
@@ -1481,9 +1486,10 @@ static int netlink_rcv_skb(struct sk_buf
 			 */
 			if (err == 0)
 				return -1;
+		}
+skip:
+		if (nlh->nlmsg_flags & NLM_F_ACK || err)
 			netlink_ack(skb, nlh, err);
-		} else if (nlh->nlmsg_flags & NLM_F_ACK)
-			netlink_ack(skb, nlh, 0);
 
 		netlink_queue_skip(nlh, skb);
 	}
Index: net-2.6.22/net/xfrm/xfrm_user.c
===================================================================
--- net-2.6.22.orig/net/xfrm/xfrm_user.c	2007-03-20 23:53:19.000000000 +0100
+++ net-2.6.22/net/xfrm/xfrm_user.c	2007-03-21 01:17:13.000000000 +0100
@@ -1859,9 +1859,6 @@ static int xfrm_user_rcv_msg(struct sk_b
 	struct xfrm_link *link;
 	int type, min_len;
 
-	if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
-		return 0;
-
 	type = nlh->nlmsg_type;
 
 	/* A control message: ignore them */
Index: net-2.6.22/net/netlink/genetlink.c
===================================================================
--- net-2.6.22.orig/net/netlink/genetlink.c	2007-03-20 23:53:19.000000000 +0100
+++ net-2.6.22/net/netlink/genetlink.c	2007-03-21 01:17:13.000000000 +0100
@@ -304,9 +304,6 @@ static int genl_rcv_msg(struct sk_buff *
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err = -EINVAL;
 
-	if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
-		goto ignore;
-
 	if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
 		goto ignore;
 
Index: net-2.6.22/net/core/rtnetlink.c
===================================================================
--- net-2.6.22.orig/net/core/rtnetlink.c	2007-03-21 00:52:43.000000000 +0100
+++ net-2.6.22/net/core/rtnetlink.c	2007-03-21 01:17:13.000000000 +0100
@@ -861,10 +861,6 @@ rtnetlink_rcv_msg(struct sk_buff *skb, s
 	int type;
 	int err;
 
-	/* Only requests are handled by kernel now */
-	if (!(nlh->nlmsg_flags&NLM_F_REQUEST))
-		return 0;
-
 	type = nlh->nlmsg_type;
 
 	/* A control message: ignore them */

--

[PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Thomas Graf]

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

Changes netlink_rcv_skb() to skip netlink controll messages and don't
pass them on to the message handler.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/net/netlink/af_netlink.c
===================================================================
--- net-2.6.22.orig/net/netlink/af_netlink.c	2007-03-21 01:17:13.000000000 +0100
+++ net-2.6.22/net/netlink/af_netlink.c	2007-03-21 01:17:46.000000000 +0100
@@ -1479,6 +1479,10 @@ static int netlink_rcv_skb(struct sk_buf
 		if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
 			goto skip;
 
+		/* Skip control messages */
+		if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
+			goto skip;
+
 		if (cb(skb, nlh, &err) < 0) {
 			/* Not an error, but we have to interrupt processing
 			 * here. Note: that in this case we do not pull
Index: net-2.6.22/net/core/rtnetlink.c
===================================================================
--- net-2.6.22.orig/net/core/rtnetlink.c	2007-03-21 01:17:13.000000000 +0100
+++ net-2.6.22/net/core/rtnetlink.c	2007-03-21 01:17:46.000000000 +0100
@@ -863,10 +863,6 @@ rtnetlink_rcv_msg(struct sk_buff *skb, s
 
 	type = nlh->nlmsg_type;
 
-	/* A control message: ignore them */
-	if (type < RTM_BASE)
-		return 0;
-
 	/* Unknown message: reply with EINVAL */
 	if (type > RTM_MAX)
 		goto err_inval;
Index: net-2.6.22/net/netlink/genetlink.c
===================================================================
--- net-2.6.22.orig/net/netlink/genetlink.c	2007-03-21 01:17:13.000000000 +0100
+++ net-2.6.22/net/netlink/genetlink.c	2007-03-21 01:17:46.000000000 +0100
@@ -304,9 +304,6 @@ static int genl_rcv_msg(struct sk_buff *
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err = -EINVAL;
 
-	if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
-		goto ignore;
-
 	family = genl_family_find_byid(nlh->nlmsg_type);
 	if (family == NULL) {
 		err = -ENOENT;
@@ -364,9 +361,6 @@ static int genl_rcv_msg(struct sk_buff *
 	*errp = err = ops->doit(skb, &info);
 	return err;
 
-ignore:
-	return 0;
-
 errout:
 	*errp = err;
 	return -1;
Index: net-2.6.22/net/xfrm/xfrm_user.c
===================================================================
--- net-2.6.22.orig/net/xfrm/xfrm_user.c	2007-03-21 01:17:13.000000000 +0100
+++ net-2.6.22/net/xfrm/xfrm_user.c	2007-03-21 01:17:46.000000000 +0100
@@ -1861,10 +1861,6 @@ static int xfrm_user_rcv_msg(struct sk_b
 
 	type = nlh->nlmsg_type;
 
-	/* A control message: ignore them */
-	if (type < XFRM_MSG_BASE)
-		return 0;
-
 	/* Unknown message: reply with EINVAL */
 	if (type > XFRM_MSG_MAX)
 		goto err_einval;

--

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Patrick McHardy]

Thomas Graf wrote:
> Changes netlink_rcv_skb() to skip netlink controll messages and don't
> pass them on to the message handler.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Index: net-2.6.22/net/netlink/af_netlink.c
> ===================================================================
> --- net-2.6.22.orig/net/netlink/af_netlink.c	2007-03-21 01:17:13.000000000 +0100
> +++ net-2.6.22/net/netlink/af_netlink.c	2007-03-21 01:17:46.000000000 +0100
> @@ -1479,6 +1479,10 @@ static int netlink_rcv_skb(struct sk_buf
>  		if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
>  			goto skip;
>  
> +		/* Skip control messages */
> +		if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
> +			goto skip;
> +


This looks like it would break nfnetlink, which appears to be
using 0 as smallest message type.

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2007-03-21 05:44
> This looks like it would break nfnetlink, which appears to be
> using 0 as smallest message type.

It shouldn't do that, the first 16 message types are reserved
for control messages.

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Thomas Graf]

* Thomas Graf <tgraf@suug.ch> 2007-03-21 12:45
> * Patrick McHardy <kaber@trash.net> 2007-03-21 05:44
> > This looks like it would break nfnetlink, which appears to be
> > using 0 as smallest message type.
> 
> It shouldn't do that, the first 16 message types are reserved
> for control messages.

Alright, even though nfnetlink is wrong and buggy we can't break
it at this point.

Dave, please ignore this last patch for now.

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-03-21 05:44
> 
>>This looks like it would break nfnetlink, which appears to be
>>using 0 as smallest message type.
> 
> 
> It shouldn't do that, the first 16 message types are reserved
> for control messages.


I'm afraid it does:

enum cntl_msg_types {
        IPCTNL_MSG_CT_NEW,
        IPCTNL_MSG_CT_GET,
        IPCTNL_MSG_CT_DELETE,
        IPCTNL_MSG_CT_GET_CTRZERO,
        IPCTNL_MSG_MAX
};

This is totally broken of course since it also uses netlink_ack(),
netlink_dump() etc. :( Any smart ideas how to fix this without
breaking compatibility?

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Patrick McHardy]

Patrick McHardy wrote:
> Thomas Graf wrote:
> 
>>* Patrick McHardy <kaber@trash.net> 2007-03-21 05:44
>>
>>
>>>This looks like it would break nfnetlink, which appears to be
>>>using 0 as smallest message type.
>>
>>
>>It shouldn't do that, the first 16 message types are reserved
>>for control messages.
> 
> 
> 
> I'm afraid it does:
> 
> enum cntl_msg_types {
>         IPCTNL_MSG_CT_NEW,
>         IPCTNL_MSG_CT_GET,
>         IPCTNL_MSG_CT_DELETE,
>         IPCTNL_MSG_CT_GET_CTRZERO,
>         IPCTNL_MSG_MAX
> };
> 
> This is totally broken of course since it also uses netlink_ack(),
> netlink_dump() etc. :( Any smart ideas how to fix this without
> breaking compatibility?


Seems like we're lucky, nfnetlink encodes the "subsystem ID"
in the upper 8 bits of the message type and uses 1 as the
smallest ID:

/* netfilter netlink message types are split in two pieces:
 * 8 bit subsystem, 8bit operation.
 */

#define NFNL_SUBSYS_ID(x)       ((x & 0xff00) >> 8)
#define NFNL_MSG_TYPE(x)        (x & 0x00ff)

#define NFNL_SUBSYS_NONE                0
#define NFNL_SUBSYS_CTNETLINK           1
#define NFNL_SUBSYS_CTNETLINK_EXP       2
#define NFNL_SUBSYS_QUEUE               3
#define NFNL_SUBSYS_ULOG                4
#define NFNL_SUBSYS_COUNT               5



So this should work fine.

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2007-03-21 13:06
> Thomas Graf wrote:
> > * Patrick McHardy <kaber@trash.net> 2007-03-21 05:44
> > 
> >>This looks like it would break nfnetlink, which appears to be
> >>using 0 as smallest message type.
> > 
> > 
> > It shouldn't do that, the first 16 message types are reserved
> > for control messages.
> 
> 
> I'm afraid it does:
> 
> enum cntl_msg_types {
>         IPCTNL_MSG_CT_NEW,
>         IPCTNL_MSG_CT_GET,
>         IPCTNL_MSG_CT_DELETE,
>         IPCTNL_MSG_CT_GET_CTRZERO,
>         IPCTNL_MSG_MAX
> };
> 
> This is totally broken of course since it also uses netlink_ack(),
> netlink_dump() etc. :( Any smart ideas how to fix this without
> breaking compatibility?

Hmm... I think nfnetlink isn't even broken:

/* netfilter netlink message types are split in two pieces:
 * 8 bit subsystem, 8bit operation.
 */

#define NFNL_SUBSYS_ID(x)	((x & 0xff00) >> 8)
#define NFNL_MSG_TYPE(x)	(x & 0x00ff)

/* No enum here, otherwise __stringify() trick of
 * MODULE_ALIAS_NFNL_SUBSYS()
 * won't work anymore */
#define NFNL_SUBSYS_NONE 		0
#define NFNL_SUBSYS_CTNETLINK		1
#define NFNL_SUBSYS_CTNETLINK_EXP	2
#define NFNL_SUBSYS_QUEUE		3
#define NFNL_SUBSYS_ULOG		4
#define NFNL_SUBSYS_COUNT		5

A msg_type < 0x10 would just trigger a -EINVAL as no 0x0 subsystem
can ever be registered.

Re: [PATCH 5/5] [NETLINK]: Ignore control messages directly in netlink_run_queue()

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2007-03-21 13:21
> Seems like we're lucky, nfnetlink encodes the "subsystem ID"
> in the upper 8 bits of the message type and uses 1 as the
> smallest ID:

Alright, you've been quicker :-)

[NETFILTER] nfnetlink: netlink_run_queue() already checks for NLM_F_REQUEST

[Thomas Graf]

Patrick has made use of netlink_run_queue() in nfnetlink while my patches
have been waiting for net-2.6.22 to open. So this check for NLM_F_REQUEST
can go as well.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/net/netfilter/nfnetlink.c
===================================================================
--- net-2.6.22.orig/net/netfilter/nfnetlink.c	2007-03-21 13:27:48.000000000 +0100
+++ net-2.6.22/net/netfilter/nfnetlink.c	2007-03-21 13:28:11.000000000 +0100
@@ -207,10 +207,6 @@ static int nfnetlink_rcv_msg(struct sk_b
 		return -1;
 	}
 
-	/* Only requests are handled by kernel now. */
-	if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
-		return 0;
-
 	/* All the messages must at least contain nfgenmsg */
 	if (nlh->nlmsg_len < NLMSG_SPACE(sizeof(struct nfgenmsg)))
 		return 0;

Re: [NETFILTER] nfnetlink: netlink_run_queue() already checks for NLM_F_REQUEST

[Patrick McHardy]

Thomas Graf wrote:
> Patrick has made use of netlink_run_queue() in nfnetlink while my patches
> have been waiting for net-2.6.22 to open. So this check for NLM_F_REQUEST
> can go as well.


Looks good, thanks. I've added it to my queue.