rtnetlink: Fix problem with buffer allocation

Implement a new nlattr type IFLA_EXT_MASK. The mask is a 32 bit value that can be used to indicate to the kernel that certain extended ifinfo values are requested by the user application. At this time the only mask value created is RTEXT_FILTER_VF to indicate that the user wants the ifinfo dump to send information about the VFs belonging to the interface.

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present. It does not act as the filter itself which has changed since the first submission of this RFC. Older versions of user applications won't set this flag which should fix the problem of some applications not allocating a large enough buffer for all the extended interface information.

Signed-off-by Greg Rose <[email protected]>

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Stephen Hemminger
Sent: Monday, February 13, 2012 9:29 AM
To: Rose, Gregory V
Cc: [email protected]; [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

Quoted 14 lines · On Sun, 12 Feb 2012 11:13:42 -0800 Greg Rose <[email protected]> wrote:

Implement a new nlattr type IFLA_EXT_MASK. The mask is a 32 bit value that can be used to indicate to the kernel that certain extended ifinfo values are requested by the user application. At this time the only mask value created is RTEXT_FILTER_VF to indicate that the user wants the ifinfo dump to send information about the VFs belonging to the interface.

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present. It does not act as the filter itself which has changed since the first submission of this RFC. Older versions of user applications won't set this flag which should fix the problem of some applications not allocating a large enough buffer for all the extended interface information.

Quoted 1 line

Signed-off-by Greg Rose <[email protected]>

Is u32 going to be enough in next 5 years?

Since it is TLV value it should work with bigger value; could you try sending a oversize attribute and code kernel to ignore bits it doesn't understand?

Implement a new nlattr type IFLA_EXT_MASK. The mask is a 32 bit value that can be used to indicate to the kernel that certain extended ifinfo values are requested by the user application. At this time the only mask value created is RTEXT_FILTER_VF to indicate that the user wants the ifinfo dump to send information about the VFs belonging to the interface.

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present. It does not act as the filter itself which has changed since the first submission of this RFC. Older versions of user applications won't set this flag which should fix the problem of some applications not allocating a large enough buffer for all the extended interface information.

include/linux/netlink.h +1 -0

@@ -59,6 +59,7 @@ struct nlmsghdr {
 #define NLM_F_MATCH	0x200	/* return all matching	*/
 #define NLM_F_ATOMIC	0x400	/* atomic GET		*/
 #define NLM_F_DUMP	(NLM_F_ROOT|NLM_F_MATCH)
+#define NLM_F_EXT	0x800	/* Get extended interface info such as VFs */
 
 /* Modifiers to NEW request */
 #define NLM_F_REPLACE	0x100	/* Override existing		*/

net/core/rtnetlink.c +0 -0

net/core/rtnetlink.c +0 -1

@@ -1861,12 +1876,12 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 	if (dev == NULL)
 		return -ENODEV;
 
-	nskb = nlmsg_new(if_nlmsg_size(dev), GFP_KERNEL);

net/core/rtnetlink.c +4 -2

@@ -1877,9 +1892,26 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 	return err;
 }
 
-static u16 rtnl_calcit(struct sk_buff *skb)
+static u16 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
-	return min_ifinfo_dump_size;
+	struct rtnl_req_extended *req;
+	u32 ext_filter_mask = 0;
+

-----Original Message-----
From: Ben Hutchings [mailto:[email protected]]
Sent: Tuesday, February 14, 2012 1:13 PM
To: Rose, Gregory V
Cc: [email protected]; [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

Thanks for persisting, Greg.

Quoted 13 lines

Implement a new nlattr type IFLA_EXT_MASK. The mask is a 32 bit value that can be used to indicate to the kernel that certain extended ifinfo values are requested by the user application. At this time the only mask value created is RTEXT_FILTER_VF to indicate that the user wants the ifinfo dump to send information about the VFs belonging to the interface.

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present. It does not act as the filter itself which has changed since the first submission of this RFC. Older versions of user applications won't set this flag which should fix the problem of some applications not allocating a large enough buffer for all the extended interface information.

[...]

include/linux/netlink.h struct nlmsghdr { · line 62

Quoted 7 lines

include/linux/netlink.h +1 -0

@@ -59,6 +59,7 @@ struct nlmsghdr {
 #define NLM_F_MATCH	0x200	/* return all matching	*/
 #define NLM_F_ATOMIC	0x400	/* atomic GET		*/
 #define NLM_F_DUMP	(NLM_F_ROOT|NLM_F_MATCH)
+#define NLM_F_EXT	0x800	/* Get extended interface info such as VFs

*/

include/linux/netlink.h int netlink_sendskb(struct sock *sk, struct sk_buff

Quoted 4 lines

include/linux/netlink.h +0 -0

 /* Modifiers to NEW request */
 #define NLM_F_REPLACE	0x100	/* Override existing		*/

@@ -215,6 +216,7 @@ int netlink_sendskb(struct sock *sk, struct sk_buff

*skb);

include/linux/netlink.h int netlink_sendskb(struct sock *sk, struct sk_buff *skb); · line 219

Quoted 4 lines

include/linux/netlink.h +1 -0

 #else
 #define NLMSG_GOODSIZE	SKB_WITH_OVERHEAD(8192UL)
 #endif
+#define NLMSG_EXT_GOODSIZE SKB_WITH_OVERHEAD(32768UL)

This will probably do for now, but it would be preferable to really calculate the maximum size.

I fear this is going to be too small in some cases while resulting in allocation failures in others (that's an order-4 page allocation!). Maybe reduce the message size slightly so that the skb data allocation is within 32K?

[...]

Quoted 2 lines

net/core/rtnetlink.c +0 -0

[...]

net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb,

Quoted 1 line

net/core/rtnetlink.c +0 -0

@@ -1055,6 +1059,17 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,

struct netlink_callback *cb)

net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1062

Quoted 4 lines

net/core/rtnetlink.c +1 -0

 	rcu_read_lock();
 	cb->seq = net->dev_base_seq;
 
+	if (cb->nlh->nlmsg_flags && NLM_F_EXT) {

& not &&

net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1071

Quoted 10 lines

net/core/rtnetlink.c +9 -0

+		struct rtattr *ext_req;
+		u32 *ext_req_data;
+		req = (struct rtnl_req_extended *)cb->nlh;
+		ext_req = (struct rtattr *)&req->ext;
+		if (ext_req->rta_type == IFLA_EXT_MASK) {
+			ext_req_data = RTA_DATA(ext_req);
+			ext_filter_mask = *ext_req_data;
+		}
+	}

We cannot trust a flag to tell us what the length of the message is. We have to check the value of nlmsg_len (which netlink has already validated as being within the skb length and >= our declared request header length). I think that makes the flag redundant.

In fact, I think we should really use nlmsg_parse() here. That might be overkill when there's only a single valid attribute; I don't know.

[...]

net/core/rtnetlink.c static int rtnl_getlink(struct sk_buff *skb,

Quoted 1 line

net/core/rtnetlink.c +0 -0

@@ -1861,12 +1876,12 @@ static int rtnl_getlink(struct sk_buff *skb,

struct nlmsghdr* nlh, void *arg)

Quoted 8 lines

net/core/rtnetlink.c +1 -1

 	if (dev == NULL)
 		return -ENODEV;
 
-	nskb = nlmsg_new(if_nlmsg_size(dev), GFP_KERNEL);
+	nskb = nlmsg_new(if_nlmsg_size(dev, 0), GFP_KERNEL);
 	if (nskb == NULL)
 		return -ENOBUFS;
 

net/core/rtnetlink.c static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg) · line 1887

Quoted 6 lines

net/core/rtnetlink.c +1 -1

 	err = rtnl_fill_ifinfo(nskb, dev, RTM_NEWLINK, NETLINK_CB(skb).pid,
-			       nlh->nlmsg_seq, 0, 0);
+			       nlh->nlmsg_seq, 0, 0, 0);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size */
 		WARN_ON(err == -EMSGSIZE);

It seems like this ought to support IFLA_EXT_MASK as well, though it's maybe less important ('ip link' doesn't need it).

net/core/rtnetlink.c static int rtnl_getlink(struct sk_buff *skb,

Quoted 2 lines

net/core/rtnetlink.c +0 -0

@@ -1877,9 +1892,26 @@ static int rtnl_getlink(struct sk_buff *skb,

struct nlmsghdr* nlh, void *arg)

Quoted 20 lines

net/core/rtnetlink.c +14 -2

 	return err;
 }
 
-static u16 rtnl_calcit(struct sk_buff *skb)
+static u16 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
-	return min_ifinfo_dump_size;
+	struct rtnl_req_extended *req;
+	u32 ext_filter_mask = 0;
+
+	if (nlh->nlmsg_flags && NLM_F_EXT) {
+		struct rtattr *ext_req;
+		u32 *ext_req_data;
+		req = (struct rtnl_req_extended *)&nlh;
+		ext_req = (struct rtattr *)&req->ext;
+		if (ext_req->rta_type == IFLA_EXT_MASK) {
+			ext_req_data = RTA_DATA(ext_req);
+			ext_filter_mask = *ext_req_data;
+		}
+	}

[...]

This has the same parsing problem as rtnl_dump_ifinfo().

That's a real sticky problem. When the dump request comes in we don't know how big the required buffer might actually get. That's why we used to have the min_ifinfo_dump_size. However there were problems with that also so Dave asked me to get rid of it. The only thing I could come up with just allocating a really big buffer. If someone could point out to me how to get a maximum ifinfo dump size before rtnl_dump_ifinfo starts filtering through the matching interfaces then that would really help. But at the point the dump request comes in we don't know beforehand how to calculate the info dump size because if_nlmsg_size() requires a pointer to a netdevice.

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 14, 2012 1:32 PM
To: Rose, Gregory V
Cc: [email protected]; [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

From: "Rose, Gregory V" <[email protected]>
Date: Tue, 14 Feb 2012 21:27:59 +0000

Quoted 10 lines

That's a real sticky problem. When the dump request comes in we don't know how big the required buffer might actually get. That's why we used to have the min_ifinfo_dump_size. However there were problems with that also so Dave asked me to get rid of it. The only thing I could come up with just allocating a really big buffer. If someone could point out to me how to get a maximum ifinfo dump size before rtnl_dump_ifinfo starts filtering through the matching interfaces then that would really help. But at the point the dump request comes in we don't know beforehand how to calculate the info dump size because if_nlmsg_size() requires a pointer to a netdevice.

Two quick ideas:

1) Have a first pass which iterates over the devices and calculates the maximum of all if_nlmsg_size() calculations over those devices then uses that for the dump buffer allocation size.

2) Replace min_ifinfo_dump_size() with an array of sizes, one for each combination of extended features. The only concern with this one is that whilst it's practical to do this now with only 1 or a few feature bits, in the future it may be a lot less practical.

The second item sort of matches what I said in the last reply. Base the buffer allocation size on the maximum possible for the given extension which as I said, is up to 255 VFs for the case under immediate consideration.

The first one seems like a good idea but I wonder what the effect would be on a system with a large number of interfaces.

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 14, 2012 1:48 PM
To: Rose, Gregory V
Cc: [email protected]; [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

From: "Rose, Gregory V" <[email protected]>
Date: Tue, 14 Feb 2012 21:41:57 +0000

Quoted 4 lines

The second item sort of matches what I said in the last reply. Base the buffer allocation size on the maximum possible for the given extension which as I said, is up to 255 VFs for the case under immediate consideration.

That's what we're trying to avoid, because that will result in multi-order allocations (which are failure prone) when most of the time such a large buffer is entirely unnecessary.

Quoted 2 lines

The first one seems like a good idea but I wonder what the effect would be on a system with a large number of interfaces.

It's already expensive to dump a large number of devices, and in effect you'll be optimizing the buffer allocation which could in fact end up helping performance.

net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1071

Quoted 9 lines

net/core/rtnetlink.c +9 -0

+		struct rtattr *ext_req;
+		u32 *ext_req_data;
+		req = (struct rtnl_req_extended *)cb->nlh;
+		ext_req = (struct rtattr *)&req->ext;
+		if (ext_req->rta_type == IFLA_EXT_MASK) {
+			ext_req_data = RTA_DATA(ext_req);
+			ext_filter_mask = *ext_req_data;
+		}
+	}

We cannot trust a flag to tell us what the length of the message is. We have to check the value of nlmsg_len (which netlink has already validated as being within the skb length and >= our declared request header length). I think that makes the flag redundant.

In fact, I think we should really use nlmsg_parse() here. That might be overkill when there's only a single valid attribute; I don't know.

-----Original Message-----
From: Thomas Graf [mailto:[email protected]] On Behalf Of Thomas Graf
Sent: Wednesday, February 15, 2012 6:08 AM
To: Ben Hutchings
Cc: Rose, Gregory V; [email protected]; [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

Quoted 15 lines · Ben Hutchings

net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1071

Quoted 9 lines

net/core/rtnetlink.c +9 -0

+		struct rtattr *ext_req;
+		u32 *ext_req_data;
+		req = (struct rtnl_req_extended *)cb->nlh;
+		ext_req = (struct rtattr *)&req->ext;
+		if (ext_req->rta_type == IFLA_EXT_MASK) {
+			ext_req_data = RTA_DATA(ext_req);
+			ext_filter_mask = *ext_req_data;
+		}
+	}

We cannot trust a flag to tell us what the length of the message is. We have to check the value of nlmsg_len (which netlink has already validated as being within the skb length and >= our declared request header length). I think that makes the flag redundant.

Quoted 2 lines

In fact, I think we should really use nlmsg_parse() here. That might be overkill when there's only a single valid attribute; I don't know.

I think it's worth the effort. You get all the validation for free. And please use the netlink interface in <net/netlink.h>, the rtattr based interface has been deprecated a while ago.

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present.

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 14, 2012 1:23 PM
To: Rose, Gregory V
Cc: [email protected]
Subject: Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation

From: Greg Rose <[email protected]>
Date: Sun, 12 Feb 2012 11:13:42 -0800

Quoted 2 lines

I have kept the NLM_F_EXT nlmsg_flags bit to indicate to the kernel that the extended ifinfo dump filter mask is present.

No extra indications other than presence of the attribute itself is necessary. Please remove this flag.

Review int netlink_sendskb(struct sock *sk, struct sk_buff

Quoted 1 line

(quoted diff) +0 -0

@@ -215,6 +216,7 @@ int netlink_sendskb(struct sock *sk, struct sk_buff

*skb);

include/linux/netlink.h int netlink_sendskb(struct sock *sk, struct sk_buff *skb); · line 219

Quoted 4 lines

include/linux/netlink.h +1 -0

 #else
 #define NLMSG_GOODSIZE	SKB_WITH_OVERHEAD(8192UL)
 #endif
+#define NLMSG_EXT_GOODSIZE SKB_WITH_OVERHEAD(32768UL)

I indicated in my suggestions that you'll need to calculate this at run-time based upon the extensions enabled and the size of the resulting message plus attributes.

You absolutely cannot just pick some large number and run with this, it's highly wasteful and will be potentially causing allocation failures.

The goodsize value is specifically choosen such that we don't exceed an order-1 allocation with page sizes of 4096 and larger.

include/linux/rtnetlink.h struct tcamsg { · line 613

Quoted 10 lines

include/linux/rtnetlink.h +9 -0

+struct rtnl_req_extended {
+	struct nlmsghdr nlh;
+	struct rtgenmsg g;
+	char ext[RTA_SPACE(sizeof(__u32))];
+};
+
+/* New extended info filters for IFLA_EXT_MASK */
+#define RTEXT_FILTER_VF		(1 << 0)
+

This seems completely unnecessary.

Just define IFLA_EXT_MASK as a variable length array of u32's, but to be honest, for now, you can just make it a normal u32 attribute.

If we extend it in the future, we can make the kernel handle any length, u32 or otherwise. The size field already present in all netlink attributes will allow us to do this transparently.