Message-ID: <20180925094908.yuu5bsrazkiq3ihy@brauner.io>
Date:   Tue, 25 Sep 2018 11:49:10 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Ahern <dsahern@...il.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: netlink: 16 bytes leftover after parsing attributes in process
 `ip'.

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@...uner.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists