dhcpd

[Greg Troxel]

[dhclient fails with sys/net/if.c 1.200]

I am also seeing trouble with dhclient. The SIOCGIFCONF interface is
tricky, and it may be that moving to struct sockaddr_storage results in
different interpretations of what the API should be. racoon works fine
with the new version.

It's not surprising both dhclient and dhcpd fail; they use the same
support code.

Can anyone run them under gdb and see how they cope with parsing the
results of SIOCGIFCONF?

But, I can see the problem; dhclient checks the size of the sockaddr in
ifreq against struct sockaddr, not against sizeof(ifreq).

There's also another bug, in that the 'was the buffer big enough check'
is unreliable. But that's not what's causing trouble now for most
people.


So, this comes down to whether the API for struct ifreq was

size is determined by sa_len if size is > struct sockaddr

or

size is determined by sa_len if size is > (union in ifreq for
sockaddrs, and hence the sockaddr actually needs to extend beyond
ifreq)

racoon assumed the latter. The former is crazy - it means that an IPv4
address results in ifreq spacing of struct ifreq (144 bytes), because
sockaddr_in fits in sockaddr. But sockaddr_dl or sockaddr_in6 is
bigger, and would result in *smaller* struct ifreq, exactly being big
enough.

The real problem is that SIOCGIFCONF never had a real API definition.

I think the right fix is to change dhcp to use sizeof (ifreq.ifr_ifru)
instead of sockaddr for deciding if more space is needed. This declares
that we follow the 2nd choice of what the API used to be.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

Post by Greg Troxel
[dhclient fails with sys/net/if.c 1.200]
The real problem is that SIOCGIFCONF never had a real API definition.
I think the right fix is to change dhcp to use sizeof (ifreq.ifr_ifru)
instead of sockaddr for deciding if more space is needed. This declares
that we follow the 2nd choice of what the API used to be.

Is it possible to fix this somehow without breaking binary compatibility?

The real incompatible change is in -current but not 4, and -current has
binary compat with the old ioctl of smaller size. Binary compatibility
with the old version of SIOCGIFCONF in currrent, with the larger size
and the bizarre size-dependent behavior doesn't make sense, and would
cause the other half of the programs to fail (well, right now we have
dhcp vs racoon and I don't know about others).

So only people running current will have trouble, and if we fix dhcp,
then once they update both kernel/user all will be ok. It seems we
don't do intra-current binary compat usually, especially when fixing bugs.



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Simon Burge]

Post by Greg Troxel

Is it possible to fix this somehow without breaking binary compatibility?

The real incompatible change is in -current but not 4, and -current has
binary compat with the old ioctl of smaller size. Binary compatibility
with the old version of SIOCGIFCONF in currrent, with the larger size
and the bizarre size-dependent behavior doesn't make sense, and would
cause the other half of the programs to fail (well, right now we have
dhcp vs racoon and I don't know about others).
So only people running current will have trouble, and if we fix dhcp,
then once they update both kernel/user all will be ok. It seems we
don't do intra-current binary compat usually, especially when fixing bugs.

Ok, that falls into more the annoyance catergory rather than the seriously
hurts catergory (for me at least!) for the binary compat part.

One other thing I'm unclear about is whether dhcpd/dhclient just needed
a recompile or source code changes? If the latter, is this going to
be something that'll bite us in pkgsrc, where we on at least the last
release version as well. Ie, if we have a package that uses SIOCGIFCONF
will it need some #ifdef ugliness to work with various versions of
NetBSD?

Cheers,
Simon.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Tom Spindler]

Post by Greg Troxel
The real incompatible change is in -current but not 4, and -current has
binary compat with the old ioctl of smaller size. Binary compatibility
with the old version of SIOCGIFCONF in currrent, with the larger size
and the bizarre size-dependent behavior doesn't make sense, and would
cause the other half of the programs to fail (well, right now we have
dhcp vs racoon and I don't know about others).

If this is the case, shouldn't the SIOCGIFCONF ioctl number be bumped?


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Tom Spindler]

It was bumped when sockaddr_storage was added, so it's different in 4
and current. Before 1.200, SIOCGIFCONF was broken in current. (Dhcpd
was and still is broken, but now the latent bug is causing trouble - but
I realize that this is overly harsh because the interface was never well
defined.)
Are you suggesting that we have binary compat among versions of current,
even for bugfixes that cause trouble?

Nope; I hadn't realized that the ioctl was bumped at all. In this case,
I think it's reasonable for people to recompile and Just Deal.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Hisashi T Fujinaka]

If anyone wants to try this change, I think it will fix dhcp, but I
haven't even compiled it. (The code looks like it has more problems -
lending weight to the 'all users of SIOCGIFCONF shoudl be converted to
getifaddrs.) The if being changed will never be taken in -current.
Index: dist/dhcp/common/discover.c
===================================================================
RCS file: /cvsroot/src/dist/dhcp/common/discover.c,v
retrieving revision 1.9
diff -u -p -r1.9 discover.c
--- dist/dhcp/common/discover.c 31 May 2007 02:58:10 -0000 1.9
+++ dist/dhcp/common/discover.c 13 Sep 2007 00:07:07 -0000
@@ -235,7 +235,7 @@ void discover_interfaces (state)
memcpy(&ifcpy, (caddr_t)ic.ifc_req + i, sizeof(struct ifreq));
#ifdef HAVE_SA_LEN
- if (ifp -> ifr_addr.sa_len > sizeof (struct sockaddr)) {
+ if (ifp -> ifr_addr.sa_len > sizeof (ifp.ifr_ifru)) {
if (sizeof(struct ifreq) + ifp->ifr_addr.sa_len >
sizeof(ifcpy))
break;

Hmm, what I get is:
# compile common/discover.o
/usr/src/obj.i386/tooldir.NetBSD-4.99.31-i386/bin/i386--netbsdelf-gcc -O2 -Wall -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-sign-compare -Wno-traditional -Werror -fno-strict-aliasing -fstack-protector -Wstack-protector --param ssp-buffer-size=1 -I/usr/src/dist/dhcp -I/usr/src/dist/dhcp/includes -Wno-unused -D_FORTIFY_SOURCE=2 -nostdinc -isystem /usr/include -c /usr/src/dist/dhcp/common/discover.c -o discover.o
/usr/src/dist/dhcp/common/discover.c: In function 'discover_interfaces':
/usr/src/dist/dhcp/common/discover.c:238: error: request for member 'ifr_ifru' in something not a structure or union
*** Error code 1

[Greg Troxel]

sorry, that should be -> (I was making changes too late at night)

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

I have committed a fix to dhcpd. Unfortunately it's not possible to
change the kernel to make all the previous users of SIOCGIFCONF work
correctly given the change to sockaddr_storage in struct ifreq, and I
chose the sane interpretation.

From: Greg Troxel <***@netbsd.org>
Subject: CVS commit: src/dist/dhcp/common
To: source-***@NetBSD.org
Date: Thu, 13 Sep 2007 11:56:42 +0000 (UTC)
Reply-To: ***@netbsd.org


Module Name: src
Committed By: gdt
Date: Thu Sep 13 11:56:41 UTC 2007

Modified Files:
src/dist/dhcp/common: discover.c

Log Message:
Follow NetBSD's interpretation of the interface to SIOCGIFCONF: the
next ifreq is sizeof(struct ifreq) after the current one unless the
sockaddr is bigger than the union in ifreq that holds it.

In the original 4.4BSD code, this interpretation results in the same
behavior as the "is the sockaddr bigger than struct sockaddr", because
sizeof(struct sockaddr) and sizeof(ifc->ifr_ifru) are the same.

Add comments pointing out problems in the 'need bigger buffer' code,
and copying excessive amounts of data.


To generate a diff of this commit:
cvs rdiff -r1.9 -r1.10 src/dist/dhcp/common/discover.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Nicolas Joly]

If anyone wants to try this change, I think it will fix dhcp, but I
haven't even compiled it. (The code looks like it has more problems -
lending weight to the 'all users of SIOCGIFCONF shoudl be converted to
getifaddrs.) The if being changed will never be taken in -current.

[...]

I tried
if (ifp -> ifr_addr.sa_len > sizeof (ifp -> ifr_ifru)) {
as you suggest, and all is well!

It work better ... but this does not seems to be enough.

The MAC address used by dhclient is wrong :

bge0 at pci2 dev 9 function 0: Broadcom BCM5703X Gigabit Ethernet
bge0: interrupting at ioapic1 pin 0 (irq 11)
bge0: ASIC BCM5703 A2 (0x1002), Ethernet address 00:e0:81:31:59:61
brgphy0 at bge0 phy 1: BCM5703 1000BASE-T media interface, rev. 2
brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FDX, auto

Listening on BPF/bge0/00:e0:81:31:fa:14
Sending on BPF/bge0/00:e0:81:31:fa:14
Sending on Socket/fallback
DHCPREQUEST on bge0 to 255.255.255.255 port 67
DHCPREQUEST on bge0 to 255.255.255.255 port 67
DHCPREQUEST on bge0 to 255.255.255.255 port 67
DHCPDISCOVER on bge0 to 255.255.255.255 port 67 interval 7
DHCPDISCOVER on bge0 to 255.255.255.255 port 67 interval 13
DHCPDISCOVER on bge0 to 255.255.255.255 port 67 interval 13
DHCPDISCOVER on bge0 to 255.255.255.255 port 67 interval 20
DHCPDISCOVER on bge0 to 255.255.255.255 port 67 interval 8
No DHCPOFFERS received.
Trying recorded lease 157.99.xx.xx

[Patrick Welche]

Post by Nicolas Joly

as you suggest, and all is well!

OK - dhcpd is well - and serving lots of requests now... I didn't try
dhclient!

Post by Nicolas Joly
It work better ... but this does not seems to be enough.
bge0: ASIC BCM5703 A2 (0x1002), Ethernet address 00:e0:81:31:59:61
Listening on BPF/bge0/00:e0:81:31:fa:14
Sending on BPF/bge0/00:e0:81:31:fa:14

that is remarkable 5961 -> FA14 101100101100001 -> 1111101000010100
no similarity at all...

Cheers,

Patrick

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Blair Sadewitz]

I can confirm this problem with wm as well using sources from 9/12. I
saw the commit in common/dhcp; is this fixed now?

Regards,

--Blair

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

In src/dist/dhcp/common/discover.c:discover(), try changing buf from
2048 to 32768. I suspect that now that each ifreq has a
sockaddr_storage that all your interfaces don't fit in 2048. The
resize/retry logic is broken, and always has been, but it used to not
usually matter because 50 interfaces fit.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

I looked at the ifconf() code and now think that the kernel wasn't
copying the whole sockaddr. I have compile tested but not run the
following patch, which I think will fix the 'missing two bytes' of the
mac address problem, as well as unbreak v6 addresses.

Index: sys/net/if.c
===================================================================
RCS file: /cvsroot/src/sys/net/if.c,v
retrieving revision 1.200
diff -u -p -r1.200 if.c
--- sys/net/if.c 11 Sep 2007 19:31:22 -0000 1.200
+++ sys/net/if.c 13 Sep 2007 18:28:48 -0000
@@ -1669,7 +1669,7 @@ ifconf(u_long cmd, void *data)

if (ifrp != NULL)
{
- ifr.ifr_addr = *sa;
+ memcpy(&ifr.ifr_space, sa, sa->sa_len);
if (space >= sz) {
error = copyout(&ifr, ifrp, sz);
if (error != 0)
Index: sys/net/if.h
===================================================================
RCS file: /cvsroot/src/sys/net/if.h,v
retrieving revision 1.126
diff -u -p -r1.126 if.h
--- sys/net/if.h 2 Sep 2007 00:24:18 -0000 1.126
+++ sys/net/if.h 13 Sep 2007 18:28:48 -0000
@@ -552,6 +552,7 @@ struct ifreq {
#define ifr_addr ifr_ifru.ifru_addr /* address */
#define ifr_dstaddr ifr_ifru.ifru_dstaddr /* other end of p-to-p link */
#define ifr_broadaddr ifr_ifru.ifru_broadaddr /* broadcast address */
+#define ifr_space ifr_ifru.ifru_space /* sockaddr_storage */
#define ifr_flags ifr_ifru.ifru_flags /* flags */
#define ifr_metric ifr_ifru.ifru_metric /* metric */
#define ifr_mtu ifr_ifru.ifru_mtu /* mtu */

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Greg Troxel
I looked at the ifconf() code and now think that the kernel wasn't
copying the whole sockaddr. I have compile tested but not run the
following patch, which I think will fix the 'missing two bytes' of the
mac address problem, as well as unbreak v6 addresses.
Index: sys/net/if.c
===================================================================
RCS file: /cvsroot/src/sys/net/if.c,v
retrieving revision 1.200
diff -u -p -r1.200 if.c
--- sys/net/if.c 11 Sep 2007 19:31:22 -0000 1.200
+++ sys/net/if.c 13 Sep 2007 18:28:48 -0000
@@ -1669,7 +1669,7 @@ ifconf(u_long cmd, void *data)
if (ifrp != NULL)
{
- ifr.ifr_addr = *sa;
+ memcpy(&ifr.ifr_space, sa, sa->sa_len);

I think that this should be MIN(sa->sa_len, sizeof(ifr.ifr_space)) just
to be paranoid... Or even better:

_KASSERT(sa->sa_len < sizeof(ifr.ifr_space));

before the memcpy...

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

***@astron.com (Christos Zoulas) writes:

[dropping current-users; we're polishing now]

Post by Christos Zoulas

Post by Greg Troxel
I looked at the ifconf() code and now think that the kernel wasn't
copying the whole sockaddr. I have compile tested but not run the
following patch, which I think will fix the 'missing two bytes' of the
mac address problem, as well as unbreak v6 addresses.
Index: sys/net/if.c
===================================================================
RCS file: /cvsroot/src/sys/net/if.c,v
retrieving revision 1.200
diff -u -p -r1.200 if.c
--- sys/net/if.c 11 Sep 2007 19:31:22 -0000 1.200
+++ sys/net/if.c 13 Sep 2007 18:28:48 -0000
@@ -1669,7 +1669,7 @@ ifconf(u_long cmd, void *data)
if (ifrp != NULL)
{
- ifr.ifr_addr = *sa;
+ memcpy(&ifr.ifr_space, sa, sa->sa_len);

I think that this should be MIN(sa->sa_len, sizeof(ifr.ifr_space)) just
_KASSERT(sa->sa_len < sizeof(ifr.ifr_space));
before the memcpy...
christos

I think so too, but the whole loop has:

TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
struct sockaddr *sa = ifa->ifa_addr;
/* all sockaddrs must fit in sockaddr_storage */
KASSERT(sa->sa_len <= sizeof(ifr.ifr_ifru));

if (ifrp != NULL)
{
memcpy(&ifr.ifr_space, sa, sa->sa_len);
if (space >= sz) {
error = copyout(&ifr, ifrp, sz);
if (error != 0)
return (error);
ifrp++; space -= sz;
}
}
else
space += sz;
}

So perhaps you could argue that the KASSERT only belongs in the if branch.

I used the union, not space, because that's the real destination of the
copy. But they should be the same.

Why < vs <= ?

Is _KASSERT just a typeo for KASSERT?






--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

On Sep 14, 7:29pm, ***@ir.bbn.com (Greg Troxel) wrote:
-- Subject: Re: dhcpd

| I think so too, but the whole loop has:
|
| TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
| struct sockaddr *sa = ifa->ifa_addr;
| /* all sockaddrs must fit in sockaddr_storage */
| KASSERT(sa->sa_len <= sizeof(ifr.ifr_ifru));
|
| if (ifrp != NULL)
| {
| memcpy(&ifr.ifr_space, sa, sa->sa_len);
| if (space >= sz) {
| error = copyout(&ifr, ifrp, sz);
| if (error != 0)
| return (error);
| ifrp++; space -= sz;
| }
| }
| else
| space += sz;
| }
|
| So perhaps you could argue that the KASSERT only belongs in the if branch.

Either way is fine.

| I used the union, not space, because that's the real destination of the
| copy. But they should be the same.

fine.

|
| Why < vs <= ?

<= is correct.

| Is _KASSERT just a typeo for KASSERT?

Right. Thanks for taking care of this!

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de