| Message ID | 1401279319-29088-1-git-send-email-mschmidt@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Michal Schmidt <mschmidt@redhat.com> Date: Wed, 28 May 2014 14:15:19 +0200 > 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 <mschmidt@redhat.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/31/2014 02:42 AM, David Miller wrote: > From: Michal Schmidt <mschmidt@redhat.com> > Date: Wed, 28 May 2014 14:15:19 +0200 > >> 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 <mschmidt@redhat.com> > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/03/2014 02:21 PM, Michal Schmidt wrote: > On 05/31/2014 02:42 AM, David Miller wrote: >> From: Michal Schmidt <mschmidt@redhat.com> >> Date: Wed, 28 May 2014 14:15:19 +0200 >> >>> 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 <mschmidt@redhat.com> >> >> 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). 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? > I could understand your objection if my proposed patch could break > something else, but you are not saying that, are you? Michal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michal Schmidt <mschmidt@redhat.com> Date: Thu, 12 Jun 2014 16:13:09 +0200 > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Thu, Jun 12, 2014 at 7:13 AM, Michal Schmidt <mschmidt@redhat.com> wrote:
> On 06/03/2014 02:21 PM, Michal Schmidt wrote:
>> On 05/31/2014 02:42 AM, David Miller wrote:
>>>
>>> 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.
>>
>> 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?
>
>> I could understand your objection if my proposed patch could break
>> something else, but you are not saying that, are you?
>
> Michal
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Linus Torvalds <torvalds@linux-foundation.org> Date: Thu, 12 Jun 2014 10:44:49 -0700 > 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. Yeah I guess you're right and I'll end up applying Michal's patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 12, 2014 at 10:47 AM, David Miller <davem@davemloft.net> wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michal Schmidt <mschmidt@redhat.com> Date: Tue, 03 Jun 2014 14:21:07 +0200 > 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 :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 2d8d8fc..750119b 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -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]); }
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 <mschmidt@redhat.com> --- net/core/rtnetlink.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)