RE: [PATCH 2/3] netlink: nla_parse_nested_compat was not parsing nested attributes
From: Duyck, Alexander H <hidden>
Date: 2008-08-22 17:40:28
I figured I would CC Stephen Hemminger and Patrick McHardy since they also will likely have an interest in this. Just to put together a quick history on this issue I will list off how things got to where they are: 1. "[NET_SCHED]: sch_netem: use nla_parse_nested_compat" (http://marc.info/?l=linux-netdev&m=120110639320780&w=2) from Patrick McHardy. After this patch netem started reporting "bytes leftover after parsing attributes" errors as it wasn't actually receiving a nested compat attribute. This patch was added around 2.6.25-rc1. 2. "[NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly" (http://marc.info/?l=linux-netdev&m=121145625328370&w=2) from Thomas Graf. This resolved the errors but in turn any netlink messages that generated correct nested netlink attributes would have those attributes ignored as they would be automatically parsed out as one attribute with a length of all the nested attributes. This patch was added around 2.6.26-rc4. Patches 3 & 4 haven't been applied yet and are under review: 3. "[PATCH 2/3] netlink: nla_parse_nested_compat was not parsing nested attributes" (http://marc.info/?l=linux-netdev&m=121936623112195&w=2) from me. This reverts patch 2. 4. "[PATCH] IPROUTE: correct nla nested message generated by netem_parse_opt" (http://marc.info/?l=linux-netdev&m=121936623112195&w=2) from me. This patch changes iproute2 netem_parse_opt to generate a correctly formatted set of nested compat attributes that can be parsed after the introduction of patch 1. The way I see it we have 3 possible solutions to all of this. First there is my proposed solution in patches 3 & 4. The second option would be to revert patch 1 and apply patch 3 which would maintain the kernel ABI and not require any patching to iproute2. The third option I see would be to go back and patch include/net/netlink.h in the kernel and lib/libnetlink.c in iproute2 so that they all support the new format which is parsed by nla_parse_nested_compat. Thanks, Alex -----Original Message----- From: David Miller [mailto:davem@davemloft.net] Sent: Friday, August 22, 2008 3:18 AM To: Kirsher, Jeffrey T Cc: jeff@garzik.org; netdev@vger.kernel.org; Duyck, Alexander H; tgraf@suug.ch Subject: Re: [PATCH 2/3] netlink: nal_parse_nested_compat was not parsing nested attributes From: Jeff Kirsher <redacted> Date: Thu, 21 Aug 2008 17:51:26 -0700 Please at least CC: the author of the patch you are reverting so that they can provide feedback and have a look at this. Thomas Graf CC:'d.
quoted hunk ↗ jump to hunk
From: Alexander Duyck <redacted> This patch reverts previous commit: b9a2f2e450b0f770bb4347ae8d48eb2dea701e24 netlink: Fix nla_parse_nested_compat() to call nla_parse() directly The purpose of nla_parse_nested_compat is to parse attributes which contain a struct followed by a container attribute with a stream of nested attributes. This patch reverts the previous patch which assumed that there was no container attribute due to a malformed string of attributes being generated by netem_parse_opts Signed-off-by: Alexander Duyck <redacted> Signed-off-by: Jeff Kirsher <redacted> --- include/net/netlink.h | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)diff --git a/include/net/netlink.h b/include/net/netlink.h index 18024b8..b295cd1 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h@@ -770,13 +770,12 @@ static inline int __nla_parse_nested_compat(struct nlattr *tb[], int maxtype, const struct nla_policy *policy, int len) { - int nested_len = nla_len(nla) - NLA_ALIGN(len); - - if (nested_len < 0) + if (nla_len(nla) < len) return -EINVAL; - if (nested_len >= nla_attr_size(0)) - return nla_parse(tb, maxtype, nla_data(nla) + NLA_ALIGN(len), - nested_len, policy); + if (nla_len(nla) >= NLA_ALIGN(len) + sizeof(struct nlattr)) + return nla_parse_nested(tb, maxtype, + nla_data(nla) + NLA_ALIGN(len), + policy); memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1)); return 0; }