netlink: 16 bytes leftover after parsing attributes in process `ip'.

netlink: 16 bytes leftover after parsing attributes in process `ip'.

[David Ahern]

On top of net-next I am see a dmesg error:

netlink: 16 bytes leftover after parsing attributes in process `ip'.

I traced it to address lists and commit:

commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
Author: Christian Brauner <christian@brauner.io>
Date:   Tue Sep 4 21:53:50 2018 +0200

    ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR

Per the commit you are trying to guess whether the ancillary header is
an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong. :-)

I don't have time to take this to ground, but address listing is not the
only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
have thought about this for route dumps, but its solution does not work
here. You'll need to find something because the current warning on every
address dump is not acceptable.

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]

On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:
> On top of net-next I am see a dmesg error:
> 
> netlink: 16 bytes leftover after parsing attributes in process `ip'.
> 
> I traced it to address lists and commit:
> 
> commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> Author: Christian Brauner <christian@brauner.io>
> Date:   Tue Sep 4 21:53:50 2018 +0200
> 
>     ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> 
> Per the commit you are trying to guess whether the ancillary header is
> an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong. :-)

Well, I currently don't guess at all. :) I'm parsing with struct
ifaddrmsg as assumed header size but ignore parsing errors when that
fails. You don't get the niceties of the new property if you don't pack
it up nicely in an ifaddrmsg struct. :) 

> 
> I don't have time to take this to ground, but address listing is not the
> only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> have thought about this for route dumps, but its solution does not work
> here. You'll need to find something because the current warning on every
> address dump is not acceptable.

Two points before I propose a migitation:

1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
   userspace is doing something wrong is imho justifiable.
   Actually, I would argue that we should not hide the problem from
   userspace at all. The rate-limited (so no logging DOS afaict) warning
   messages are a perfect indicator that a tool is doing something wrong
   *without* introducing any regressions.
   The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
   be used too. Additionally, userspace stuffs an ifinfomsg in there but
   expects to receive ifaddrmsg. They should be warned loudly. :) So I
   actually like the warning messages.
2. Userspace should be fixed. Especially such an important standard tool
   as iproute2 that is maintained on git.kernel.org (glibc is already
   doing the right.).

So if people really want to hide this issue as much as we can then we
can play the guessing game. I could send a patch that roughly does the
following:

if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
        guessed_header_len = sizeof(struct ifaddrmsg);
else
        guessed_header_len = sizeof(struct ifinfomsg);

This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
This propert is a __s32 which should bring the message up to 12 bytes
(not sure about alignment requiremnts and where we might wend up ten)
which is still less than the 16 bytes without that property from
ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
break when ifaddrmsg grows a new member or we introduce another property
that is valid in RTM_GETADDR requests. It also will not work cleanly
when users stuff additional properties in there that are valid for the
address family but are not used int RTM_GETADDR requests.

I would like to hear what other people and davem think we should do.
Patch it away or print the warning.

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Stephen Hemminger]

On Tue, 25 Sep 2018 11:49:10 +0200
Christian Brauner <christian@brauner.io> wrote:

> On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:
> > On top of net-next I am see a dmesg error:
> > 
> > netlink: 16 bytes leftover after parsing attributes in process `ip'.
> > 
> > I traced it to address lists and commit:
> > 
> > commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> > Author: Christian Brauner <christian@brauner.io>
> > Date:   Tue Sep 4 21:53:50 2018 +0200
> > 
> >     ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> > 
> > Per the commit you are trying to guess whether the ancillary header is
> > an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong. :-)  
> 
> Well, I currently don't guess at all. :) I'm parsing with struct
> ifaddrmsg as assumed header size but ignore parsing errors when that
> fails. You don't get the niceties of the new property if you don't pack

There are legacy parts of netlink interface with kernel.
The ABI has evolved over time but some old parts are stuck in the past.


> > I don't have time to take this to ground, but address listing is not the
> > only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> > have thought about this for route dumps, but its solution does not work
> > here. You'll need to find something because the current warning on every
> > address dump is not acceptable.  
> 
> Two points before I propose a migitation:
> 
> 1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
>    userspace is doing something wrong is imho justifiable.
>    Actually, I would argue that we should not hide the problem from
>    userspace at all. The rate-limited (so no logging DOS afaict) warning
>    messages are a perfect indicator that a tool is doing something wrong
>    *without* introducing any regressions.
>    The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
>    be used too. Additionally, userspace stuffs an ifinfomsg in there but
>    expects to receive ifaddrmsg. They should be warned loudly. :) So I
>    actually like the warning messages.
> 2. Userspace should be fixed. Especially such an important standard tool
>    as iproute2 that is maintained on git.kernel.org (glibc is already
>    doing the right.).
> 
> So if people really want to hide this issue as much as we can then we
> can play the guessing game. I could send a patch that roughly does the
> following:
> 
> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
>         guessed_header_len = sizeof(struct ifaddrmsg);
> else
>         guessed_header_len = sizeof(struct ifinfomsg);
> 
> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> This propert is a __s32 which should bring the message up to 12 bytes
> (not sure about alignment requiremnts and where we might wend up ten)
> which is still less than the 16 bytes without that property from
> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> break when ifaddrmsg grows a new member or we introduce another property
> that is valid in RTM_GETADDR requests. It also will not work cleanly
> when users stuff additional properties in there that are valid for the
> address family but are not used int RTM_GETADDR requests.
> 
> I would like to hear what other people and davem think we should do.
> Patch it away or print the warning.
> 
> Christian

You can't break old programs. That is one of the rules of kernel.
Therefore, please either revert the kernel change or put the new attribute
in a place where old versions do not cause problem.

There are people who run new kernels on old versions of iproute (like enterprise
distributions) and vice versa.

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Stephen Hemminger]

On Tue, 25 Sep 2018 14:34:08 +0200
Christian Brauner <christian@brauner.io> wrote:

> On Tue, Sep 25, 2018, 14:07 Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
> > On Tue, 25 Sep 2018 11:49:10 +0200
> > Christian Brauner <christian@brauner.io> wrote:
> >  
> > > On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:  
> > > > On top of net-next I am see a dmesg error:
> > > >
> > > > netlink: 16 bytes leftover after parsing attributes in process `ip'.
> > > >
> > > > I traced it to address lists and commit:
> > > >
> > > > commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> > > > Author: Christian Brauner <christian@brauner.io>
> > > > Date:   Tue Sep 4 21:53:50 2018 +0200
> > > >
> > > >     ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> > > >
> > > > Per the commit you are trying to guess whether the ancillary header is
> > > > an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong.  
> > :-)  
> > >
> > > Well, I currently don't guess at all. :) I'm parsing with struct
> > > ifaddrmsg as assumed header size but ignore parsing errors when that
> > > fails. You don't get the niceties of the new property if you don't pack  
> >
> > There are legacy parts of netlink interface with kernel.
> > The ABI has evolved over time but some old parts are stuck in the past.
> >
> >  
> > > > I don't have time to take this to ground, but address listing is not  
> > the  
> > > > only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> > > > have thought about this for route dumps, but its solution does not work
> > > > here. You'll need to find something because the current warning on  
> > every  
> > > > address dump is not acceptable.  
> > >
> > > Two points before I propose a migitation:
> > >
> > > 1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
> > >    userspace is doing something wrong is imho justifiable.
> > >    Actually, I would argue that we should not hide the problem from
> > >    userspace at all. The rate-limited (so no logging DOS afaict) warning
> > >    messages are a perfect indicator that a tool is doing something wrong
> > >    *without* introducing any regressions.
> > >    The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
> > >    be used too. Additionally, userspace stuffs an ifinfomsg in there but
> > >    expects to receive ifaddrmsg. They should be warned loudly. :) So I
> > >    actually like the warning messages.
> > > 2. Userspace should be fixed. Especially such an important standard tool
> > >    as iproute2 that is maintained on git.kernel.org (glibc is already
> > >    doing the right.).
> > >
> > > So if people really want to hide this issue as much as we can then we
> > > can play the guessing game. I could send a patch that roughly does the
> > > following:
> > >
> > > if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> > >         guessed_header_len = sizeof(struct ifaddrmsg);
> > > else
> > >         guessed_header_len = sizeof(struct ifinfomsg);
> > >
> > > This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> > > The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> > > This propert is a __s32 which should bring the message up to 12 bytes
> > > (not sure about alignment requiremnts and where we might wend up ten)
> > > which is still less than the 16 bytes without that property from
> > > ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> > > break when ifaddrmsg grows a new member or we introduce another property
> > > that is valid in RTM_GETADDR requests. It also will not work cleanly
> > > when users stuff additional properties in there that are valid for the
> > > address family but are not used int RTM_GETADDR requests.
> > >
> > > I would like to hear what other people and davem think we should do.
> > > Patch it away or print the warning.
> > >
> > > Christian  
> >
> > You can't break old programs. That is one of the rules of kernel.
> > Therefore, please either revert the kernel change or put the new attribute
> > in a place where old versions do not cause problem.
> >  
> 
> Sorry, there's a misunderstanding here. The code doesn't regress anything.
> The patch  was  written  in a backward compatible way. The only thing it
> causes are rate-limited logging messages when the wrong struct is passed.
> But the request still succeeds. The issue is with the logging afaict.
> 
> Christian


That still means enterprise distributions that use the current kernel will
get customer complaints. You need to remove the warning.

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Christian Brauner]

On Tue, Sep 25, 2018 at 02:16:12PM +0100, Stephen Hemminger wrote:
> On Tue, 25 Sep 2018 14:34:08 +0200
> Christian Brauner <christian@brauner.io> wrote:
> 
> > On Tue, Sep 25, 2018, 14:07 Stephen Hemminger <stephen@networkplumber.org>
> > wrote:
> > 
> > > On Tue, 25 Sep 2018 11:49:10 +0200
> > > Christian Brauner <christian@brauner.io> wrote:
> > >  
> > > > On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:  
> > > > > On top of net-next I am see a dmesg error:
> > > > >
> > > > > netlink: 16 bytes leftover after parsing attributes in process `ip'.
> > > > >
> > > > > I traced it to address lists and commit:
> > > > >
> > > > > commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> > > > > Author: Christian Brauner <christian@brauner.io>
> > > > > Date:   Tue Sep 4 21:53:50 2018 +0200
> > > > >
> > > > >     ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> > > > >
> > > > > Per the commit you are trying to guess whether the ancillary header is
> > > > > an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong.  
> > > :-)  
> > > >
> > > > Well, I currently don't guess at all. :) I'm parsing with struct
> > > > ifaddrmsg as assumed header size but ignore parsing errors when that
> > > > fails. You don't get the niceties of the new property if you don't pack  
> > >
> > > There are legacy parts of netlink interface with kernel.
> > > The ABI has evolved over time but some old parts are stuck in the past.
> > >
> > >  
> > > > > I don't have time to take this to ground, but address listing is not  
> > > the  
> > > > > only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> > > > > have thought about this for route dumps, but its solution does not work
> > > > > here. You'll need to find something because the current warning on  
> > > every  
> > > > > address dump is not acceptable.  
> > > >
> > > > Two points before I propose a migitation:
> > > >
> > > > 1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
> > > >    userspace is doing something wrong is imho justifiable.
> > > >    Actually, I would argue that we should not hide the problem from
> > > >    userspace at all. The rate-limited (so no logging DOS afaict) warning
> > > >    messages are a perfect indicator that a tool is doing something wrong
> > > >    *without* introducing any regressions.
> > > >    The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
> > > >    be used too. Additionally, userspace stuffs an ifinfomsg in there but
> > > >    expects to receive ifaddrmsg. They should be warned loudly. :) So I
> > > >    actually like the warning messages.
> > > > 2. Userspace should be fixed. Especially such an important standard tool
> > > >    as iproute2 that is maintained on git.kernel.org (glibc is already
> > > >    doing the right.).
> > > >
> > > > So if people really want to hide this issue as much as we can then we
> > > > can play the guessing game. I could send a patch that roughly does the
> > > > following:
> > > >
> > > > if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> > > >         guessed_header_len = sizeof(struct ifaddrmsg);
> > > > else
> > > >         guessed_header_len = sizeof(struct ifinfomsg);
> > > >
> > > > This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> > > > The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> > > > This propert is a __s32 which should bring the message up to 12 bytes
> > > > (not sure about alignment requiremnts and where we might wend up ten)
> > > > which is still less than the 16 bytes without that property from
> > > > ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> > > > break when ifaddrmsg grows a new member or we introduce another property
> > > > that is valid in RTM_GETADDR requests. It also will not work cleanly
> > > > when users stuff additional properties in there that are valid for the
> > > > address family but are not used int RTM_GETADDR requests.
> > > >
> > > > I would like to hear what other people and davem think we should do.
> > > > Patch it away or print the warning.
> > > >
> > > > Christian  
> > >
> > > You can't break old programs. That is one of the rules of kernel.
> > > Therefore, please either revert the kernel change or put the new attribute
> > > in a place where old versions do not cause problem.
> > >  
> > 
> > Sorry, there's a misunderstanding here. The code doesn't regress anything.
> > The patch  was  written  in a backward compatible way. The only thing it
> > causes are rate-limited logging messages when the wrong struct is passed.
> > But the request still succeeds. The issue is with the logging afaict.
> > 
> > Christian
> 
> 
> That still means enterprise distributions that use the current kernel will
> get customer complaints. You need to remove the warning.

First, you're the senior developer and I totally accept your decision so
we'll make the warning go away!

Rate-limited messages in dmesg on an edge kernel in response to wrong
userspace behavior *without any functional regressions* is something
that I find justifiable and to some extent necessary.
The promise about not regressing userspace is about identical results
from the kernel on unchanged userspace behavior. The contract doesn't
and shouldn't include "We're also not going to tell you that you're
doing something wrong.".
The log messages are an indicator to userspace that "Hey, you're passing
me something wrong in this request. I still will fulfill your request
just as before but please, think about changing this.". That's basically
the only way we have to get userspace to correct false prior behavior
without introducing regressions. Considering warnings in log messages to
be regressions leaves us with nothing visible to do.

Christian

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Jiri Benc]

On Tue, 25 Sep 2018 11:49:10 +0200, Christian Brauner wrote:
> So if people really want to hide this issue as much as we can then we
> can play the guessing game. I could send a patch that roughly does the
> following:
> 
> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
>         guessed_header_len = sizeof(struct ifaddrmsg);
> else
>         guessed_header_len = sizeof(struct ifinfomsg);
> 
> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> This propert is a __s32 which should bring the message up to 12 bytes
> (not sure about alignment requiremnts and where we might wend up ten)
> which is still less than the 16 bytes without that property from
> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> break when ifaddrmsg grows a new member or we introduce another property
> that is valid in RTM_GETADDR requests. It also will not work cleanly
> when users stuff additional properties in there that are vaif (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,lid for the
> address family but are not used int RTM_GETADDR requests.

I'd expect that any potential existing code that makes use of other
attributes already assumes ifaddrmsg. Hence, if the nlmsg_len is >
sizeof(ifinfomsg), you can be sure that there are attributes and thus
the struct used was ifaddrmsg.

So, in order for RTM_GETADDR to work reliably with attributes, you have
to ensure that the length is > sizeof(ifinfomsg).

This can be achieved by putting IFA_TARGET_NETNSID into a nested
attribute. Just define IFA_EXTENDED (feel free to invent a better name,
of course) and put IFA_TARGET_NETNSID inside. Then in the code, attempt
to parse only when the size is large enough:

	if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
		int err;
 
		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
		                  IFA_MAX, ifa_ipv6_policy, NULL);
		if (err < 0)
			return err;
		if (tb[IFA_EXTENDED]) {
			...parse the nested attribute...
			if (tb_nested[IFA_TARGET_NETNSID]) {
				...etc...
			}
		}
	}

Another option is forcing the user space to add another attribute, for
example, IFA_FLAGS_PRESENT, and attempt parsing only when it is
present. The logic would then be:

	if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
		int err;
 
		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
		                  IFA_MAX, ifa_ipv6_policy, NULL);
		if (err < 0)
			return err;
		if (tb[IFA_FLAGS_PRESENT] && tb[IFA_TARGET_NETNSID]) {
			...etc...
		}
	}

 Jiri

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[David Ahern]

On 9/25/18 8:47 AM, Jiri Benc wrote:
> On Tue, 25 Sep 2018 11:49:10 +0200, Christian Brauner wrote:
>> So if people really want to hide this issue as much as we can then we
>> can play the guessing game. I could send a patch that roughly does the
>> following:
>>
>> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
>>         guessed_header_len = sizeof(struct ifaddrmsg);
>> else
>>         guessed_header_len = sizeof(struct ifinfomsg);
>>
>> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
>> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
>> This propert is a __s32 which should bring the message up to 12 bytes
>> (not sure about alignment requiremnts and where we might wend up ten)
>> which is still less than the 16 bytes without that property from
>> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
>> break when ifaddrmsg grows a new member or we introduce another property
>> that is valid in RTM_GETADDR requests. It also will not work cleanly
>> when users stuff additional properties in there that are vaif (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,lid for the
>> address family but are not used int RTM_GETADDR requests.
> 
> I'd expect that any potential existing code that makes use of other
> attributes already assumes ifaddrmsg. Hence, if the nlmsg_len is >
> sizeof(ifinfomsg), you can be sure that there are attributes and thus
> the struct used was ifaddrmsg.
> 
> So, in order for RTM_GETADDR to work reliably with attributes, you have
> to ensure that the length is > sizeof(ifinfomsg).

One of the many on-going efforts I have in progress is kernel side
filtering of route dumps. It has this same problem. For it I am
proposing a new flag:

#define RTM_F_PROPER_HEADER    0x4000

ifinfomsg is 16 bytes which is > rtmsg at 12. If the message length is >
16, then rtm_flags can be checked to know if the proper header is sent.

For ifaddrmsg things do not line up as well. Worst all of the flag bits
are used. But, perhaps one can be overloaded with the limit that you can
never filter on its presence. Since you are adding the first filter to
address dumps such a limitation should be ok.

For example something like this (whitespace damaged on paste) to remove
the guess work:

diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
index dfcf3ce0097f..8e3e9d475db5 100644
--- a/include/uapi/linux/if_addr.h
+++ b/include/uapi/linux/if_addr.h
@@ -41,6 +41,11 @@ enum {
 #define IFA_MAX (__IFA_MAX - 1)

 /* ifa_flags */
+/* IFA_F_PROPER_HEADER is only set in ifa_flags for dumps
+ * to indicate the ancillary header is the expected ifaddrmsg
+ * vs ifinfomsg from legacy userspace
+ */
+#define IFA_F_PROPER_HEADER    0x01
 #define IFA_F_SECONDARY                0x01
 #define IFA_F_TEMPORARY                IFA_F_SECONDARY

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index bfe3ec7ecb14..256b9f88db8f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5022,8 +5022,13 @@ static int inet6_dump_addr(struct sk_buff *skb,
struct netlink_callback *cb,
        s_idx = idx = cb->args[1];
        s_ip_idx = ip_idx = cb->args[2];

-       if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
-                       ifa_ipv6_policy, NULL) >= 0) {
+       if (nlmsg_len(cb->nlh) >= sizeof(struct ifaddrmsg) &&
+           ((struct ifaddrmsg *) nlmsg_data(cb->nlh))->ifa_flags &
IFA_F_PROPER_HEADER) {
+
+               if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
IFA_MAX,
+                               ifa_ipv6_policy, NULL) >= 0) {
+       ...
+
                if (tb[IFA_TARGET_NETNSID]) {
                        netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);


For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
so this should be ok.

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Jiri Benc]

On Tue, 25 Sep 2018 09:37:41 -0600, David Ahern wrote:
> For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
> so this should be ok.

Does the existing user space set ifi_type to anything? Does it zero out
the field?

Are we able to find a flag value that is not going to be set by unaware
user space? I.e., a bit that is unused by the current ARPHRD values on
both little and big endian? (ARPHRD_NONE might be a problem, though...)

 Jiri

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[David Ahern]

On 9/25/18 11:51 PM, Jiri Benc wrote:
> On Tue, 25 Sep 2018 09:37:41 -0600, David Ahern wrote:
>> For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
>> so this should be ok.
> 
> Does the existing user space set ifi_type to anything? Does it zero out
> the field?
> 
> Are we able to find a flag value that is not going to be set by unaware
> user space? I.e., a bit that is unused by the current ARPHRD values on
> both little and big endian? (ARPHRD_NONE might be a problem, though...)

The goal is for userpsace to pass something to the kernel to
definitively state which header is used.

ifaddrmsg (proper header and one Christian's patch wants to use) is 8
bytes; ifinfomsg (legacy header from broken userspace) is 16. If you can
not trust that ifi_type is currently 0 on a dump request then you can
not trust ifi_flags to be correct or ifi_change to be correct and so you
can not move past the header and parse attributes. If that is the case
we are done - Christian's patches should be reverted as you can never
trust what is beyond the family entry.

But I do not believe that to be the case because of the route dump
analogy. As I mentioned route dumps have the same problem: sometimes
ifinfomsg is passed and sometimes rtmsg. Yet the kernel always looks at
rtm_flags.

In terms of which field to use the most logical to me is to pass in a
flag. Current address dumps have no reason to pass in a flag so it is
not like the field can be misinterpreted. ifa_flags is a single byte so
are there really endian issues to worry about?

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Stephen Hemminger]

On Wed, 26 Sep 2018 08:54:43 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 9/25/18 11:51 PM, Jiri Benc wrote:
> > On Tue, 25 Sep 2018 09:37:41 -0600, David Ahern wrote:  
> >> For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
> >> so this should be ok.  
> > 
> > Does the existing user space set ifi_type to anything? Does it zero out
> > the field?
> > 
> > Are we able to find a flag value that is not going to be set by unaware
> > user space? I.e., a bit that is unused by the current ARPHRD values on
> > both little and big endian? (ARPHRD_NONE might be a problem, though...)  
> 
> The goal is for userpsace to pass something to the kernel to
> definitively state which header is used.
> 

You can not safely assume anything about older code.
iproute2 is not the only thing using the API, others include Quagga, and other tools.

Sorry, this API is frozen.

Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 4411 bytes --]

On Tue, Sep 25, 2018 at 09:37:41AM -0600, David Ahern wrote:
> On 9/25/18 8:47 AM, Jiri Benc wrote:
> > On Tue, 25 Sep 2018 11:49:10 +0200, Christian Brauner wrote:
> >> So if people really want to hide this issue as much as we can then we
> >> can play the guessing game. I could send a patch that roughly does the
> >> following:
> >>
> >> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> >>         guessed_header_len = sizeof(struct ifaddrmsg);
> >> else
> >>         guessed_header_len = sizeof(struct ifinfomsg);
> >>
> >> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> >> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> >> This propert is a __s32 which should bring the message up to 12 bytes
> >> (not sure about alignment requiremnts and where we might wend up ten)
> >> which is still less than the 16 bytes without that property from
> >> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> >> break when ifaddrmsg grows a new member or we introduce another property
> >> that is valid in RTM_GETADDR requests. It also will not work cleanly
> >> when users stuff additional properties in there that are vaif (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,lid for the
> >> address family but are not used int RTM_GETADDR requests.
> > 
> > I'd expect that any potential existing code that makes use of other
> > attributes already assumes ifaddrmsg. Hence, if the nlmsg_len is >
> > sizeof(ifinfomsg), you can be sure that there are attributes and thus
> > the struct used was ifaddrmsg.
> > 
> > So, in order for RTM_GETADDR to work reliably with attributes, you have
> > to ensure that the length is > sizeof(ifinfomsg).
> 
> One of the many on-going efforts I have in progress is kernel side
> filtering of route dumps. It has this same problem. For it I am
> proposing a new flag:
> 
> #define RTM_F_PROPER_HEADER    0x4000
> 
> ifinfomsg is 16 bytes which is > rtmsg at 12. If the message length is >
> 16, then rtm_flags can be checked to know if the proper header is sent.
> 
> For ifaddrmsg things do not line up as well. Worst all of the flag bits
> are used. But, perhaps one can be overloaded with the limit that you can
> never filter on its presence. Since you are adding the first filter to
> address dumps such a limitation should be ok.
> 
> For example something like this (whitespace damaged on paste) to remove
> the guess work:
> 
> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index dfcf3ce0097f..8e3e9d475db5 100644
> --- a/include/uapi/linux/if_addr.h
> +++ b/include/uapi/linux/if_addr.h
> @@ -41,6 +41,11 @@ enum {
>  #define IFA_MAX (__IFA_MAX - 1)
> 
>  /* ifa_flags */
> +/* IFA_F_PROPER_HEADER is only set in ifa_flags for dumps
> + * to indicate the ancillary header is the expected ifaddrmsg
> + * vs ifinfomsg from legacy userspace
> + */
> +#define IFA_F_PROPER_HEADER    0x01
>  #define IFA_F_SECONDARY                0x01
>  #define IFA_F_TEMPORARY                IFA_F_SECONDARY
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index bfe3ec7ecb14..256b9f88db8f 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5022,8 +5022,13 @@ static int inet6_dump_addr(struct sk_buff *skb,
> struct netlink_callback *cb,
>         s_idx = idx = cb->args[1];
>         s_ip_idx = ip_idx = cb->args[2];
> 
> -       if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -                       ifa_ipv6_policy, NULL) >= 0) {
> +       if (nlmsg_len(cb->nlh) >= sizeof(struct ifaddrmsg) &&
> +           ((struct ifaddrmsg *) nlmsg_data(cb->nlh))->ifa_flags &
> IFA_F_PROPER_HEADER) {
> +
> +               if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
> IFA_MAX,
> +                               ifa_ipv6_policy, NULL) >= 0) {
> +       ...
> +
>                 if (tb[IFA_TARGET_NETNSID]) {
>                         netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> 
> 
> For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
> so this should be ok.

I like this idea way more than forcing userspace to create a nested
attribute. If Jiri's question is answered can we get this patch on the
road soon?
Are you happy to send it on-top of net-next or do you want me to do it?

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]