cvs commit: src UPDATING src/sys/sys param.h src/sys/net if.c if.h

[Peter Wemm]

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.
-- 
Peter Wemm - peter at wemm.org; peter at FreeBSD.org; peter at yahoo-inc.com
"All of this is for nothing if we don't go to the stars" - JMS/B5