Re: [RFC V2 PATCH] rtnetlink: Fix problem with buffer allocation
From: Ben Hutchings <hidden>
Date: 2012-02-14 21:13:16
Thanks for persisting, Greg. On Sun, 2012-02-12 at 11:13 -0800, Greg Rose 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 hunk ↗ jump to hunk
--- a/include/linux/netlink.h +++ b/include/linux/netlink.h@@ -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 */@@ -215,6 +216,7 @@ int netlink_sendskb(struct sock *sk, struct sk_buff *skb); #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 hunk ↗ jump to hunk
--- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c
[...]
quoted hunk ↗ jump to hunk
@@ -1055,6 +1059,17 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) rcu_read_lock(); cb->seq = net->dev_base_seq; + if (cb->nlh->nlmsg_flags && NLM_F_EXT) {
& not &&
+ 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. [...]
quoted hunk ↗ jump to hunk
@@ -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); + nskb = nlmsg_new(if_nlmsg_size(dev, 0), GFP_KERNEL); if (nskb == NULL) return -ENOBUFS; 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).
quoted hunk ↗ jump to hunk
@@ -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; + + 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(). Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.