Kernel header changes break glibc build

Kernel header changes break glibc build

[Joseph S. Myers]

The kernel headers installed by Linux 2.6.19-rc1 "make
headers_install" do not work for building glibc, because glibc expects
<linux/rtnetlink.h> to provide various definitions, some of which have
been moved to <linux/if_addr.h> and some of which have been removed
altogether.

This kernel patch allows glibc to build again by making rtnetlink.h
include if_addr.h and adding back the removed definitions required by
glibc, but I don't know if it's the correct approach or if glibc
should change the headers it includes and add its own macro
definitions.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
Index: include/linux/rtnetlink.h
===================================================================
--- include/linux/rtnetlink.h
+++ include/linux/rtnetlink.h
@@ -2,6 +2,7 @@
 #define __LINUX_RTNETLINK_H
 
 #include <linux/netlink.h>
+#include <linux/if_addr.h>
 #include <linux/if_link.h>
 
 /****
Index: include/linux/if_link.h
===================================================================
--- include/linux/if_link.h
+++ include/linux/if_link.h
@@ -82,6 +82,9 @@
 
 #define IFLA_MAX (__IFLA_MAX - 1)
 
+#define IFLA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifinfomsg))))
+#define IFLA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifinfomsg))
+
 /* ifi_flags.
 
    IFF_* flags.
Index: include/linux/if_addr.h
===================================================================
--- include/linux/if_addr.h
+++ include/linux/if_addr.h
@@ -52,4 +52,7 @@
 	__u32	tstamp; /* updated timestamp, hundredths of seconds */
 };
 
+#define IFA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifaddrmsg))))
+#define IFA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifaddrmsg))
+
 #endif



-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Kernel header changes break glibc build

[David Woodhouse]

On Fri, 2006-10-06 at 17:20 +0000, Joseph S. Myers wrote:
> The kernel headers installed by Linux 2.6.19-rc1 "make
> headers_install" do not work for building glibc, because glibc expects
> <linux/rtnetlink.h> to provide various definitions, some of which have
> been moved to <linux/if_addr.h> and some of which have been removed
> altogether.
> 
> This kernel patch allows glibc to build again by making rtnetlink.h
> include if_addr.h and adding back the removed definitions required by
> glibc, but I don't know if it's the correct approach or if glibc
> should change the headers it includes and add its own macro
> definitions.
> 
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>

Since it was Thomas Graf who made these changes and David Miller who
committed them, you'd do well to Cc both of those people.

Sorry for the delayed response on my part -- I've been away
concentrating on OLPC stuff, and just about everything else has fallen
by the wayside.

Thomas, this is in response to your changes in
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=1823730fbc89fadde72a7bb3b7bdf03cc7b8835c;hp=47f68512d2685431f1781830dfcbab31bda87644
in which you create <linux/if_addr.h> and require that it's included
directly rather than being part of (or even included from)
<linux/rtnetlink.h>. Was there a good reason for changing that
user-visible header? Is there a reason not to include if_addr.h from
rtnetlink.h as Joseph's patch does?

I suspect that if the IF{L,}A_{PAYLOAD,RTA} macros aren't used in the
kernel then the best answer is for glibc to define those for itself.

> ---
> Index: include/linux/rtnetlink.h
> ===================================================================
> --- include/linux/rtnetlink.h
> +++ include/linux/rtnetlink.h
> @@ -2,6 +2,7 @@
>  #define __LINUX_RTNETLINK_H
>  
>  #include <linux/netlink.h>
> +#include <linux/if_addr.h>
>  #include <linux/if_link.h>
>  
>  /****
> Index: include/linux/if_link.h
> ===================================================================
> --- include/linux/if_link.h
> +++ include/linux/if_link.h
> @@ -82,6 +82,9 @@
>  
>  #define IFLA_MAX (__IFLA_MAX - 1)
>  
> +#define IFLA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifinfomsg))))
> +#define IFLA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifinfomsg))
> +
>  /* ifi_flags.
>  
>     IFF_* flags.
> Index: include/linux/if_addr.h
> ===================================================================
> --- include/linux/if_addr.h
> +++ include/linux/if_addr.h
> @@ -52,4 +52,7 @@
>  	__u32	tstamp; /* updated timestamp, hundredths of seconds */
>  };
>  
> +#define IFA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifaddrmsg))))
> +#define IFA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifaddrmsg))
> +
>  #endif
> 
> 
> 
-- 
dwmw2

Re: Kernel header changes break glibc build

[Thomas Graf]

* David Woodhouse <dwmw2@infradead.org> 2006-12-03 12:25
> Thomas, this is in response to your changes in
> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=1823730fbc89fadde72a7bb3b7bdf03cc7b8835c;hp=47f68512d2685431f1781830dfcbab31bda87644
> in which you create <linux/if_addr.h> and require that it's included
> directly rather than being part of (or even included from)
> <linux/rtnetlink.h>. Was there a good reason for changing that
> user-visible header? Is there a reason not to include if_addr.h from
> rtnetlink.h as Joseph's patch does?

Userspace is not supposd to directly include kernel headers, instead
it has to make local copies and compile against them. Binary
compatibility is always guaranteed but in times of development within
a stable tree it's wrong to assume that headers never change.

I do not agree with the change to include if_addr.h in rtnetlink.h.
The point is to move bits apart and have multiple small pieces
of header files defining a specific rtnetlink family which are a
lot easier to maintain for both kernel and userspace than one giant
rtnetlink.h for everything.

> I suspect that if the IF{L,}A_{PAYLOAD,RTA} macros aren't used in the
> kernel then the best answer is for glibc to define those for itself.

Right, if they did it right they would only have noticed when they
updated the kernel headers to some newer versions and only had to
move the bits to some compat header.

Re: Kernel header changes break glibc build

[David Woodhouse]

On Mon, 2006-12-04 at 10:13 +0100, Thomas Graf wrote:
> Userspace is not supposd to directly include kernel headers, instead
> it has to make local copies and compile against them.

No. It was _never_ sensible to simply declare that userspace "shall not
use kernel headers" in the absence of any serious alternative. It was
always just a cop-out.

Historically, there were a number of efforts to provide 'sanitised'
kernel headers which are needed for userspace to build against. Various
people forked headers from the kernel at different times, got burned out
at different times, took different approaches w.r.t. how much abuse of
kernel-private stuff should be permitted. We ended up with a bunch of
inconsistent sets of 'kernel headers' each done differently and updated
sporadically.

Thankfully, this is no longer the case. The kernel now has a
'headers_install' make target which spits out those headers which are
suitable for userspace, sanitised by removing anything within
__KERNEL__. We have a _consistent_, up to date set of headers which
distributions can use for building glibc and other system libraries and
tools. Some distributions are already shipping like that; others are
still working on doing so.

> Binary compatibility is always guaranteed but in times of development
> within a stable tree it's wrong to assume that headers never change.
>
> I do not agree with the change to include if_addr.h in rtnetlink.h.
> The point is to move bits apart and have multiple small pieces
> of header files defining a specific rtnetlink family which are a
> lot easier to maintain for both kernel and userspace than one giant
> rtnetlink.h for everything.

That makes some sense, but we need to be more careful about making
incompatible changes in the headers which we export -- it's no longer
acceptable to just toss a pile of crap over the wall and declare it to
be someone else's problem.

That isn't to say that we should _never_ make such changes, but we
should at least make sure we think about it and make sure that glibc
adapts to cope, _before_ people start to see build failures.

> > I suspect that if the IF{L,}A_{PAYLOAD,RTA} macros aren't used in the
> > kernel then the best answer is for glibc to define those for itself.
> 
> Right, if they did it right they would only have noticed when they
> updated the kernel headers to some newer versions and only had to
> move the bits to some compat header.

No. They _are_ doing it right -- they're running 'make headers_install'
against the 2.6.19 kernel and only _now_ are they finding that we broke
it without even the courtesy of a warning, let alone any consultation.

If _we_ had done it right, then they would have been warned when we
decided to change this, and we wouldn't have just released 2.6.19 with
changes which break the glibc build.

-- 
dwmw2

Re: Kernel header changes break glibc build

[Jakub Jelinek]

On Wed, Dec 06, 2006 at 01:01:54PM +0000, David Woodhouse wrote:
> No. They _are_ doing it right -- they're running 'make headers_install'
> against the 2.6.19 kernel and only _now_ are they finding that we broke
> it without even the courtesy of a warning, let alone any consultation.
> 
> If _we_ had done it right, then they would have been warned when we
> decided to change this, and we wouldn't have just released 2.6.19 with
> changes which break the glibc build.

Yeah, I don't think glibc was doing anything wrong and the 2.6.19
changes to the make headers_install created headers mean we'd
either need to add configure checks for the headers (we can't
simply #include <linux/if_addr.h> because that header didn't
exist pre 2.6.19 and IF*_{RTA,PAYLOAD} macros were dropped anyway),
or we need to start defining this ourselves.

Here is the second variant.
I just hope further kernel header "cleanups" don't cause similar
breakage though.

2006-12-06  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/unix/sysv/linux/netlinkaccess.h (struct ifaddrmsg): New type.
	(IFA_UNSPEC, IFA_ADDRESS, IFA_LOCAL, IFA_LABEL, IFA_BROADCAST,
	IFA_ANYCAST, IFA_CACHEINFO, IFA_MULTICAST): New enum.
	(IFA_F_SECONDARY, IFA_F_TEMPORARY, IFA_F_HOMEADDRESS,
	IFA_F_DEPRECATED): Define if not defined.
	(IFA_RTA, IFA_PAYLOAD, IFLA_RTA, IFLA_PAYLOAD): Likewise.
	* sysdeps/unix/sysv/linux/check_pf.c: Include netlinkaccess.h
	instead of asm/types.h, linux/netlink.h and linux/rtnetlink.h.

--- libc/sysdeps/unix/sysv/linux/netlinkaccess.h.jj	2006-01-08 09:21:15.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/netlinkaccess.h	2006-12-06 13:48:50.000000000 +0100
@@ -25,6 +25,51 @@
 
 #include <kernel-features.h>
 
+/* 2.6.19 kernel headers helpfully removed some macros and
+   moved lots of stuff into new headers, some of which aren't
+   included by linux/rtnetlink.h.  */
+
+#ifndef IFA_MAX
+struct ifaddrmsg
+{
+  uint8_t ifa_family;
+  uint8_t ifa_prefixlen;
+  uint8_t ifa_flags;
+  uint8_t ifa_scope;
+  uint32_t ifa_index;
+};
+
+enum
+{
+  IFA_UNSPEC,
+  IFA_ADDRESS,
+  IFA_LOCAL,
+  IFA_LABEL,
+  IFA_BROADCAST,
+  IFA_ANYCAST,
+  IFA_CACHEINFO,
+  IFA_MULTICAST
+};
+#endif
+
+#ifndef IFA_F_SECONDARY
+# define IFA_F_SECONDARY	0x01
+# define IFA_F_TEMPORARY	IFA_F_SECONDARY
+# define IFA_F_HOMEADDRESS	0x10
+# define IFA_F_DEPRECATED	0x20
+#endif
+
+#ifndef IFA_RTA
+# define IFA_RTA(r) \
+  ((struct rtattr *) ((char *)(r) + NLMSG_ALIGN (sizeof (struct ifaddrmsg))))
+# define IFA_PAYLOAD(n) NLMSG_PAYLOAD (n, sizeof (struct ifaddrmsg))
+#endif
+
+#ifndef IFLA_RTA
+# define IFLA_RTA(r) \
+  ((struct rtattr *) ((char *)(r) + NLMSG_ALIGN (sizeof (struct ifinfomsg))))
+# define IFLA_PAYLOAD(n) NLMSG_PAYLOAD (n, sizeof (struct ifinfomsg))
+#endif
 
 struct netlink_res
 {
--- libc/sysdeps/unix/sysv/linux/check_pf.c.jj	2006-09-24 18:50:22.000000000 +0200
+++ libc/sysdeps/unix/sysv/linux/check_pf.c	2006-12-06 13:54:37.000000000 +0100
@@ -27,13 +27,10 @@
 #include <unistd.h>
 #include <sys/socket.h>
 
-#include <asm/types.h>
-#include <linux/netlink.h>
-#include <linux/rtnetlink.h>
-
 #include <not-cancel.h>
 #include <kernel-features.h>
 
+#include "netlinkaccess.h"
 
 #ifndef IFA_F_TEMPORARY
 # define IFA_F_TEMPORARY IFA_F_SECONDARY


	Jakub

Re: Kernel header changes break glibc build

[David Woodhouse]

On Wed, 2006-12-06 at 14:43 +0100, Jakub Jelinek wrote:
> 
> +/* 2.6.19 kernel headers helpfully removed some macros and
> +   moved lots of stuff into new headers, some of which aren't
> +   included by linux/rtnetlink.h.  */
> +
> +#ifndef IFA_MAX
> +struct ifaddrmsg
> +{
> +  uint8_t ifa_family;
> +  uint8_t ifa_prefixlen;
> +  uint8_t ifa_flags;
> +  uint8_t ifa_scope;
> +  uint32_t ifa_index;
> +};
> +
> +enum
> +{
> +  IFA_UNSPEC,
> +  IFA_ADDRESS,
> +  IFA_LOCAL,
> +  IFA_LABEL,
> +  IFA_BROADCAST,
> +  IFA_ANYCAST,
> +  IFA_CACHEINFO,
> +  IFA_MULTICAST
> +};
> +#endif
> +
> +#ifndef IFA_F_SECONDARY
> +# define IFA_F_SECONDARY       0x01
> +# define IFA_F_TEMPORARY       IFA_F_SECONDARY
> +# define IFA_F_HOMEADDRESS     0x10
> +# define IFA_F_DEPRECATED      0x20
> +#endif

This much is still available from the new <linux/if_addr.h> -- you could
include that directly instead of copying it.

-- 
dwmw2

Re: Kernel header changes break glibc build

[Jakub Jelinek]

On Wed, Dec 06, 2006 at 01:51:07PM +0000, David Woodhouse wrote:
> On Wed, 2006-12-06 at 14:43 +0100, Jakub Jelinek wrote:
> > 
> > +/* 2.6.19 kernel headers helpfully removed some macros and
> > +   moved lots of stuff into new headers, some of which aren't
> > +   included by linux/rtnetlink.h.  */
> > +
> > +#ifndef IFA_MAX
> > +struct ifaddrmsg
> > +{
> > +  uint8_t ifa_family;
> > +  uint8_t ifa_prefixlen;
> > +  uint8_t ifa_flags;
> > +  uint8_t ifa_scope;
> > +  uint32_t ifa_index;
> > +};
> > +
> > +enum
> > +{
> > +  IFA_UNSPEC,
> > +  IFA_ADDRESS,
> > +  IFA_LOCAL,
> > +  IFA_LABEL,
> > +  IFA_BROADCAST,
> > +  IFA_ANYCAST,
> > +  IFA_CACHEINFO,
> > +  IFA_MULTICAST
> > +};
> > +#endif
> > +
> > +#ifndef IFA_F_SECONDARY
> > +# define IFA_F_SECONDARY       0x01
> > +# define IFA_F_TEMPORARY       IFA_F_SECONDARY
> > +# define IFA_F_HOMEADDRESS     0x10
> > +# define IFA_F_DEPRECATED      0x20
> > +#endif
> 
> This much is still available from the new <linux/if_addr.h> -- you could
> include that directly instead of copying it.

Yes, but as I said, I'd need to add configure checks for that, using
#include <linux/if_addr.h>
alone breaks build with older headers.
glibc so far managed to build without a single configure check for header
existence, this would be the first place where something like that is
needed.

	Jakub

Re: Kernel header changes break glibc build

[Thomas Graf]

* Jakub Jelinek <jakub@redhat.com> 2006-12-06 14:43
> On Wed, Dec 06, 2006 at 01:01:54PM +0000, David Woodhouse wrote:
> > No. They _are_ doing it right -- they're running 'make headers_install'
> > against the 2.6.19 kernel and only _now_ are they finding that we broke
> > it without even the courtesy of a warning, let alone any consultation.
> > 
> > If _we_ had done it right, then they would have been warned when we
> > decided to change this, and we wouldn't have just released 2.6.19 with
> > changes which break the glibc build.
> 
> Yeah, I don't think glibc was doing anything wrong and the 2.6.19
> changes to the make headers_install created headers mean we'd
> either need to add configure checks for the headers (we can't
> simply #include <linux/if_addr.h> because that header didn't
> exist pre 2.6.19 and IF*_{RTA,PAYLOAD} macros were dropped anyway),
> or we need to start defining this ourselves.

Are you suggesting that the kernel has to keep macros around which
are of no use to the kernel itself just because glibc uses them?

What's wrong with copying the headers and ship them? Every glibc
release is based on some kernel version anyway and its no problem
to run glibc compiled with a 2.6.19 header set on a 2.6.18 kernel.

Re: Kernel header changes break glibc build

[David Woodhouse]

On Wed, 2006-12-06 at 14:57 +0100, Jakub Jelinek wrote:
> Yes, but as I said, I'd need to add configure checks for that, using
> #include <linux/if_addr.h>
> alone breaks build with older headers.

I was thinking that the #ifndef IFA_MAX you already have ought to be
sufficient for that. Or even checking KERNEL_VERSION.

-- 
dwmw2

Re: Kernel header changes break glibc build

[David Woodhouse]

On Wed, 2006-12-06 at 14:59 +0100, Thomas Graf wrote:
> Are you suggesting that the kernel has to keep macros around which
> are of no use to the kernel itself just because glibc uses them?

No, although in fact that _is_ the only reason we use these horrid __uXX
types rather than proper C datatypes, isn't it?

I'm suggesting that if you want to change things around as you did, you
should make sure the users of those headers adapt to cope. You did fix
the in-kernel users; you neglected to fix glibc -- and as far as I can
tell you didn't even bother to _warn_ glibc folks.

We need to do better than that. The dark ages where we used to toss a
pile of crap over the wall and declare it was someone else's problem are
now behind us, thankfully.

-- 
dwmw2

Re: Kernel header changes break glibc build

[Jakub Jelinek]

On Wed, Dec 06, 2006 at 02:07:19PM +0000, David Woodhouse wrote:
> On Wed, 2006-12-06 at 14:59 +0100, Thomas Graf wrote:
> > Are you suggesting that the kernel has to keep macros around which
> > are of no use to the kernel itself just because glibc uses them?
> 
> No, although in fact that _is_ the only reason we use these horrid __uXX
> types rather than proper C datatypes, isn't it?

There are the kernel's own headers and kernel ABI headers for userland use.
Until recently the latter has been maintained by various distributions
and manually occassionally updated to sync a little bit with kernel ABI
additions (new syscalls, etc.)., but now, thanks to David, these are
generated from kernel's own headers.  If the macros were part of
such ABI (I don't think these macros were meant to be #ifdef __KERNEL__
and just by omission exported to userland), then if you change
the kernel headers (which of course you can do, that's kernel private
headers), then you IMNSHO should also add magic to make headers_install
to keep the kernel ABI headers for userland headers stable.
Which in this case would mean if you decide rtnetlink.h shouldn't include
the newly added if_addr.h that you add rules for generating the userland
rtnetlink.h such that it will include linux/if_addr.h and define the
macros you intentionally omitted.

	Jakub

Re: Kernel header changes break glibc build

[Thomas Graf]

* David Woodhouse <dwmw2@infradead.org> 2006-12-06 14:07
> On Wed, 2006-12-06 at 14:59 +0100, Thomas Graf wrote:
> > Are you suggesting that the kernel has to keep macros around which
> > are of no use to the kernel itself just because glibc uses them?
> 
> No, although in fact that _is_ the only reason we use these horrid __uXX
> types rather than proper C datatypes, isn't it?

Alright, so we agree that there must be a possibility of getting rid of
deprecated crap which leads to interface abusage.

Fixing things is as simple as #ifndef IFA_MAX respectively IFLA_RTA in
some compat header.

> I'm suggesting that if you want to change things around as you did, you
> should make sure the users of those headers adapt to cope. You did fix
> the in-kernel users; you neglected to fix glibc -- and as far as I can
> tell you didn't even bother to _warn_ glibc folks.

I didn't warn them because I didn't know better. I was under the
impression that glibc still maintains their own set of headers
and will fix this automatically when they look at the diff. That's
what I do for my userspace applications that use kernel headers.

Ideally install_headers would do the trick but it often fails f.e.
when some application which uses bsd features thus including net/if.h
also wants to use new linux features and includes linux/if.h which
then conflicts.

Re: Kernel header changes break glibc build

[Thomas Graf]

* Jakub Jelinek <jakub@redhat.com> 2006-12-06 15:18
> There are the kernel's own headers and kernel ABI headers for userland use.
> Until recently the latter has been maintained by various distributions
> and manually occassionally updated to sync a little bit with kernel ABI
> additions (new syscalls, etc.)., but now, thanks to David, these are
> generated from kernel's own headers.  If the macros were part of
> such ABI

Macros can't possibly be part of an ABI :-) You probably mean API.

> (I don't think these macros were meant to be #ifdef __KERNEL__
> and just by omission exported to userland), then if you change
> the kernel headers (which of course you can do, that's kernel private
> headers), then you IMNSHO should also add magic to make headers_install
> to keep the kernel ABI headers for userland headers stable.

At the time they were added they were meant to be exported but netlink
has evolved and we now have a type safe API. Guess what, I'm going to
remove more bits of the old interface because they are no longer needed.

> Which in this case would mean if you decide rtnetlink.h shouldn't include
> the newly added if_addr.h that you add rules for generating the userland
> rtnetlink.h such that it will include linux/if_addr.h and define the
> macros you intentionally omitted.

Sure, moving these bits to some compat header which gets automatically
included by make install_headers instead of removing them sounds
like a good compromise, it just has to be clear that they are deprecated
and not supposed to be used by new code.

Re: Kernel header changes break glibc build

[Al Viro]

On Wed, Dec 06, 2006 at 03:31:46PM +0100, Thomas Graf wrote:
 
> At the time they were added they were meant to be exported but netlink
> has evolved and we now have a type safe API.

Where?  AFAICS, netlink might be considered type-safe only within an
address family...

Re: Kernel header changes break glibc build

[Stefan Rompf]

Am Montag, 4. Dezember 2006 10:13 schrieb Thomas Graf:

> I do not agree with the change to include if_addr.h in rtnetlink.h.
> The point is to move bits apart and have multiple small pieces
> of header files defining a specific rtnetlink family which are a
> lot easier to maintain for both kernel and userspace than one giant
> rtnetlink.h for everything.

According to a user's report, your change also broke compilation of my 
dhcpclient because it neeeds if_addr.h since 2.6.19. Any suggestion how to 
make one source code build on 2.6.19 and older headers? I hope you don't want 
me to check on UTS_RELEASE in a userspace program?

Stefan

Re: Kernel header changes break glibc build

[Thomas Graf]

* Stefan Rompf <stefan@loplof.de> 2006-12-06 20:32
> According to a user's report, your change also broke compilation of my 
> dhcpclient because it neeeds if_addr.h since 2.6.19. Any suggestion how to 
> make one source code build on 2.6.19 and older headers? I hope you don't want 
> me to check on UTS_RELEASE in a userspace program?

#ifndef IFA_MAX
#include <linux/if_addr.h>
#endif

Re: Kernel header changes break glibc build

[Thomas Graf]

* Al Viro <viro@ftp.linux.org.uk> 2006-12-06 17:13
> On Wed, Dec 06, 2006 at 03:31:46PM +0100, Thomas Graf wrote:
>  
> > At the time they were added they were meant to be exported but netlink
> > has evolved and we now have a type safe API.
> 
> Where?  AFAICS, netlink might be considered type-safe only within an
> address family...

The new interface can be found in net/netlink.h, it obsoletes the
old interface which is spread over linux/netlink.h and linux/rtnetlink.h

I'm removing the old bits as soon as I've finished converting all
its users.

An almost identical interface is provided to userspace by libnl.

Re: Kernel header changes break glibc build

[Al Viro]

On Wed, Dec 06, 2006 at 09:26:39PM +0100, Thomas Graf wrote:
> * Al Viro <viro@ftp.linux.org.uk> 2006-12-06 17:13
> > On Wed, Dec 06, 2006 at 03:31:46PM +0100, Thomas Graf wrote:
> >  
> > > At the time they were added they were meant to be exported but netlink
> > > has evolved and we now have a type safe API.
> > 
> > Where?  AFAICS, netlink might be considered type-safe only within an
> > address family...
> 
> The new interface can be found in net/netlink.h, it obsoletes the
> old interface which is spread over linux/netlink.h and linux/rtnetlink.h

... and for different address families you have conflicting policies.
You can't tell if ATTR_... means __le16, __be32, 16byte-array or something
else - the answer depends on the code interpreting the damn thing.
Moreover, you get zero warnings if you use wrong accessor to decode.

Re: Kernel header changes break glibc build

[Thomas Graf]

* Al Viro <viro@ftp.linux.org.uk> 2006-12-06 20:34
> On Wed, Dec 06, 2006 at 09:26:39PM +0100, Thomas Graf wrote:
> > * Al Viro <viro@ftp.linux.org.uk> 2006-12-06 17:13
> > > On Wed, Dec 06, 2006 at 03:31:46PM +0100, Thomas Graf wrote:
> > >  
> > > > At the time they were added they were meant to be exported but netlink
> > > > has evolved and we now have a type safe API.
> > > 
> > > Where?  AFAICS, netlink might be considered type-safe only within an
> > > address family...
> > 
> > The new interface can be found in net/netlink.h, it obsoletes the
> > old interface which is spread over linux/netlink.h and linux/rtnetlink.h
> 
> ... and for different address families you have conflicting policies.
> You can't tell if ATTR_... means __le16, __be32, 16byte-array or something
> else - the answer depends on the code interpreting the damn thing.
> Moreover, you get zero warnings if you use wrong accessor to decode.

That's right, some attribute cary address specific content such
as addresses. By type safe I meant the API which no longer consists
of macros prone to errors. I didn't mean to say netlink attributes
are now type safe.

Re: Kernel header changes break glibc build

[David Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Wed, 6 Dec 2006 20:32:40 +0100 (MET)

> Am Montag, 4. Dezember 2006 10:13 schrieb Thomas Graf:
> 
> > I do not agree with the change to include if_addr.h in rtnetlink.h.
> > The point is to move bits apart and have multiple small pieces
> > of header files defining a specific rtnetlink family which are a
> > lot easier to maintain for both kernel and userspace than one giant
> > rtnetlink.h for everything.
> 
> According to a user's report, your change also broke compilation of my 
> dhcpclient because it neeeds if_addr.h since 2.6.19. Any suggestion how to 
> make one source code build on 2.6.19 and older headers? I hope you don't want 
> me to check on UTS_RELEASE in a userspace program?

That's enough for me.

Thomas we need to restore things to how they were before.
If that means including if_addr.h from rtnetlink.h so be it.

We can't break shit like this, there are no excuses, especially
now that we properly frob the headers for userspace consumption
in the kernel tree.

Before you hit the reply button, read me again, there are no excuses
for this breakage we've caused.  We must fix it now.

Thanks.

Re: Kernel header changes break glibc build

[Thomas Graf]

* David Miller <davem@davemloft.net> 2006-12-06 16:56
> That's enough for me.
> 
> Thomas we need to restore things to how they were before.
> If that means including if_addr.h from rtnetlink.h so be it.
> 
> We can't break shit like this, there are no excuses, especially
> now that we properly frob the headers for userspace consumption
> in the kernel tree.

So you want to restore IFLA_RTA/IFLA_PAYLOAD etc. as well?

Re: Kernel header changes break glibc build

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 7 Dec 2006 11:47:21 +0100

> * David Miller <davem@davemloft.net> 2006-12-06 16:56
> > That's enough for me.
> > 
> > Thomas we need to restore things to how they were before.
> > If that means including if_addr.h from rtnetlink.h so be it.
> > 
> > We can't break shit like this, there are no excuses, especially
> > now that we properly frob the headers for userspace consumption
> > in the kernel tree.
> 
> So you want to restore IFLA_RTA/IFLA_PAYLOAD etc. as well?

Just fixup the if_addr.h include for now.

[NETLINK]: Restore API compatibility of address and neighbour bits

[Thomas Graf]

Restore API compatibility due to bits moved from rtnetlink.h to
separate headers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6/include/linux/rtnetlink.h
===================================================================
--- net-2.6.orig/include/linux/rtnetlink.h	2006-12-07 11:25:29.000000000 +0100
+++ net-2.6/include/linux/rtnetlink.h	2006-12-07 11:32:25.000000000 +0100
@@ -3,6 +3,8 @@
 
 #include <linux/netlink.h>
 #include <linux/if_link.h>
+#include <linux/if_addr.h>
+#include <linux/neighbour.h>
 
 /****
  *		Routing/neighbour discovery messages.

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Woodhouse]

On Thu, 2006-12-07 at 11:55 +0100, Thomas Graf wrote:
> Restore API compatibility due to bits moved from rtnetlink.h to
> separate headers.

For 2.6.19.1 too, please.

> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Index: net-2.6/include/linux/rtnetlink.h
> ===================================================================
> --- net-2.6.orig/include/linux/rtnetlink.h	2006-12-07 11:25:29.000000000 +0100
> +++ net-2.6/include/linux/rtnetlink.h	2006-12-07 11:32:25.000000000 +0100
> @@ -3,6 +3,8 @@
>  
>  #include <linux/netlink.h>
>  #include <linux/if_link.h>
> +#include <linux/if_addr.h>
> +#include <linux/neighbour.h>
>  
>  /****
>   *		Routing/neighbour discovery messages.
> -
> 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
-- 
dwmw2

Re: Kernel header changes break glibc build

[David Woodhouse]

On Wed, 2006-12-06 at 15:23 +0100, Thomas Graf wrote:
> > I'm suggesting that if you want to change things around as you did, you
> > should make sure the users of those headers adapt to cope. You did fix
> > the in-kernel users; you neglected to fix glibc -- and as far as I can
> > tell you didn't even bother to _warn_ glibc folks.
> 
> I didn't warn them because I didn't know better. 

Fair enough. Now you do -- and thanks for fixing it.

> I was under the impression that glibc still maintains their own set of
> headers and will fix this automatically when they look at the diff.
> That's what I do for my userspace applications that use kernel
> headers.

That's never been the case for glibc. As I said, various people have
_attempted_ to do it, but it really isn't a workable approach and the
results were wildly inconsistent. Having a single, centrally-exported
set of headers is a massive improvement -- but yes, it does mean we have
to take a little care with what we're exporting to userspace.

> Ideally install_headers would do the trick but it often fails f.e.
> when some application which uses bsd features thus including net/if.h
> also wants to use new linux features and includes linux/if.h which
> then conflicts.

Applications in general shouldn't be doing that -- kernel headers are
for system libraries and tools only. We should try to ensure that the
_sensible_ use cases don't break, but we don't have to go overboard with
keeping our namespace clean and anticipating _every_ strange thing which
a userspace app might do with our headers -- we still get to say 'caveat
emptor'; it's just that we can't be _quite_ as haphazard about it as
we've been in the past.

-- 
dwmw2

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 7 Dec 2006 11:55:02 +0100

> Restore API compatibility due to bits moved from rtnetlink.h to
> separate headers.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Applied, thanks for taking care of this Thomas.

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 07 Dec 2006 11:28:28 +0000

> On Thu, 2006-12-07 at 11:55 +0100, Thomas Graf wrote:
> > Restore API compatibility due to bits moved from rtnetlink.h to
> > separate headers.
> 
> For 2.6.19.1 too, please.

I will, thanks for reminding me.

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Stefan Rompf]

Hi Thomas,

Am Donnerstag, 7. Dezember 2006 11:55 schrieb Thomas Graf:

> --- net-2.6.orig/include/linux/rtnetlink.h	2006-12-07 11:25:29.000000000
> +0100 +++ net-2.6/include/linux/rtnetlink.h	2006-12-07 11:32:25.000000000
> +0100 @@ -3,6 +3,8 @@
>
>  #include <linux/netlink.h>
>  #include <linux/if_link.h>
> +#include <linux/if_addr.h>
> +#include <linux/neighbour.h>
>
>  /****
>   *		Routing/neighbour discovery messages.

The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
compile, but there may be more macros than in the following patch. You may want
to look at keepalived, iproute, ...

Stefan

--- linux-2.6.19/include/linux/if_addr.h.orig	2006-12-08 14:08:29.000000000 +0100
+++ linux-2.6.19/include/linux/if_addr.h	2006-12-08 15:16:59.000000000 +0100
@@ -52,4 +52,12 @@
 	__u32	tstamp; /* updated timestamp, hundredths of seconds */
 };
 
+#ifndef __KERNEL__
+
+/* Userspace header compatibility */
+#define IFA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifaddrmsg))))
+#define IFA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifaddrmsg))
+
+#endif
+
 #endif
--- linux-2.6.19/include/linux/if_link.h.orig	2006-12-08 14:08:29.000000000 +0100
+++ linux-2.6.19/include/linux/if_link.h	2006-12-08 15:16:47.000000000 +0100
@@ -133,4 +133,12 @@
 	__u32	retrans_time;
 };
 
+#ifndef __KERNEL__
+
+/* Userspace header compatibility */
+#define IFLA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifinfomsg))))
+#define IFLA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifinfomsg))
+
+#endif
+
 #endif /* _LINUX_IF_LINK_H */

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Jim Gifford]

I have seen issues with some version of unifdef remove any instance of 
__KERNEL__, is the #ifndef __KERNEL__ really necessary. Has anyone 
tested to make sure the install_headers will not accidently remove this?

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Mike Frysinger]

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

On Friday 08 December 2006 12:33, Jim Gifford wrote:
> I have seen issues with some version of unifdef remove any instance of
> __KERNEL__

so use a version of unifdef that isnt broken ?
-mike

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Fri, 8 Dec 2006 15:25:18 +0100 (MET)

> The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
> restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
> compile, but there may be more macros than in the following patch. You may want
> to look at keepalived, iproute, ...

Those are not coming back, sorry.  They are really broken
and usage is extremely discouraged.

iproute got fixed, dhcpclient and friends should get fixed to not use
them either

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Daniel Jacobowitz]

On Fri, Dec 08, 2006 at 01:33:34PM -0800, David Miller wrote:
> From: Stefan Rompf <stefan@loplof.de>
> Date: Fri, 8 Dec 2006 15:25:18 +0100 (MET)
> 
> > The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
> > restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
> > compile, but there may be more macros than in the following patch. You may want
> > to look at keepalived, iproute, ...
> 
> Those are not coming back, sorry.  They are really broken
> and usage is extremely discouraged.
> 
> iproute got fixed, dhcpclient and friends should get fixed to not use
> them either

Does this mean glibc should also?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Fri, 8 Dec 2006 16:36:14 -0500

> On Fri, Dec 08, 2006 at 01:33:34PM -0800, David Miller wrote:
> > From: Stefan Rompf <stefan@loplof.de>
> > Date: Fri, 8 Dec 2006 15:25:18 +0100 (MET)
> > 
> > > The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
> > > restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
> > > compile, but there may be more macros than in the following patch. You may want
> > > to look at keepalived, iproute, ...
> > 
> > Those are not coming back, sorry.  They are really broken
> > and usage is extremely discouraged.
> > 
> > iproute got fixed, dhcpclient and friends should get fixed to not use
> > them either
> 
> Does this mean glibc should also?

GLIBC does not use them.

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Daniel Jacobowitz]

On Fri, Dec 08, 2006 at 01:47:52PM -0800, David Miller wrote:
> > > > The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
> > > > restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
> > > > compile, but there may be more macros than in the following patch. You may want
> > > > to look at keepalived, iproute, ...

> GLIBC does not use them.

Aren't these the ones you're talking about?

sysdeps/unix/sysv/linux/check_pf.c:                   struct rtattr *rta = IFA_RTA (ifam);
sysdeps/unix/sysv/linux/if_index.c:           struct rtattr *rta = IFLA_RTA (ifim);
sysdeps/unix/sysv/linux/ifaddrs.c:            struct rtattr *rta = IFLA_RTA (ifim);
sysdeps/unix/sysv/linux/ifaddrs.c:            struct rtattr *rta = IFLA_RTA (ifim);
sysdeps/unix/sysv/linux/ifaddrs.c:            struct rtattr *rta = IFA_RTA (ifam);

sysdeps/unix/sysv/linux/if_index.c:           size_t rtasize = IFLA_PAYLOAD (nlh);
sysdeps/unix/sysv/linux/if_index.c:               size_t rta_payload = RTA_PAYLOAD (rta);
sysdeps/unix/sysv/linux/ifaddrs.c:            size_t rtasize = IFLA_PAYLOAD (nlh);
sysdeps/unix/sysv/linux/ifaddrs.c:                size_t rta_payload = RTA_PAYLOAD (rta);
sysdeps/unix/sysv/linux/ifaddrs.c:            size_t rtasize = IFLA_PAYLOAD (nlh);
sysdeps/unix/sysv/linux/ifaddrs.c:                size_t rta_payload = RTA_PAYLOAD (rta);
sysdeps/unix/sysv/linux/ifaddrs.c:            size_t rtasize = IFA_PAYLOAD (nlh);
sysdeps/unix/sysv/linux/ifaddrs.c:                size_t rta_payload = RTA_PAYLOAD (rta);

-- 
Daniel Jacobowitz
CodeSourcery

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Fri, 8 Dec 2006 16:52:06 -0500

> On Fri, Dec 08, 2006 at 01:47:52PM -0800, David Miller wrote:
> > > > > The _RTA and _PAYLOAD-macros are also part of userspace headers and need to be
> > > > > restored. Both dhcpclient and quagga need at least IFA_RTA and IFLA_RTA to
> > > > > compile, but there may be more macros than in the following patch. You may want
> > > > > to look at keepalived, iproute, ...
> 
> > GLIBC does not use them.
> 
> Aren't these the ones you're talking about?
> 
> sysdeps/unix/sysv/linux/check_pf.c:                   struct rtattr *rta = IFA_RTA (ifam);

hohum... guess we'll need to bring back that crap too...

i'll take care of this

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 08 Dec 2006 16:43:45 -0800 (PST)

> hohum... guess we'll need to bring back that crap too...
> 
> i'll take care of this

Here's what I'll push into the tree.

commit c0279128f20aa3580b0b43aaa49f351f6bad5f30
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Fri Dec 8 17:05:13 2006 -0800

    [NETLINK]: Put {IFA,IFLA}_{RTA,PAYLOAD} macros back for userspace.
    
    GLIBC uses them etc.
    
    They are guarded by ifndef __KERNEL__ so nobody will start
    accidently using them in the kernel again, it's just for
    userspace.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/if_addr.h b/include/linux/if_addr.h
index dbe8f61..d557e4c 100644
--- a/include/linux/if_addr.h
+++ b/include/linux/if_addr.h
@@ -52,4 +52,10 @@ struct ifa_cacheinfo
 	__u32	tstamp; /* updated timestamp, hundredths of seconds */
 };
 
+/* backwards compatibility for userspace */
+#ifndef __KERNEL__
+#define IFA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifaddrmsg))))
+#define IFA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifaddrmsg))
+#endif
+
 #endif
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index e963a07..35ed3b5 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -82,6 +82,12 @@ #define IFLA_WEIGHT IFLA_WEIGHT
 
 #define IFLA_MAX (__IFLA_MAX - 1)
 
+/* backwards compatibility for userspace */
+#ifndef __KERNEL__
+#define IFLA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ifinfomsg))))
+#define IFLA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct ifinfomsg))
+#endif
+
 /* ifi_flags.
 
    IFF_* flags.

Re: [NETLINK]: Restore API compatibility of address and neighbour bits

[Stefan Rompf]

Am Freitag, 8. Dezember 2006 22:33 schrieb David Miller:

> [IF(L)A_(RTA|PAYLOAD) macros]
>
> iproute got fixed, dhcpclient and friends should get fixed to not use
> them either

Today's git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git 
contains stuff like

#ifndef IFA_RTA
#define IFA_RTA(r) \
        ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct 
ifaddrmsg))))
#endif

and still uses these macros in various places. I wouldn't call that fixed.

Even though the macros are back now, what is the suggested replacement for 
userspace programs? The new nla_get_*()-inlines in net/netlink.h are not even 
exported by make headers_install.

For new projects, I'll just use libnetlink. dhcpclient however is also 
targeted at embedded systems (and has been used to learn about netlink ;-), 
so I don't plan to link it to another not so small library.

Stefan

[NETLINK]: Schedule removal of old macros exported to userspace

[Thomas Graf]

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6/Documentation/feature-removal-schedule.txt
===================================================================
--- net-2.6.orig/Documentation/feature-removal-schedule.txt	2006-12-09 11:29:25.000000000 +0100
+++ net-2.6/Documentation/feature-removal-schedule.txt	2006-12-09 11:37:54.000000000 +0100
@@ -6,6 +6,18 @@ be removed from this file.
 
 ---------------------------
 
+What:	Netlink message and attribute parsing macros
+When:	July 2007
+Why:	The old interface which often lead to buggy code has been replaced
+	with a new type safe interface. Parts of this interface, mainly
+	macros, has been exported to userspace via linux/netlink.h and
+	linux/rtnetlink.h. Use of this interface is discontinued, all helper
+	and utility macros will be removed. Userspace applications should use
+	one of the available libraries.
+Who:	Thomas Graf <tgraf@suug.ch>
+
+---------------------------
+
 What:	/sys/devices/.../power/state
 	dev->power.power_state
 	dpm_runtime_{suspend,resume)()

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Stefan Rompf]

Am Samstag, 9. Dezember 2006 11:39 schrieb Thomas Graf:

[Added linux-kernel to CC]

> Index: net-2.6/Documentation/feature-removal-schedule.txt
> ===================================================================
> --- net-2.6.orig/Documentation/feature-removal-schedule.txt	2006-12-09

NAK.

> +What:  Netlink message and attribute parsing macros
> +When:  July 2007
> +Why:   The old interface which often lead to buggy code has been replaced
> +       with a new type safe interface. Parts of this interface, mainly
> +       macros, has been exported to userspace via linux/netlink.h and
> +       linux/rtnetlink.h. Use of this interface is discontinued, all
> helper +       and utility macros will be removed. Userspace applications
> should use +       one of the available libraries.
> +Who:   Thomas Graf <tgraf@suug.ch>

So glibc should be linked to libnl that depends on glibc to compile? Be 
serious!

I see a worrying tendency of kernel developers trying to push their 
stable-api-is-nonsense approach to userspace. You cannot just go ahead and 
remove userspace API that has been exported for years in a six month period. 
99,9% of application developers are not even aware that 
feature-removal-schedule.txt exists. Sorry, these macros will have to stay 
for *years*, even though they are ugly.

Btw, do you know why I didn't realize the breakage before a user alerted me? I 
stopped testing and running every new kernel. Reason? It was stated that 
2.6.18 requires a mandatory upgrade of udev bloat. Last time I needed to 
compile a new udev because of incompatible sysfs changes, it took me over 
three hours to get my notebook running again. Considering that I need to do 
actual money earning work on that system, 2.6.17.x runs nicely and has no new 
bugs that concern me, I just keep using it. Collateral damage.

You know, I'm not so happy with the in kernel stable-api-is-nonsense approach 
because it does create insecurity for developers and therefore bugs. Anyway, 
I accept it, I'm just a part time kernel hacker. But behaving towards 
applications developers this way is *deadly* for linux acceptance! Stuff like 
KDE, or a postgres database server, or whatever is complex enough that 
developers don't have time to follow userspace breakage introduced just 
because of ugly macros.

Stefan

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Thomas Graf]

* Stefan Rompf <stefan@loplof.de> 2006-12-09 12:49
> Am Samstag, 9. Dezember 2006 11:39 schrieb Thomas Graf:
> 
> [Added linux-kernel to CC]
> 
> > Index: net-2.6/Documentation/feature-removal-schedule.txt
> > ===================================================================
> > --- net-2.6.orig/Documentation/feature-removal-schedule.txt	2006-12-09
> 
> NAK.
> 
> > +What:  Netlink message and attribute parsing macros
> > +When:  July 2007
> > +Why:   The old interface which often lead to buggy code has been replaced
> > +       with a new type safe interface. Parts of this interface, mainly
> > +       macros, has been exported to userspace via linux/netlink.h and
> > +       linux/rtnetlink.h. Use of this interface is discontinued, all
> > helper +       and utility macros will be removed. Userspace applications
> > should use +       one of the available libraries.
> > +Who:   Thomas Graf <tgraf@suug.ch>
> 
> So glibc should be linked to libnl that depends on glibc to compile? Be 
> serious!

Please calm down a bit. I didn't claim that glibc should be linking to
libnl. glibc is obviously a special case which can simply copy the existing
macros into some internal compat header just like any other application
that doesn't wish to use any of the libraries available.

The point is to stop new applications from using the interface which has
resulted in buggy code in the past.

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Stefan Rompf]

Am Samstag, 9. Dezember 2006 13:55 schrieb Thomas Graf:

> Please calm down a bit. I didn't claim that glibc should be linking to
> libnl. glibc is obviously a special case which can simply copy the existing
> macros into some internal compat header just like any other application
> that doesn't wish to use any of the libraries available.

But when even glibc needs internal compat headers to compile with the second 
kernel version that provides userspace headers, what is the long-term - even 
mid-term - perspective of make headers_install then?

Stefan

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Sat, 9 Dec 2006 11:39:53 +0100

> +What:	Netlink message and attribute parsing macros
> +When:	July 2007
> +Why:	The old interface which often lead to buggy code has been replaced
> +	with a new type safe interface. Parts of this interface, mainly
> +	macros, has been exported to userspace via linux/netlink.h and
> +	linux/rtnetlink.h. Use of this interface is discontinued, all helper
> +	and utility macros will be removed. Userspace applications should use
> +	one of the available libraries.
> +Who:	Thomas Graf <tgraf@suug.ch>

You can't deprecate stuff visible to userspace, sorry Thomas,
we just can't do it.

You can migrate people to "better" interfaces, but you can't
pull the rug out from anyone once things like this are visible
to userspace.  It's permanently there, and we have to live with
that.

Once idea I have is that you could tag these things as
"deprecated" by making them use inline functions or similar
and adding the deprecated GCC attribute to them.  I'd be
very happy to include a patch like that.

This way userland gets the warning and people building it (and
in particular the developer) will see that they have something
to fix up.

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Sat, 9 Dec 2006 13:55:33 +0100

> The point is to stop new applications from using the interface which has
> resulted in buggy code in the past.

You don't get people to use new interface by breaking the
build on them in userspace.

You get them to do it by making suggestions and informing them, not
by forcing them.

That's why 1) you can't get rid of these macros, ever, but 2) you can
warn them by using inline functions and depcrecated attribute tags.

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[David Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Sat, 9 Dec 2006 15:58:01 +0100 (MET)

> But when even glibc needs internal compat headers to compile with the second 
> kernel version that provides userspace headers, what is the long-term - even 
> mid-term - perspective of make headers_install then?

Relax Stefan, I'll make sure these macros never go away.

They may emit warnings, via __attribute__((deprecated)), at
some point, but they will never be flat out removed.

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[David Woodhouse]

On Sat, 2006-12-09 at 15:58 +0100, Stefan Rompf wrote:
> But when even glibc needs internal compat headers to compile with the second 
> kernel version that provides userspace headers, what is the long-term - even 
> mid-term - perspective of make headers_install then?

We've only _just_ started paying attention to what we export. I tried to
keep the initial cut of headers_install very simple and uncontentious --
sticking to implementation, and not policy. So I didn't do a
particularly thorough cull of stuff that we shouldn't be exposing --
it's expected that we may lose _some_ more stuff.

But breaking userspace like _this_ isn't acceptable, and we're not doing
it.

-- 
dwmw2

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Thomas Graf]

* David Miller <davem@davemloft.net> 2006-12-09 13:45
> You can't deprecate stuff visible to userspace, sorry Thomas,
> we just can't do it.

It has been done before but I don't want to drag this on
any further. I did what I found to be right and obviously
I've hit a wall.

> Once idea I have is that you could tag these things as
> "deprecated" by making them use inline functions or similar
> and adding the deprecated GCC attribute to them.  I'd be
> very happy to include a patch like that.
> 
> This way userland gets the warning and people building it (and
> in particular the developer) will see that they have something
> to fix up.

I'm not going to spend any time on patching broken interfaces. I've
provided an alternative for those able and willing to switch, that's
as far as I'll go.

I had a short glance at dhcpclient's netlink code and found half
a dozen bugs within five minutes all caused by cut and pasting
sniplets from other netlink code. It is my opinion that people
will continue to drag along bugs this way unless the interface is
removed. I never believed in providing backward compatibility to
broken code.

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Jeff Bailey]

On 09/12/06, David Miller <davem@davemloft.net> wrote:
> You can't deprecate stuff visible to userspace, sorry Thomas,
> we just can't do it.
>
> You can migrate people to "better" interfaces, but you can't
> pull the rug out from anyone once things like this are visible
> to userspace.  It's permanently there, and we have to live with
> that.

Is there some way of marking it so that newer architectures that come
on don't wind up with that export?  That way at least as newer
machines and arches come out, they don't have to carry the oldest
baggage.  As the oldest architectures eventually get dropped, cruft
can be removed.

-- 
Jeff Bailey - http://www.raspberryginger.com/jbailey/

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Al Viro]

On Sat, Dec 09, 2006 at 08:42:45PM -0500, Jeff Bailey wrote:
> On 09/12/06, David Miller <davem@davemloft.net> wrote:
> >You can't deprecate stuff visible to userspace, sorry Thomas,
> >we just can't do it.
> >
> >You can migrate people to "better" interfaces, but you can't
> >pull the rug out from anyone once things like this are visible
> >to userspace.  It's permanently there, and we have to live with
> >that.
> 
> Is there some way of marking it so that newer architectures that come
> on don't wind up with that export?  That way at least as newer
> machines and arches come out, they don't have to carry the oldest
> baggage.  As the oldest architectures eventually get dropped, cruft
> can be removed.

Which architectures ever got dropped?

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Stefan Rompf]

Hi,

Am Sonntag, 10. Dezember 2006 00:28 schrieb Thomas Graf:

> I had a short glance at dhcpclient's netlink code and found half
> a dozen bugs within five minutes all caused by cut and pasting
> sniplets from other netlink code.

Please send me the list of bugs you've spotted. Of course I want to fix them.

> It is my opinion that people 
> will continue to drag along bugs this way unless the interface is
> removed. I never believed in providing backward compatibility to
> broken code.

My main problem with netlink was missing or wrong documentation (and yes, I 
know it's much easier writing code than writing docs, especially if OSS 
development is just a hobby). F.e. until 2.6.19 it has not been possible to 
query one ifi_index with RTM_GETLINK even though rfc3549 specified this 
operation. Instead, the application had to dump all interfaces and filter the 
record it is interested in. Stuff like this creates much more bugs than a 
non-typesafe macro because it makes you think "Damn it's working now, never 
touch that code again".

Don't take this as an offense, I do value your work and will use libnl for 
future projects, but I definitely do not share your opinion on how to deal 
with ugly/obsolete userspace interfaces (even though this one is not 
obsolete, because it is the only low level interface that exists at all).

Stefan

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[Thomas Graf]

* Stefan Rompf <stefan@loplof.de> 2006-12-10 11:11
> Please send me the list of bugs you've spotted. Of course I want to fix them.

Sure...

static void nl_handlemsg(struct nlmsghdr *msg, unsigned int len) {
  if (len < sizeof(*msg)) return;

  while(NLMSG_OK(msg,len)) {
    if (nlcb_run &&
	nlcb_pid == msg->nlmsg_pid &&
	nlcb_seq == msg->nlmsg_seq) {
      nlcb_function(msg, nlcb_args);

Missing check for sufficient payload, family specific header
and attributes are accessed directly, you've only made sure
a netlink message header is present so far.

static void copy_ifdata(struct nlmsghdr *msg, void **args) {
  struct dhcp_interface *dhcpif = args[0];
  struct ifinfomsg *ifinfo = NLMSG_DATA(msg);
  struct rtattr *rta = IFLA_RTA(ifinfo);
  int len = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifinfo));

This is also wrong, it's lacking NLMSG_ALIGN() to consider
the padding between the family specific header and the
attributes, you'd assume the padding to be attributes and
access memory beyond the message.

  if (msg->nlmsg_type != RTM_NEWLINK) return;
  if (dhcpif->ifidx) return;

  for(; RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
    switch(rta->rta_type) {
    case IFLA_IFNAME:
      if (!strcmp(dhcpif->ifname, (char *)RTA_DATA(rta)))

Payload length not checked or a horrible crash if payload is
not NUL terminated.

	dhcpif->ifidx = ifinfo->ifi_index;
      break;
    case IFLA_ADDRESS:
      memcpy(dhcpif->ifhwaddr, RTA_DATA(rta), 6); /* FIXME */
      break;

Even worse, IFLA_ADDRESS may be of arbitary length up to
MAX_ADDR_LEN.

Basically the slightest malformed netlink message or even semantic
changes within the legal boundries will simply crash your application.

> My main problem with netlink was missing or wrong documentation (and yes, I 
> know it's much easier writing code than writing docs, especially if OSS 
> development is just a hobby).

Before accusing it would have been nice to do some research and you may
have found a lot of netlink documentation inside the libnl documentation.

> F.e. until 2.6.19 it has not been possible to 
> query one ifi_index with RTM_GETLINK even though rfc3549 specified this 
> operation.

It was available through wireless extensions and there is also a
AF_INET6 specific variation providing even more details. It wasn't
implemented for AF_UNSPEC before because, I believe, Alexey was just
too lazy to do it and used the bsd interface for this purpose in
iproute2 which is based on ioctl :-) I made rtnl_getlink() available
because I agree its useful but you've been preaching compatibility so
much I really can't imagine it being useful to you as it would make
your appliaction depend on >= 2.6.19.

> Instead, the application had to dump all interfaces and filter the 
> record it is interested in. Stuff like this creates much more bugs than a 
> non-typesafe macro because it makes you think "Damn it's working now, never 
> touch that code again".

Right... Would also have been an option to use the perfectly documented
interface and save a lot of sweat: (From libnl documentation)

1) Retrieving information about available links

 // The first step is to retrieve a list of all available interfaces within
 // the kernel and put them into a cache.
 struct nl_cache *cache = rtnl_link_alloc_cache(nl_handle);

 // In a second step, a specific link may be looked up by either interface
 // index or interface name.
 struct rtnl_link *link = rtnl_link_get_by_name(cache, "lo");

 // rtnl_link_get_by_name() is the short version for translating the
 // interface name to an interface index first like this:
 int ifindex = rtnl_link_name2i(cache, "lo");
 struct rtnl_link *link = rtnl_link_get(cache, ifindex);

 // After successful usage, the object must be given back to the cache
 rtnl_link_put(link);

> Don't take this as an offense, I do value your work and will use libnl for 
> future projects, but I definitely do not share your opinion on how to deal 
> with ugly/obsolete userspace interfaces (even though this one is not 
> obsolete, because it is the only low level interface that exists at all).

That's just another inaccurate statement, libnl provides a low level interface
very similiar to libnetlink. On top of that it also implements the actual
netlink families to avoid every application having to write their own protocol
parser.

I see your point in libnl being expensive in terms of size, its modular design
would have made it trivial though to ship all but the core as loadable modules
and have embeded systems omit the modules it doesn't require such as traffic
control or even routing.

dhcpclient netlink bugs (was Re: [NETLINK]: Schedule removal of old macros exported to userspace)

[Stefan Rompf]

Am Sonntag, 10. Dezember 2006 13:15 schrieb Thomas Graf:

> > Please send me the list of bugs you've spotted. Of course I want to fix
> > them.
>
> Sure...
>
> static void nl_handlemsg(struct nlmsghdr *msg, unsigned int len) {
>   if (len < sizeof(*msg)) return;
>
>   while(NLMSG_OK(msg,len)) {
>     if (nlcb_run &&
> 	nlcb_pid == msg->nlmsg_pid &&
> 	nlcb_seq == msg->nlmsg_seq) {
>       nlcb_function(msg, nlcb_args);
>
> Missing check for sufficient payload, family specific header
> and attributes are accessed directly, you've only made sure
> a netlink message header is present so far.

Yes, the code has quite some trust into the kernel that if it answers the 
asked question the answer is semantically correct. But to be fair, if you 
issue a write(), you also expect the number of bytes written in return and 
not the msec taken ;-) Will fix that and the other stuff you pointed out, 
thanks!

Stefan

Re: [NETLINK]: Schedule removal of old macros exported to userspace

[David Woodhouse]

On Sat, 2006-12-09 at 15:58 +0100, Stefan Rompf wrote:
> But when even glibc needs internal compat headers to compile with the second 
> kernel version that provides userspace headers, what is the long-term - even 
> mid-term - perspective of make headers_install then?

Bear in mind that with the first cut of the headers_install I was trying
to keep it entirely uncontentious and stick to _mechanism_, not policy.
The _first_ set of exported headers still aren't ideal, and we're still
cleaning up some parts of them (like properly getting rid of the
_syscallX() macros, etc.). 

We've only just started to be sensible about what we export to userspace
-- it's not entirely set in stone at this point, 
 
-- 
dwmw2

Re: dhcpclient netlink bugs (was Re: [NETLINK]: Schedule removal of old macros exported to userspace)

[Herbert Xu]

Stefan Rompf <stefan@loplof.de> wrote:
>
> Yes, the code has quite some trust into the kernel that if it answers the 
> asked question the answer is semantically correct. But to be fair, if you 
> issue a write(), you also expect the number of bytes written in return and 
> not the msec taken ;-) Will fix that and the other stuff you pointed out, 
> thanks!

I hope you checked that the message is really from the kernel (based on
saddr).  Unconnected sockets can receive messages from any user on the
host.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt