| Message ID | 20180927175857.3511-1-christian@brauner.io |
|---|---|
| Headers | show |
| Series | rtnetlink: add RTM_GETADDR2 | expand |
On 9/27/18 11:58 AM, Christian Brauner wrote: > Various userspace programs (e.g. iproute2) have sent RTM_GETADDR > requests with struct ifinfomsg. This is wrong and should have been > struct ifaddrmsg all along as mandated by the manpages. However, dump > requests so far didn't parse the netlink message that was sent and > succeeded even when a wrong struct was passed along. ... > The correct solution at this point seems to me to introduce a new > RTM_GETADDR2 request. This way we can parse the message and fail hard if > the struct is not struct ifaddrmsg and can safely extend it in the > future. Userspace tools that rely on the buggy RTM_GETADDR API will > still keep working without even having to see any log messages and new > userspace tools that want to make user of new features can make use of > the new RTM_GETADDR2 requests. First, I think this is the wrong precedent when all we need is a single bit flag that userspace can use to tell the kernel "I have a clue and I am passing in the proper header for this dump request". Second, you are not addressing the problems of the past by requiring the proper header and checking values passed in it. I have another idea. I'll send an RFC patch soon.
On September 27, 2018 10:24:36 PM GMT+02:00, David Ahern <dsahern@gmail.com> wrote: >On 9/27/18 11:58 AM, Christian Brauner wrote: >> Various userspace programs (e.g. iproute2) have sent RTM_GETADDR >> requests with struct ifinfomsg. This is wrong and should have been >> struct ifaddrmsg all along as mandated by the manpages. However, dump >> requests so far didn't parse the netlink message that was sent and >> succeeded even when a wrong struct was passed along. > >... > >> The correct solution at this point seems to me to introduce a new >> RTM_GETADDR2 request. This way we can parse the message and fail hard >if >> the struct is not struct ifaddrmsg and can safely extend it in the >> future. Userspace tools that rely on the buggy RTM_GETADDR API will >> still keep working without even having to see any log messages and >new >> userspace tools that want to make user of new features can make use >of >> the new RTM_GETADDR2 requests. > >First, I think this is the wrong precedent when all we need is a single >bit flag that userspace can use to tell the kernel "I have a clue and I >am passing in the proper header for this dump request". That had been NAKed previously but if you have an idea that will be accepted all the more power to you. > >Second, you are not addressing the problems of the past by requiring >the >proper header and checking values passed in it. I don't follow. RTM_GETADDR requests are absolutely unchanged. The full legacy behavior is restored by this patchset. And requiring that RTM_GETADDR2 requests always pass the correct header is absolutely fine. We don't want built invalid legacy behavior into a new request type. > >I have another idea. I'll send an RFC patch soon.
Various userspace programs (e.g. iproute2) have sent RTM_GETADDR requests with struct ifinfomsg. This is wrong and should have been struct ifaddrmsg all along as mandated by the manpages. However, dump requests so far didn't parse the netlink message that was sent and succeeded even when a wrong struct was passed along. Currently, the message is parsed under the assumption that the correct struct ifaddrmsg is sent down. If the parsing fails the kernel will still fulfill the request to preserve backwards compatability but a rate-limited message that there were leftover bytes after parsing the message is recorded in dmesg. It has been argued that this is unacceptable [1]. But various new features that got and will get added to RTM_GETADDR make it necessary to definitely know what header was passed along. This is currently not possible without resorting to (likely unreliable) hacks such as introducing a nested attribute that ensures that RTM_GETADDR which pass along properties such as IFA_TARGET_NETNSID always exceed RTM_GETADDR requests that send down the wrong struct ifinfomsg [2]. Basically, multiple approaches to solve this have been shut down. Furthermore, the API expressed via RTM_GETADDR is apparently frozen [3] so the wiggle room at this point seems very much zero. The correct solution at this point seems to me to introduce a new RTM_GETADDR2 request. This way we can parse the message and fail hard if the struct is not struct ifaddrmsg and can safely extend it in the future. Userspace tools that rely on the buggy RTM_GETADDR API will still keep working without even having to see any log messages and new userspace tools that want to make user of new features can make use of the new RTM_GETADDR2 requests. [1]: https://lists.openwall.net/netdev/2018/09/25/59 [2]: https://lists.openwall.net/netdev/2018/09/25/75 [3]: https://lists.openwall.net/netdev/2018/09/26/166 Signed-off-by: Christian Brauner <christian@brauner.io> Cc: David Ahern <dsahern@gmail.com> Cc: Jiri Benc <jbenc@redhat.com> Cc: Stephen Hemminger <stephen@networkplumber.org> Christian Brauner (7): rtnetlink: add RTM_GETADDR2 ipv4: add RTM_GETADDR2 ipv6: add RTM_GETADDR2 decnet: add RTM_GETADDR2 phonet: add RTM_GETADDR2 selinux: add RTM_GETADDR2 rtnetlink: enable RTM_GETADDR2 include/uapi/linux/rtnetlink.h | 3 +++ net/core/rtnetlink.c | 1 + net/decnet/dn_dev.c | 25 +++++++++++++++++++++++-- net/ipv4/devinet.c | 24 +++++++++++++++++++++--- net/ipv6/addrconf.c | 30 ++++++++++++++++++++++++------ net/phonet/pn_netlink.c | 25 +++++++++++++++++++++++-- security/selinux/nlmsgtab.c | 3 ++- 7 files changed, 97 insertions(+), 14 deletions(-)