rtnetlink: Fix problem with buffer allocation

PATCH In review Greg Rose 3 participants 11 messages Feb 21, 2012

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Greg Rose

author

2012-02-21 20:47 UTC #0 lore raw

PATCH 1/1 rtnetlink: Fix problem with buffer allocation

Implement a new netlink attribute 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 defined is RTEXT_FILTER_VF to indicate that the user wants the ifinfo dump to send information about the VFs belonging to the interface.

This patch fixes a bug in which certain applications do not have large enough buffers to accommodate the extra information returned by the kernel with large numbers of SR-IOV virtual functions. Those applications will not send the new netlink attribute with the interface info dump request netlink messages so they will not get unexpectedly large request buffers returned by the kernel.

Modifies the rtnl_calcit function to traverse the list of net devices and compute the minimum buffer size that can hold the info dumps of all matching devices based upon the filter passed in via the new netlink attribute filter mask. If no filter mask is sent then the buffer allocation defaults to NLMSG_GOODSIZE.

With this change it is possible to add yet to be defined netlink attributes to the dump request which should make it fairly extensible in the future.

CC [email protected]

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

---

 include/linux/if_link.h   |    1 +
 include/linux/rtnetlink.h |    3 ++
 include/net/rtnetlink.h   |    2 +
 net/core/rtnetlink.c      |   66 ++++++++++++++++++++++++++++++++++-----------
 4 files changed, 55 insertions(+), 17 deletions(-)

include/linux/if_link.h +1 -0

index 052c240..35357f6 100644
@@ -137,6 +137,7 @@ enum {
 	IFLA_AF_SPEC,
 	IFLA_GROUP,		/* Group the device belongs to */
 	IFLA_NET_NS_FD,
+	IFLA_EXT_MASK,		/* Extended info mask, VFs, etc */
 	__IFLA_MAX
 };

include/linux/rtnetlink.h +3 -0

index 8e872ea..577592e 100644
@@ -602,6 +602,9 @@ struct tcamsg {
 #define TCA_ACT_TAB 1 /* attr type must be >=1 */	
 #define TCAA_MAX 1
 
+/* New extended info filters for IFLA_EXT_MASK */
+#define RTEXT_FILTER_VF		(1 << 0)
+
 /* End of information exported to user level */
 
 #ifdef __KERNEL__

include/net/rtnetlink.h +1 -1

index 678f1ff..3702939 100644
@@ -6,7 +6,7 @@
 
 typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, void *);
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
-typedef u16 (*rtnl_calcit_func)(struct sk_buff *);
+typedef u16 (*rtnl_calcit_func)(struct sk_buff *, struct nlmsghdr *);
 
 extern int	__rtnl_register(int protocol, int msgtype,
 				rtnl_doit_func, rtnl_dumpit_func,

net/core/rtnetlink.c +50 ~~-16~~

164 lines not on this page · raw message

David Miller

maintainer

2012-02-21 21:16 UTC #1 lore raw

Greg, where is the code that guards the actual dumping of the VF information based upon the ext mask?

I only see the ext mask controlling the sizing of the SKB. What good is that if we still dump the VFs?

David Miller

maintainer

2012-02-21 21:21 UTC #2 lore raw

From: David Miller <[email protected]>
Date: Tue, 21 Feb 2012 16:16:30 -0500 (EST)

Quoted 6 lines

Greg, where is the code that guards the actual dumping of the VF information based upon the ext mask?
I only see the ext mask controlling the sizing of the SKB. What good is that if we still dump the VFs?

Actually, it's even worse, this thing doesn't even compile because you haven't updated the actual implementation of rtnl_vfinfo_size() to take the new mask argument.

You didn't even compile test this, are you fucking kidding me?

net/core/rtnetlink.c: In function ‘if_nlmsg_size’:
net/core/rtnetlink.c:789:9: error: too many arguments to function ‘rtnl_vfinfo_size’ net/core/rtnetlink.c:726:19: note: declared here
make[1]: *** [net/core/rtnetlink.o] Error 1
make: *** [net/core/rtnetlink.o] Error 2

Rose, Gregory V

author

2012-02-21 21:28 UTC #3 lore raw

Quoted 22 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of David Miller
Sent: Tuesday, February 21, 2012 1:21 PM
To: Rose, Gregory V
Cc: [email protected]
Subject: Re: [PATCH] rtnetlink: Fix problem with buffer allocation
From: David Miller <[email protected]>
Date: Tue, 21 Feb 2012 16:16:30 -0500 (EST)
Quoted 6 lines
Greg, where is the code that guards the actual dumping of the VF information based upon the ext mask?
I only see the ext mask controlling the sizing of the SKB. What good is that if we still dump the VFs?
Actually, it's even worse, this thing doesn't even compile because you haven't updated the actual implementation of rtnl_vfinfo_size() to take the new mask argument.

Quoted 8 lines

You didn't even compile test this, are you fucking kidding me?
net/core/rtnetlink.c: In function ʽif_nlmsg_sizeʼ:
net/core/rtnetlink.c:789:9: error: too many arguments to function ʽrtnl_vfinfo_sizeʼ
net/core/rtnetlink.c:726:19: note: declared here make[1]: *** [net/core/rtnetlink.o] Error 1
make: *** [net/core/rtnetlink.o] Error 2

I compile tested it on my development machine and then exported it to a machine that can connect to the internet and mail patches. I must have screwed something up at that point.

Let me see what the heck I did.

- Greg

Quoted 2 lines

θΊ{.nΗ+·+%Λlzwmιbλ§²ζμrΈzwuλβΨ^nrʽφ¦zΛλh¨θΪ&ʼψG«ιh( ιέʼj"ϊΆm§οκδzΉήΰώf£ʼ·h§~m

David Miller

maintainer

2012-02-21 21:32 UTC #4 lore raw

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

Quoted 5 lines

I compile tested it on my development machine and then exported it to a machine that can connect to the internet and mail patches. I must have screwed something up at that point.
Let me see what the heck I did.

It looks like a quarter of the patch got lost, because if you look at the RFC,V3 version of your patch at:

http://patchwork.ozlabs.org/patch/141714/

all the necessary bits appear to be there.

Rose, Gregory V

author

2012-02-21 21:33 UTC #5 lore raw

Quoted 14 lines

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 21, 2012 1:32 PM
To: Rose, Gregory V
Cc: [email protected]
Subject: Re: [PATCH] rtnetlink: Fix problem with buffer allocation
From: "Rose, Gregory V" <[email protected]>
Date: Tue, 21 Feb 2012 21:28:17 +0000
Quoted 4 lines
I compile tested it on my development machine and then exported it to a machine that can connect to the internet and mail patches. I must have screwed something up at that point.

Quoted 8 lines

Quoted 1 line
Let me see what the heck I did.
It looks like a quarter of the patch got lost, because if you look at the RFC,V3 version of your patch at:
http://patchwork.ozlabs.org/patch/141714/
all the necessary bits appear to be there.

Yeah, I screwed the pooch somewhere between making the last update you requested and then pushing the patch. I have no idea what I did but I'll fix it up and repost.

- Greg

Rose, Gregory V

author

2012-02-21 21:41 UTC #6 lore raw

Quoted 27 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Rose, Gregory V
Sent: Tuesday, February 21, 2012 1:34 PM
To: David Miller
Cc: [email protected]
Subject: RE: [PATCH] rtnetlink: Fix problem with buffer allocation
Quoted 19 lines
-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 21, 2012 1:32 PM
To: Rose, Gregory V
Cc: [email protected]
Subject: Re: [PATCH] rtnetlink: Fix problem with buffer allocation
From: "Rose, Gregory V" <[email protected]>
Date: Tue, 21 Feb 2012 21:28:17 +0000
Quoted 5 lines
I compile tested it on my development machine and then exported it to a machine that can connect to the internet and mail patches. I must have screwed something up at that point.
Let me see what the heck I did.
It looks like a quarter of the patch got lost, because if you look at the RFC,V3 version of your patch at:

Quoted 7 lines

Quoted 3 lines
http://patchwork.ozlabs.org/patch/141714/
all the necessary bits appear to be there.
Yeah, I screwed the pooch somewhere between making the last update you requested and then pushing the patch. I have no idea what I did but I'll fix it up and repost.

I sent from the wrong branch. I'd been in the process of moving the patch over to another development branch on my git tree and it wasn't up to date.

The correct patch from the right dev branch will be up in a couple of minutes.

- Greg

Eric Dumazet

2012-02-21 21:48 UTC #7 lore raw

In reply to Greg Rose

Quoted 6 lines · Greg Rose

Modifies the rtnl_calcit function to traverse the list of net devices and compute the minimum buffer size that can hold the info dumps of all matching devices based upon the filter passed in via the new netlink attribute filter mask. If no filter mask is sent then the buffer allocation defaults to NLMSG_GOODSIZE.

...

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

Quoted 9 lines

net/core/rtnetlink.c +9 -0

+	/*
+	 * traverse the list of net devices and compute the minimum
+	 * buffer size based upon the filter mask.
+	 */
+	list_for_each_entry_rcu(dev, &net->dev_base_head, dev_list) {
+		min_ifinfo_dump_size = max_t(u16, min_ifinfo_dump_size,
+					     if_nlmsg_size(dev,
+						           ext_filter_mask));
+	}

Thats going to hurt some setups (thousand of devices on a server).

Also why are you using _rcu() variant here, we hold rtnl I presume ?

David Miller

maintainer

2012-02-21 21:50 UTC #8 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 21 Feb 2012 22:48:16 +0100

Quoted 1 line

Thats going to hurt some setups (thousand of devices on a server).

This logic only triggers when the VF attribute is explicitly asked for.

Quoted 1 line

Also why are you using _rcu() variant here, we hold rtnl I presume ?

Because that's what I told him to use probably :-) Indeed, I think we can use the non-RCU variant in this spot.

Rose, Gregory V

author

2012-02-21 21:56 UTC #9 lore raw

Quoted 12 lines

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Tuesday, February 21, 2012 1:50 PM
To: [email protected]
Cc: Rose, Gregory V; [email protected]
Subject: Re: [PATCH] rtnetlink: Fix problem with buffer allocation
From: Eric Dumazet <[email protected]>
Date: Tue, 21 Feb 2012 22:48:16 +0100
Quoted 1 line
Thats going to hurt some setups (thousand of devices on a server).

Quoted 7 lines

This logic only triggers when the VF attribute is explicitly asked for.
Quoted 1 line
Also why are you using _rcu() variant here, we hold rtnl I presume ?
Because that's what I told him to use probably :-) Indeed, I think we can use the non-RCU variant in this spot.

In rtnl_fill_ifinfo the code there uses hlist_for_each_entry_rcu() so I think that's why it got carried over here. I can remove it.

- Greg

David Miller

maintainer

2012-02-21 22:02 UTC #10 lore raw

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

Quoted 2 lines

In rtnl_fill_ifinfo the code there uses hlist_for_each_entry_rcu() so I think that's why it got carried over here. I can remove it.

It does so because rtnl_fill_ifinfo() runs within an rcu_read_lock() sequence.

I fixed up the calcit case when I applied your patch, so we're good now.