Date: Tue, 31 Aug 2004 19:11:41 -0700 From: Peter Wemm <peter@wemm.org> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src UPDATING src/sys/sys param.h src/sys/net if.c if.h Message-ID: <200408311911.41020.peter@wemm.org> In-Reply-To: <200408311821.09991.peter@wemm.org> References: <200408300629.i7U6TQ5C088279@repoman.freebsd.org> <20040901010241.GC25779@odin.ac.hmc.edu> <200408311821.09991.peter@wemm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 31 August 2004 06:21 pm, Peter Wemm wrote:
> On Tuesday 31 August 2004 06:02 pm, Brooks Davis wrote:
> > On Tue, Aug 31, 2004 at 05:46:50PM -0700, Peter Wemm wrote:
> > > On Monday 30 August 2004 05:43 pm, Brooks Davis wrote:
> > > > On Mon, Aug 30, 2004 at 05:29:22PM -0700, Peter Wemm wrote:
> > > > > On Monday 30 August 2004 02:53 pm, Brooks Davis wrote:
> > > > > > On Mon, Aug 30, 2004 at 05:34:23PM -0400, Andrew Gallatin
>
> wrote:
> > > > > > > Brooks Davis writes:
> > > > > > > > I don't think so. The main offenders will be programs
> > > > > > > > that walk the interface list via libkvm and programs
> > > > > > > > that use struct if_data. ifconfig does neither. If
> > > > > > > > you haven't recompiled a module, a non-working
> > > > > > > > ifconfig might result if by some miracle you didn't
> > > > > > > > get a panic on attach.
> > > > > > >
> > > > > > > ppc does not even build modules, so that's out. An older
> > > > > > > ifconfig pukes, while a new one works, so some sort of
> > > > > > > kernel/userland ABI must have changed in the last week.
> > > > > > > Has the data exported by sysctl changed recently?
> > > > > >
> > > > > > I think I found it, at least partially. The issue is
> > > > > > struct if_msghdr which is used by ifconfig. It contains a
> > > > > > struct if_data. It has grown by a struct timeval. The
> > > > > > weird thing is that this shouldn't be a problem becuase
> > > > > > if_msghdr has a length and my addition was to the end so
> > > > > > ifconfig should be able to skip over it, but it doesn't
> > > > > > seem to actually work.
> > > > >
> > > > > It is broken on amd64 too, FWIW.
> > > >
> > > > If if_msghdr is the problem, it will be broken on all platforms
> > > > and a rebuild of everything thing should fix it. I don't know
> > > > why ifconfig is having a problem with the extension of
> > > > if_msghdr. It shouldn't care since the format is unchanged
> > > > other then the addition of a structure at the very end and the
> > > > ifm_msglen member appears to be used correct to advance through
> > > > the array passed by the sysctl.
> > >
> > > Actually, I more want to know why it didn't work. But I haven't
> > > figured out where the message lengths are even getting set, let
> > > alone (mis?)used by ifconfig.
> >
> > The if_msghdrs are used in the loop that starts around line 582 in
> > ifconfig.c. That seems to correct. The length seems to be set in
> > in net/rtsock.c in the rt_msg1 and rt_msg2 functions. The reason
> > it's so darn hard to find is that routing messages are defined by
> > different structs with the same first several members. Due to the
> > naming of the members, that renders grep useless. :-( I can't help
> > but think someone was too clever for their own good. I still don't
> > see why it's breaking though since that should set the values. The
> > only thing I can think of is that there's a dependency tracking bug
> > somewhere, but that seems unlikely which would seem to indicate
> > that I'm not on to the real cause yet. :-(
>
> Found it.. The routing socket ABI is broken. eg:
> ...
> if (ifm->ifm_type == RTM_IFINFO) {
> sdl = (struct sockaddr_dl *)(ifm + 1);
> flags = ifm->ifm_flags;
> ...
> /* Expand the compacted addresses */
> rt_xaddrs((char *)(ifam + 1), ifam->ifam_msglen +
> (char *)ifam,
> &info);
> ...
>
> The +1 trick encodes the sizeof the structure into the code. The
> problem is that certain records have additional data appended but
> without recording the offset.
>
> eg:
> struct if_msghdr {
> u_short ifm_msglen; /* to skip over non-understood
> messages */
> u_char ifm_version; /* future binary compatibility */
> u_char ifm_type; /* message type */
> int ifm_addrs; /* like rtm_addrs */
> int ifm_flags; /* value of if_flags */
> u_short ifm_index; /* index for associated ifp */
> struct if_data ifm_data;/* statistics and other data about
> if */
> };
>
> The problem is that you extended the size of if_data, but there is no
> way to represent that..
> Suppose the structure was 0x200 bytes. Suppose there was 0x80
> additional bytes. It would occupy offsets 0x200 through 0x280.
> ifm_msglen would be 0x280.
>
> However. Now that the base structure has changed, the additional
> data starts now at 0x220 and the msglen is 0x2a0. Old binaries have
> no way to find out where the (variable length) additional data
> begins. You can't find the cutover point when you have two unknown
> variable length data blobs. (You could use sizeof vs msglen to
> calculate it if you knew a fixed length additional data item)
>
> So, we're hosed. struct if_data either can't be extended, or we have
> to encode an additional data offset pointer or the size of the
> ifm_data.
>
> if_data has a hidden (due to packing) u_char space avalable at the
> beginning for a length value, and there is also ifi_unused near the
> end.
>
> An embedded length in the if_data struct is valueable so we can
> extend it in the future again.
>
> We could encode 'u_char ifi_datalen' for free without changing the
> abi again. ifi_data is roughly 168 bytes on a 64 bit machine, I
> don't imagine too many extra stats to keep. Or we could re-use
> ifi_unused.
>
> Let me whip up a patch.
This seems to work for me. Of course, for it to be really useful, it
would have to be in all our old ifconfig binaries, but it would be nice
if it could go into 5.x. That would make the 5.x binaries work again
when booted with a 6.x kernel. Thats going to be important for a
while.
Note again that this doesn't break anything else. It just uses a
hidden (due to alignment) variable and has a best-effort heuristic
for old kernels.
==== //depot/projects/hammer/sbin/ifconfig/ifconfig.c#18 - /home/src/sbin/ifconfig/ifconfig.c ====
@@ -585,7 +585,10 @@
ifm = (struct if_msghdr *)next;
if (ifm->ifm_type == RTM_IFINFO) {
- sdl = (struct sockaddr_dl *)(ifm + 1);
+ if (ifm->ifm_data.ifi_datalen == 0)
+ ifm->ifm_data.ifi_datalen = sizeof(struct if_data);
+ sdl = (struct sockaddr_dl *)((char *)ifm + sizeof(struct if_msghdr) -
+ sizeof(struct if_data) + ifm->ifm_data.ifi_datalen);
flags = ifm->ifm_flags;
} else {
fprintf(stderr, "out of sync parsing NET_RT_IFLIST\n");
==== //depot/projects/hammer/sys/net/if.c#26 - /home/src/sys/net/if.c ====
@@ -399,6 +399,7 @@
if_index = ifp->if_index;
if (if_index >= if_indexlim)
if_grow();
+ ifp->if_data.ifi_datalen = sizeof(struct if_data);
ifnet_byindex(ifp->if_index) = ifp;
ifdev_byindex(ifp->if_index) = make_dev(&net_cdevsw,
==== //depot/projects/hammer/sys/net/if.h#12 - /home/src/sys/net/if.h ====
@@ -85,6 +85,7 @@
u_char ifi_link_state; /* current link state */
u_char ifi_recvquota; /* polling quota for receive intrs */
u_char ifi_xmitquota; /* polling quota for xmit intrs */
+ u_char ifi_datalen; /* length of this data struct */
u_long ifi_mtu; /* maximum transmission unit */
u_long ifi_metric; /* routing metric (external only) */
u_long ifi_baudrate; /* linespeed */
--
Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com
"All of this is for nothing if we don't go to the stars" - JMS/B5
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200408311911.41020.peter>