[PATCH] Deletion of route scrubs ifa flag IFA_ROUTE even if it's not the automatic route

[David Young]

To make ammends, attached is a patch to fix this. It is intended for
pullup to netbsd-5.

And here is a different patch.
We add a new function, rt_ifa_connected which returns 1 if the route is
the connected route (prefix, subnet, etc) for the ifa.
This makes the code in rtsock.c and route.c a lot simpler. I've also
added some debug messages via the existing RT_DPRINTF define.
For reference, I've also added a patch which applies this before my
origonal patch was comitted to show the intent of what we're trying to
do here.
I think this pretty much covers everything now. Anyone have any issues
with this before I commit?

Please separate the cosmetic changes from the functional changes
before you commit.

Avoid accessing _rt_key except by using the accessor, rt_getkey().

Perhaps the following code logically belongs in rt_replace_ifa()?

+ if (oifa && oifa->ifa_flags & IFA_ROUTE &&
+ rt_ifa_connected(rt, oifa))
+ {
+ RT_DPRINTF("rt->_rt_key = %p, "
+ "change deleted IFA_ROUTE\n",
+ (void *)rt->_rt_key);
+ oifa->ifa_flags &= ~IFA_ROUTE;
+ if (rt_ifa_connected(rt, ifa)) {
+ RT_DPRINTF("rt->_rt_key = %p, "
+ "change added IFA_ROUTE\n",
+ (void *)rt->_rt_key);
ifa->ifa_flags |= IFA_ROUTE;

Dave

[Roy Marples]

Post by David Young

To make ammends, attached is a patch to fix this. It is intended for
pullup to netbsd-5.

And here is a different patch.
We add a new function, rt_ifa_connected which returns 1 if the route is
the connected route (prefix, subnet, etc) for the ifa.
This makes the code in rtsock.c and route.c a lot simpler. I've also
added some debug messages via the existing RT_DPRINTF define.
For reference, I've also added a patch which applies this before my
origonal patch was comitted to show the intent of what we're trying to
do here.
I think this pretty much covers everything now. Anyone have any issues
with this before I commit?

Please separate the cosmetic changes from the functional changes
before you commit.
Avoid accessing _rt_key except by using the accessor, rt_getkey().
Perhaps the following code logically belongs in rt_replace_ifa()?

Yes, it makes more sense there.

I will commit these patches tomorrow one after the other.
rtsock-revert.diff reverts rtsock r1.119 restoring it to it's pristine
state before I touched it.
route-ifaroute.diff has the new fix and is purely functional.

I've elected to keep to the style of the current code and keeping
_rt_key. If it should be changed, then it should be done in another
patch imo, which doesn't have to be pulled up. Remember, I'm targeting
this for pullup to NetBSD-5.

Thanks

Roy

[David Young]

Post by Roy Marples

Post by David Young

To make ammends, attached is a patch to fix this. It is intended for
pullup to netbsd-5.

And here is a different patch.
We add a new function, rt_ifa_connected which returns 1 if the route is
the connected route (prefix, subnet, etc) for the ifa.
This makes the code in rtsock.c and route.c a lot simpler. I've also
added some debug messages via the existing RT_DPRINTF define.
For reference, I've also added a patch which applies this before my
origonal patch was comitted to show the intent of what we're trying to
do here.
I think this pretty much covers everything now. Anyone have any issues
with this before I commit?

Please separate the cosmetic changes from the functional changes
before you commit.
Avoid accessing _rt_key except by using the accessor, rt_getkey().
Perhaps the following code logically belongs in rt_replace_ifa()?

Yes, it makes more sense there.
I will commit these patches tomorrow one after the other.
rtsock-revert.diff reverts rtsock r1.119 restoring it to it's pristine
state before I touched it.
route-ifaroute.diff has the new fix and is purely functional.
I've elected to keep to the style of the current code and keeping
_rt_key. If it should be changed, then it should be done in another
patch imo, which doesn't have to be pulled up. Remember, I'm targeting
this for pullup to NetBSD-5.

Make sure to have a look at all of the sites that call rt_replace_ifa(),
because the callers may expect to manage IFA_ROUTE themselves.

It would be too bad if we ship a worse 5.0 kernel than possible, but if
you can workaround this defect in userland, now, instead of changing the
kernel so late in the 5.0 release cycle, with unforeseen consequences,
then we may ship a better 5.0 kernel than otherwise in the end! :-)

Dave

[Roy Marples]

Post by David Young
Make sure to have a look at all of the sites that call rt_replace_ifa(),
because the callers may expect to manage IFA_ROUTE themselves.
It would be too bad if we ship a worse 5.0 kernel than possible, but if
you can workaround this defect in userland, now, instead of changing the
kernel so late in the 5.0 release cycle, with unforeseen consequences,
then we may ship a better 5.0 kernel than otherwise in the end! :-)

I agree.
My patch to rtsock.c has now been proved to cause more damage than it
tried to fix, so at the very least it should be reverted before 5.0.

I'm now starting to think that my initial reason to transfer the
IFA_ROUTE flag was because of an issue with dhcpcd-5. I'll do some
testing tomorrow against an unpatched kernel.

Thanks

Roy

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

[Roy Marples]

Post by David Young
Make sure to have a look at all of the sites that call rt_replace_ifa(),
because the callers may expect to manage IFA_ROUTE themselves.
It would be too bad if we ship a worse 5.0 kernel than possible, but if
you can workaround this defect in userland, now, instead of changing the
kernel so late in the 5.0 release cycle, with unforeseen consequences,
then we may ship a better 5.0 kernel than otherwise in the end! :-)

OK, here's the issue.

ifconfig bge0 alias 192.168.0.1/24
ifconfig iwi0 alias 192.168.0.2/24
route change 192.168.0.0/24 -ifa 192.168.0.2

So far so good - we've changed the automatic subnet route.

ifconfig bge0 -alias 192.168.0.1/24
ifconfig iwi0 -alias 192.168.0.2/24

Now, should the automatic subnet route still be around, or should it be
deleted? I'm guessing that it should stay around as the fact we changed
it makes it no longer automatic. [1]

http://mail-index.netbsd.org/tech-net/2008/12/23/msg000920.html
I tried to do the same thing for IPv6.
It appears that IPv6 leaves the automatic route around in this instance
as well. I've not tested my rt_replace_ifa() patch to see the route is
removed after all addresses have been when the ifa has been changed.

I think I'll just revert the rtsock.c change, get that pulled up to
NetBSD-5 and document this in dhcpcd-5. It's the safest course of
action.

Thanks

Roy

[1] This all stems from me trying to make dhcpcd restore as much of the
initial environment as possible. One of the features of dhcpcd-5 is to
prefer a wired interface over a wireless one on the same subnet even
when the wireless interface is active first. We do this by changing the
ifa, which works nicely. However, there is also the case where the user
has an existing subnet route, which will be destroyed by dhcpcd in this
instance. The rt_replace_ifa() patch was an effort to avoid this.

[Steven M. Bellovin]

On Wed, 11 Mar 2009 02:14:47 +0000

Post by Roy Marples

Post by David Young
Make sure to have a look at all of the sites that call
rt_replace_ifa(), because the callers may expect to manage
IFA_ROUTE themselves.
It would be too bad if we ship a worse 5.0 kernel than possible,
but if you can workaround this defect in userland, now, instead of
changing the kernel so late in the 5.0 release cycle, with
unforeseen consequences, then we may ship a better 5.0 kernel than
otherwise in the end! :-)

OK, here's the issue.
ifconfig bge0 alias 192.168.0.1/24
ifconfig iwi0 alias 192.168.0.2/24
route change 192.168.0.0/24 -ifa 192.168.0.2
So far so good - we've changed the automatic subnet route.
ifconfig bge0 -alias 192.168.0.1/24
ifconfig iwi0 -alias 192.168.0.2/24
Now, should the automatic subnet route still be around, or should it
be deleted? I'm guessing that it should stay around as the fact we
changed it makes it no longer automatic. [1]
http://mail-index.netbsd.org/tech-net/2008/12/23/msg000920.html
I tried to do the same thing for IPv6.
It appears that IPv6 leaves the automatic route around in this
instance as well. I've not tested my rt_replace_ifa() patch to see
the route is removed after all addresses have been when the ifa has
been changed.
I think I'll just revert the rtsock.c change, get that pulled up to
NetBSD-5 and document this in dhcpcd-5. It's the safest course of
action.
Thanks
Roy
[1] This all stems from me trying to make dhcpcd restore as much of
the initial environment as possible. One of the features of dhcpcd-5
is to prefer a wired interface over a wireless one on the same subnet
even when the wireless interface is active first. We do this by
changing the ifa, which works nicely. However, there is also the case
where the user has an existing subnet route, which will be destroyed
by dhcpcd in this instance. The rt_replace_ifa() patch was an effort
to avoid this.

I've had problems in this sort of situation with dhclient and with
routes not going away. What I ended up doing was putting a 'route
flushall' into my suspend/resume script -- which isn't quite right, but
does the trick for most of my situations.


--Steve Bellovin, http://www.cs.columbia.edu/~smb

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

[Roy Marples]

Post by Roy Marples

Post by David Young
Make sure to have a look at all of the sites that call rt_replace_ifa(),
because the callers may expect to manage IFA_ROUTE themselves.
It would be too bad if we ship a worse 5.0 kernel than possible, but if
you can workaround this defect in userland, now, instead of changing the
kernel so late in the 5.0 release cycle, with unforeseen consequences,
then we may ship a better 5.0 kernel than otherwise in the end! :-)

OK, here's the issue.
ifconfig bge0 alias 192.168.0.1/24
ifconfig iwi0 alias 192.168.0.2/24
route change 192.168.0.0/24 -ifa 192.168.0.2
So far so good - we've changed the automatic subnet route.
ifconfig bge0 -alias 192.168.0.1/24
ifconfig iwi0 -alias 192.168.0.2/24
Now, should the automatic subnet route still be around, or should it be
deleted? I'm guessing that it should stay around as the fact we changed
it makes it no longer automatic. [1]
http://mail-index.netbsd.org/tech-net/2008/12/23/msg000920.html
I tried to do the same thing for IPv6.
It appears that IPv6 leaves the automatic route around in this instance
as well. I've not tested my rt_replace_ifa() patch to see the route is
removed after all addresses have been when the ifa has been changed.
[1] This all stems from me trying to make dhcpcd restore as much of the
initial environment as possible. One of the features of dhcpcd-5 is to
prefer a wired interface over a wireless one on the same subnet even
when the wireless interface is active first. We do this by changing the
ifa, which works nicely. However, there is also the case where the user
has an existing subnet route, which will be destroyed by dhcpcd in this
instance. The rt_replace_ifa() patch was an effort to avoid this.

I still believe that my new patch is the right thing todo.
Thus far, the only argument against it is possible impact to 5.0, which
I can easily understand. So I plan to commit this over the weekend and
let it stew in -current so that dhcpcd can take advantage of it for
NetBSD-6.

Thanks

Roy

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