rtnetlink: fix userspace API breakage for iproute2 < v3.9.0

PATCH Applied Michal Schmidt 3 participants 9 messages May 28, 2014 – Jun 12, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Michal Schmidt

author

2014-05-28 12:15 UTC #0 lore raw

PATCH 1/1 rtnetlink: fix userspace API breakage for iproute2 < v3.9.0

When running RHEL6 userspace on a current upstream kernel, "ip link" fails to show VF information.

The reason is a kernel<->userspace API change introduced by commit 88c5b5ce5cb57 ("rtnetlink: Call nlmsg_parse() with correct header length"), after which the kernel does not see iproute2's IFLA_EXT_MASK attribute in the netlink request.

iproute2 adjusted for the API change in its commit 63338dca4513 ("libnetlink: Use ifinfomsg instead of rtgenmsg in rtnl_wilddump_req_filter").

The problem has been noticed before:
http://marc.info/?l=linux-netdev&m=136692296022182&w=2
(Subject: Re: getting VF link info seems to be broken in 3.9-rc8)

We can do better than tell those with old userspace to upgrade. We can recognize the old iproute2 in the kernel by checking the netlink message length. Even when including the IFLA_EXT_MASK attribute, its netlink message is shorter than struct ifinfomsg.

With this patch "ip link" shows VF information in both old and new iproute2 versions.

Signed-off-by Michal Schmidt <[email protected]>

---
 net/core/rtnetlink.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

net/core/rtnetlink.c +18 -4

index 2d8d8fc..750119b 100644
@@ -1234,6 +1234,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
 	struct nlattr *tb[IFLA_MAX+1];
 	u32 ext_filter_mask = 0;
 	int err;
+	int hdrlen;
 
 	s_h = cb->args[0];
 	s_idx = cb->args[1];

@@ -1241,8 +1242,17 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
 	rcu_read_lock();
 	cb->seq = net->dev_base_seq;
 
-	if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
-			ifla_policy) >= 0) {
+	/* A hack to preserve kernel<->userspace interface.
+	 * The correct header is ifinfomsg. It is consistent with rtnl_getlink.
+	 * However, before Linux v3.9 the code here assumed rtgenmsg and that's
+	 * what iproute2 < v3.9.0 used.
+	 * We can detect the old iproute2. Even including the IFLA_EXT_MASK
+	 * attribute, its netlink message is shorter than struct ifinfomsg.
+	 */
+	hdrlen = nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg) ?
+		 sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
+
+	if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX, ifla_policy) >= 0) {
 
 		if (tb[IFLA_EXT_MASK])
 			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);

@@ -2095,9 +2105,13 @@ static u16 rtnl_calcit(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct nlattr *tb[IFLA_MAX+1];
 	u32 ext_filter_mask = 0;
 	u16 min_ifinfo_dump_size = 0;
+	int hdrlen;
+
+	/* Same kernel<->userspace interface hack as in rtnl_dump_ifinfo. */
+	hdrlen = nlmsg_len(nlh) < sizeof(struct ifinfomsg) ?
+		 sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);
 
-	if (nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
-			ifla_policy) >= 0) {
+	if (nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy) >= 0) {
 		if (tb[IFLA_EXT_MASK])
 			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
 	}

Signature · 1 line

1.9.3

David Miller

maintainer

2014-05-31 00:42 UTC #1 lore raw

From: Michal Schmidt <[email protected]>
Date: Wed, 28 May 2014 14:15:19 +0200

Quoted 20 lines

When running RHEL6 userspace on a current upstream kernel, "ip link" fails to show VF information.
The reason is a kernel<->userspace API change introduced by commit 88c5b5ce5cb57 ("rtnetlink: Call nlmsg_parse() with correct header length"), after which the kernel does not see iproute2's IFLA_EXT_MASK attribute in the netlink request.
iproute2 adjusted for the API change in its commit 63338dca4513 ("libnetlink: Use ifinfomsg instead of rtgenmsg in rtnl_wilddump_req_filter").
The problem has been noticed before:
http://marc.info/?l=linux-netdev&m=136692296022182&w=2
(Subject: Re: getting VF link info seems to be broken in 3.9-rc8)
We can do better than tell those with old userspace to upgrade. We can recognize the old iproute2 in the kernel by checking the netlink message length. Even when including the IFLA_EXT_MASK attribute, its netlink message is shorter than struct ifinfomsg.

Quoted 4 lines

With this patch "ip link" shows VF information in both old and new iproute2 versions.
Signed-off-by Michal Schmidt <[email protected]>

The userspace tool should be fixed on the system, rather than having the kernel cater to a user tool bug.

I'm not applying this, sorry.

Michal Schmidt

author

2014-06-03 12:21 UTC #2 lore raw

Quoted 26 lines · David Miller

From: Michal Schmidt <[email protected]>
Date: Wed, 28 May 2014 14:15:19 +0200
Quoted 23 lines
When running RHEL6 userspace on a current upstream kernel, "ip link" fails to show VF information.
The reason is a kernel<->userspace API change introduced by commit 88c5b5ce5cb57 ("rtnetlink: Call nlmsg_parse() with correct header length"), after which the kernel does not see iproute2's IFLA_EXT_MASK attribute in the netlink request.
iproute2 adjusted for the API change in its commit 63338dca4513 ("libnetlink: Use ifinfomsg instead of rtgenmsg in rtnl_wilddump_req_filter").
The problem has been noticed before:
http://marc.info/?l=linux-netdev&m=136692296022182&w=2
(Subject: Re: getting VF link info seems to be broken in 3.9-rc8)
We can do better than tell those with old userspace to upgrade. We can recognize the old iproute2 in the kernel by checking the netlink message length. Even when including the IFLA_EXT_MASK attribute, its netlink message is shorter than struct ifinfomsg.
With this patch "ip link" shows VF information in both old and new iproute2 versions.

Quoted 6 lines

Quoted 1 line
Signed-off-by Michal Schmidt <[email protected]>
The userspace tool should be fixed on the system, rather than having the kernel cater to a user tool bug.
I'm not applying this, sorry.

David,

the old version of the userspace tool did what it had to do in order to work with a contemporary kernel. You can call that a bug, but the fact is that it used to work that way.

I do not see a difference between "catering to a user tool bug" and "not breaking applications, whether we like them or not" (paraphrasing Linus's message on this very mailing list just 10 days ago).

I could understand your objection if my proposed patch could break something else, but you are not saying that, are you?

Regards,
Michal

Michal Schmidt

author

2014-06-12 14:13 UTC #3 lore raw

Quoted 36 lines · Michal Schmidt

Quoted 26 lines · David Miller
From: Michal Schmidt <[email protected]>
Date: Wed, 28 May 2014 14:15:19 +0200
Quoted 23 lines
When running RHEL6 userspace on a current upstream kernel, "ip link" fails to show VF information.
The reason is a kernel<->userspace API change introduced by commit 88c5b5ce5cb57 ("rtnetlink: Call nlmsg_parse() with correct header length"), after which the kernel does not see iproute2's IFLA_EXT_MASK attribute in the netlink request.
iproute2 adjusted for the API change in its commit 63338dca4513 ("libnetlink: Use ifinfomsg instead of rtgenmsg in rtnl_wilddump_req_filter").
The problem has been noticed before:
http://marc.info/?l=linux-netdev&m=136692296022182&w=2
(Subject: Re: getting VF link info seems to be broken in 3.9-rc8)
We can do better than tell those with old userspace to upgrade. We can recognize the old iproute2 in the kernel by checking the netlink message length. Even when including the IFLA_EXT_MASK attribute, its netlink message is shorter than struct ifinfomsg.
With this patch "ip link" shows VF information in both old and new iproute2 versions.
Quoted 6 lines
Quoted 1 line
Signed-off-by Michal Schmidt <[email protected]>
The userspace tool should be fixed on the system, rather than having the kernel cater to a user tool bug.
I'm not applying this, sorry.
David,

Quoted 7 lines

the old version of the userspace tool did what it had to do in order to work with a contemporary kernel. You can call that a bug, but the fact is that it used to work that way.
I do not see a difference between "catering to a user tool bug" and "not breaking applications, whether we like them or not" (paraphrasing Linus's message on this very mailing list just 10 days ago).

Dave,
I am sorry for harping on this, but could you please either reconsider the NAK, or explain what considerations are more important than userspace compatibility in this case?

Quoted 2 lines

I could understand your objection if my proposed patch could break something else, but you are not saying that, are you?

Michal

David Miller

maintainer

2014-06-12 17:43 UTC #4 lore raw

From: Michal Schmidt <[email protected]>
Date: Thu, 12 Jun 2014 16:13:09 +0200

Quoted 3 lines

I am sorry for harping on this, but could you please either reconsider the NAK, or explain what considerations are more important than userspace compatibility in this case?

I'm still thinking about this, please be patient.

Linus Torvalds

2014-06-12 17:44 UTC #5 lore raw

In reply to Michal Schmidt

David, I have to agree with Michal. If the user-space tool did what it had to do for old kernels, then new kernels should support the old semantics.

You can't call it a "bug" - user space does what it does and it was tested and worked.

And sure, it might have been a bug/misfeature of the old kernel, but we have to live with what we have done, not blame others. Once user space starts depending on our bugs, it's *our* problem.

Saying "fix your sh*t" is wrong, when we caused it to begin with.

Linus

Quoted 26 lines · Michal Schmidt

Quoted 7 lines · Michal Schmidt
Quoted 5 lines · David Miller
The userspace tool should be fixed on the system, rather than having the kernel cater to a user tool bug.
I'm not applying this, sorry.
Quoted 7 lines
the old version of the userspace tool did what it had to do in order to work with a contemporary kernel. You can call that a bug, but the fact is that it used to work that way.
I do not see a difference between "catering to a user tool bug" and "not breaking applications, whether we like them or not" (paraphrasing Linus's message on this very mailing list just 10 days ago).
Dave,
I am sorry for harping on this, but could you please either reconsider the NAK, or explain what considerations are more important than userspace compatibility in this case?
Quoted 2 lines
I could understand your objection if my proposed patch could break something else, but you are not saying that, are you?
Michal

David Miller

maintainer

2014-06-12 17:47 UTC #6 lore raw

From: Linus Torvalds <[email protected]>
Date: Thu, 12 Jun 2014 10:44:49 -0700

Quoted 4 lines

David, I have to agree with Michal. If the user-space tool did what it had to do for old kernels, then new kernels should support the old semantics.

Quoted 8 lines

You can't call it a "bug" - user space does what it does and it was tested and worked.
And sure, it might have been a bug/misfeature of the old kernel, but we have to live with what we have done, not blame others. Once user space starts depending on our bugs, it's *our* problem.
Saying "fix your sh*t" is wrong, when we caused it to begin with.

Yeah I guess you're right and I'll end up applying Michal's patch.

Linus Torvalds

2014-06-12 18:01 UTC #7 lore raw

Quoted 2 lines · David Miller

Yeah I guess you're right and I'll end up applying Michal's patch.

Note, as usual, I love it when we can just silently fix our broken kernel semantics, and change the ABI to be saner. I'm not saying we shouldn't do that. Some people think that my "don't break user space" means some kind of absolute "ABI can never be changed". That's not the case.

It's just that when we get caught doing so, and people complain, we have to just own up to our interfaces having been less than optimal (aka "crap") and try to make everybody happy.

In this case, hopefully in a few years, nobody runs RHEL6 any more and we can remove ugly compatibility crap. But as it is, RHEL 6 isn't quite old enough to ignore. If it was something like RHEL4 or older, I think we could say "yeah, we can't really find it in ourselves to care".

Linus

David Miller

maintainer Applied

2014-06-12 18:22 UTC #8 lore raw

In reply to Michal Schmidt

From: Michal Schmidt <[email protected]>
Date: Tue, 03 Jun 2014 14:21:07 +0200

Quoted 7 lines

the old version of the userspace tool did what it had to do in order to work with a contemporary kernel. You can call that a bug, but the fact is that it used to work that way.
I do not see a difference between "catering to a user tool bug" and "not breaking applications, whether we like them or not" (paraphrasing Linus's message on this very mailing list just 10 days ago).

I applied your patch and queued it up for -stable, thanks for persevering :)