Mailing List Archive

On Sat, 8 May 2004, Michele 'mydecay' Marchetto wrote:

> hi all
>
> i've found a possibile bug in quagga's ripv2 implementation. When
> there is a route in the routing table, and the router receive a
> response for the same route, with same netmask and metric but with
> different nexthop it is silently ignored. conforming to the
> standards, if the router is the same that announced this route the
> first time, nexthop field must be updated.

Seems sensible enough to me, but I'm not really familiar with RIP.
Would anyone who is be able to comment? (sowmini? krzystof?)

> Patch attached.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Nondeterminism means never having to say you are wrong.

Paul Jakma wrote:

>On Sat, 8 May 2004, Michele 'mydecay' Marchetto wrote:
>
>
>>hi all
>>
>>i've found a possibile bug in quagga's ripv2 implementation. When
>>there is a route in the routing table, and the router receive a
>>response for the same route, with same netmask and metric but with
>>different nexthop it is silently ignored. conforming to the
>>standards, if the router is the same that announced this route the
>>first time, nexthop field must be updated.
>>
>>
>
>Seems sensible enough to me, but I'm not really familiar with RIP.
>Would anyone who is be able to comment? (sowmini? krzystof?)
>
>
He is right. Only if a RTE with a same metric, same prefix but a
different nexthop is announced from the same router from the same
interface, we must update the RIP's RIB with the new next-hop.

If this entry is received from a different interface, I think that the
RIP's RIB should not be updated. It means that your patch should check
the interface too.

Regards,
Vincent

On Tue, 2004-05-11 at 09:40, Vincent Jardin wrote:
> If this entry is received from a different interface, I think that the
> RIP's RIB should not be updated. It means that your patch should check
> the interface too.

i think checking that the source address belong to the right router is
enough.
Two routers should only be attached through one broadcast link and so
packets could be sent only through that link.
If, the packets take a path different from the attached link, they will
arrive with higher metric and so they will be discarded.

what do you think about that?

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Tue, 11 May 2004, Vincent Jardin wrote:

> Paul Jakma wrote:
>
> >On Sat, 8 May 2004, Michele 'mydecay' Marchetto wrote:
> >
> >
> >>hi all
> >>
> >>i've found a possibile bug in quagga's ripv2 implementation. When
> >>there is a route in the routing table, and the router receive a
> >>response for the same route, with same netmask and metric but with
> >>different nexthop it is silently ignored. conforming to the
> >>standards, if the router is the same that announced this route the
> >>first time, nexthop field must be updated.
> >>
> >>
> >
> >Seems sensible enough to me, but I'm not really familiar with RIP.
> >Would anyone who is be able to comment? (sowmini? krzystof?)
> >
> >
> He is right. Only if a RTE with a same metric, same prefix but a
> different nexthop is announced from the same router from the same
> interface, we must update the RIP's RIB with the new next-hop.
>
> If this entry is received from a different interface, I think that the
> RIP's RIB should not be updated. It means that your patch should check
> the interface too.

We do not need to check interface twice, since it is already checked by
"same":

same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
&& (rinfo->ifindex == ifp->ifindex));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Best Regards,

Krzysztof Olêdzki

On Tue, 2004-05-11 at 18:17, Krzysztof Oledzki wrote:
> We do not need to check interface twice, since it is already checked by
> "same":

that's even right!
so my patch can be merged in the tree?
if so, i will send it to zebra ml too, with some changes, of course..

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Sat, 8 May 2004, Michele 'mydecay' Marchetto wrote:

> hi all
>
> i've found a possibile bug in quagga's ripv2 implementation. When there
> is a route in the routing table, and the router receive a response for
> the same route, with same netmask and metric but with different nexthop
> it is silently ignored. conforming to the standards, if the router is
> the same that announced this route the first time, nexthop field must be
> updated.
> Patch attached.

--- rip-old.c 2004-05-08 21:21:04.130947952 +0200
+++ ripd.c 2004-05-08 21:32:30.140658624 +0200
@@ -560,11 +560,13 @@
if (same)
rip_timeout_update (rinfo);

- /* Next, compare the metrics. If the datagram is from the same
- router as the existing route, and the new metric is different
- than the old one; or, if the new metric is lower than the old
- one, or if the tag has been changed; do the following actions: */
- if ((same && rinfo->metric != rte->metric) ||
+ /* Next, compare the metrics and the next hop. If the datagram is
+ from the same router as the existing route, and the new metric
+ or next hop is different than the old ones; or, if the new metric
+ is lower than the old one, or if the tag has been changed;
+ do the following actions: */
+ if ((same && (rinfo->metric != rte->metric ||
+ rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
(rte->metric < rinfo->metric) ||
(same && (rinfo->metric == rte->metric) && ntohs(rte->tag) != rinfo->tag))
{

Hm... OK, but this check still looks wrong to me - what about the tag
compare? If received datagram is "same" but with different metric the
rinfo will be updated anyway, so there is no need to check it again while
comparing tags. What about this:

if ((same && (rinfo->metric != rte->metric ||
ntohs(rte->tag) != rinfo->tag ||
rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
(rte->metric < rinfo->metric))

Something more to fix here or can I just make a diff?

Best regards,

Krzysztof Olêdzki

On Tue, 2004-05-11 at 18:41, Krzysztof Oledzki wrote:
> Hm... OK, but this check still looks wrong to me - what about the tag
> compare? If received datagram is "same" but with different metric the
> rinfo will be updated anyway, so there is no need to check it again while
> comparing tags. What about this:
>
> if ((same && (rinfo->metric != rte->metric ||
> ntohs(rte->tag) != rinfo->tag ||
> rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
> (rte->metric < rinfo->metric))

i'm completely ok with this, you can make the diff :)

regards

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Tue, 11 May 2004, Michele 'mydecay' Marchetto wrote:

> On Tue, 2004-05-11 at 18:17, Krzysztof Oledzki wrote:
> > We do not need to check interface twice, since it is already checked by
> > "same":
>
> that's even right!
> so my patch can be merged in the tree?
> if so, i will send it to zebra ml too, with some changes, of course..

Ahh...

(...)
+ rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
(...)

Not so fast ;-) I have just realized we should probably use
!IPV4_ADDR_SAME instead of the != compare:

!IPV4_ADDR_SAME(rte->nexthop, rinfo->nexthop)

Best Regards,

Krzysztof Olêdzki

On Tue, 2004-05-11 at 18:50, Krzysztof Oledzki wrote:
> Not so fast ;-) I have just realized we should probably use
> !IPV4_ADDR_SAME instead of the != compare:
>
> !IPV4_ADDR_SAME(rte->nexthop, rinfo->nexthop)

yes, this compare all the sockaddr_in structure, it is only a question
of style in this case, i think, but it is the right thing to do :)

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Tue, 11 May 2004, Michele 'mydecay' Marchetto wrote:

> yes, this compare all the sockaddr_in structure, it is only a question
> of style in this case, i think, but it is the right thing to do :)

Good good.. I'll queue it up and apply it soonish if there's no further
discussion (and no-one beats me to the commit)

--paulj

On Tue, 11 May 2004, Michele 'mydecay' Marchetto wrote:

> On Tue, 2004-05-11 at 18:41, Krzysztof Oledzki wrote:
> > Hm... OK, but this check still looks wrong to me - what about the tag
> > compare? If received datagram is "same" but with different metric the
> > rinfo will be updated anyway, so there is no need to check it again while
> > comparing tags. What about this:
> >
> > if ((same && (rinfo->metric != rte->metric ||
> > ntohs(rte->tag) != rinfo->tag ||
> > rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
> > (rte->metric < rinfo->metric))
>
> i'm completely ok with this, you can make the diff :)

Oh, currently this code looks like this, I need some time to understand
this rip_distance_apply modification:

if ((same && rinfo->metric != rte->metric)
|| (rte->metric < rinfo->metric)
|| ((same)
&& (rinfo->metric == rte->metric)
&& ntohs (rte->tag) != rinfo->tag)
|| (rinfo->distance > rip_distance_apply (&rinfotmp))
|| ((rinfo->distance != rip_distance_apply (rinfo)) && same))
{

It seems that it was changed just before the 0.96.5 release. There are
some more changes. Heh, Heh. I'm not sure if it was a good idea to make
such modifications while we were just going to relase new stable version.

Best regards,

Krzysztof Olêdzki

On Tue, 11 May 2004, Krzysztof Oledzki wrote:

>
>
> On Tue, 11 May 2004, Michele 'mydecay' Marchetto wrote:
>
> > On Tue, 2004-05-11 at 18:41, Krzysztof Oledzki wrote:
> > > Hm... OK, but this check still looks wrong to me - what about the tag
> > > compare? If received datagram is "same" but with different metric the
> > > rinfo will be updated anyway, so there is no need to check it again while
> > > comparing tags. What about this:
> > >
> > > if ((same && (rinfo->metric != rte->metric ||
> > > ntohs(rte->tag) != rinfo->tag ||
> > > rte->nexthop.s_addr != rinfo->nexthop.s_addr)) ||
> > > (rte->metric < rinfo->metric))
> >
> > i'm completely ok with this, you can make the diff :)
>
> Oh, currently this code looks like this, I need some time to understand
> this rip_distance_apply modification:
>
> if ((same && rinfo->metric != rte->metric)
> || (rte->metric < rinfo->metric)
> || ((same)
> && (rinfo->metric == rte->metric)
> && ntohs (rte->tag) != rinfo->tag)
> || (rinfo->distance > rip_distance_apply (&rinfotmp))
> || ((rinfo->distance != rip_distance_apply (rinfo)) && same))
> {
>
> It seems that it was changed just before the 0.96.5 release. There are
> some more changes. Heh, Heh. I'm not sure if it was a good idea to make
> such modifications while we were just going to relase new stable version.

OK, OK :)

There were two changes, luckily ;-) This date from changelog is quite
tricky:

2004-03-19 Jean-Yves Simon <lethalwp@tiscali.be>

* ripd.c: make ripd also check on administrative distance of his
own links to update routes.

This change was commited 2004/05/01.. Heh :(

So no problem, thats the full diff:

+++ quagga-0.96.4-20040503/ripd/ripd.c 2004-05-01 22:45:38.000000000
+0200
@@ -387,7 +387,7 @@
int ret;
struct prefix_ipv4 p;
struct route_node *rp;
- struct rip_info *rinfo;
+ struct rip_info *rinfo, rinfotmp;
struct rip_interface *ri;
struct in_addr *nexthop;
u_char oldmetric;
@@ -560,13 +560,27 @@
if (same)
rip_timeout_update (rinfo);

+
+ /* Fill in a minimaly temporary rip_info structure, for a future
+ rip_distance_apply() use) */
+ memset (&rinfo,0,sizeof(rinfotmp));
~~~~~
Oh!!! This swwms to be the "typo in merge of previous patch" fixed
2004-05-03 ;-)


+ IPV4_ADDR_COPY (&rinfotmp.from, &from->sin_addr);
+ rinfotmp.rp=rinfo->rp;
+
+
/* Next, compare the metrics. If the datagram is from the same
router as the existing route, and the new metric is different
than the old one; or, if the new metric is lower than the old
- one, or if the tag has been changed; do the following actions: */
- if ((same && rinfo->metric != rte->metric) ||
- (rte->metric < rinfo->metric) ||
- (same && (rinfo->metric == rte->metric) && ntohs(rte->tag) != rinfo->tag))
+ one, or if the tag has been changed; or if there is a route
+ with a lower administrave distance; or an update of the
+ distance on the actual route; do the following actions: */
+ if (( same && rinfo->metric != rte->metric )
+ || ( rte->metric < rinfo->metric )
+ || ( (same)
+ && (rinfo->metric == rte->metric)
+ && ntohs(rte->tag) != rinfo->tag )
+ || ( rinfo->distance > rip_distance_apply (&rinfotmp) )
+ || ( (rinfo->distance != rip_distance_apply (rinfo)) && same ))
{
/* - Adopt the route from the datagram. That is, put the
new metric in, and adjust the next hop address (if

I'm not sure if we really need to create new rinfotmp? Why we simply can't
call rip_distance_apply twice with rinfo as argument?

Best Regards,

Krzysztof Olêdzki

On Wed, 12 May 2004, Jean-Yves Simon wrote:

> On Wed, 2004-05-12 at 02:34, Krzysztof Oledzki wrote:
>
> > +
> > + /* Fill in a minimaly temporary rip_info structure, for a future
> > + rip_distance_apply() use) */
> > + memset (&rinfo,0,sizeof(rinfotmp));
> > ~~~~~
> > Oh!!! This swwms to be the "typo in merge of previous patch" fixed
> > 2004-05-03 ;-)
> Yes, sorry, must be a big mistake of mine :) amazing it didn't segfault
> when i played with it
>
>
> > + if (( same && rinfo->metric != rte->metric )
> > + || ( rte->metric < rinfo->metric )
> > + || ( (same)
> > + && (rinfo->metric == rte->metric)
> > + && ntohs(rte->tag) != rinfo->tag )
> > + || ( rinfo->distance > rip_distance_apply (&rinfotmp) )
> > + || ( (rinfo->distance != rip_distance_apply (rinfo)) && same ))
> > {
> > /* - Adopt the route from the datagram. That is, put the
> > new metric in, and adjust the next hop address (if
> >
> > I'm not sure if we really need to create new rinfotmp? Why we simply can't
> > call rip_distance_apply twice with rinfo as argument?
> My problem was to get the administrative distance of new incoming rip
> message & compare it with the older rip route already existing,
Ah, yes :) So, finally, what about this:

if ((same && (rinfo->metric != rte->metric ||
ntohs(rte->tag) != rinfo->tag ||
rte->nexthop.s_addr != rinfo->nexthop.s_addr ||
rinfo->distance != rip_distance_apply (rinfo))) ||
(rinfo->distance > rip_distance_apply (&rinfotmp)) ||
(rte->metric < rinfo->metric))
{

> The only way i found those days to do it was to fill in a tmp rinfo
> "rinfotmp.rp=rinfo->rp;"
OK. Now I got the point :)

> Also i haven't played a lot with routing these days, haven't checked
> your patch yet, it's on my todo list for the day the envy comes back =)
Plase, wait about one week. I'm going to prepare a new one with some fixes
and whitch can be applied to new quagga code with the rip_output_process()
function messed by indent. :(

> the patch changed the changelog because the ppl applying it weren't
> doing it, and i already lost "credit" for some other patches ;)
Heh, you are not alone ;-)

Best regards,

Krzysztof Olêdzki

On Wed, 12 May 2004, Krzysztof Oledzki wrote:

> > > I'm not sure if we really need to create new rinfotmp? Why we simply can't
> > > call rip_distance_apply twice with rinfo as argument?
> > My problem was to get the administrative distance of new incoming rip
> > message & compare it with the older rip route already existing,
> Ah, yes :) So, finally, what about this:
>
> if ((same && (rinfo->metric != rte->metric ||
> ntohs(rte->tag) != rinfo->tag ||
> rte->nexthop.s_addr != rinfo->nexthop.s_addr ||
> rinfo->distance != rip_distance_apply (rinfo))) ||
> (rinfo->distance > rip_distance_apply (&rinfotmp)) ||
> (rte->metric < rinfo->metric))
> {

Or this one, with IPV4_ADDR_SAME, rinfo at the left and rte at the right
side and with correct comment, attached patch for quagga-0.96.5:

diff -Nur quagga-0.96.5-orig/ripd/ripd.c quagga-0.96.5/ripd/ripd.c
--- quagga-0.96.5-orig/ripd/ripd.c 2004-05-03 21:49:56.000000000 +0200
+++ quagga-0.96.5/ripd/ripd.c 2004-05-13 01:00:00.000000000 +0200
@@ -559,28 +559,25 @@
if (same)
rip_timeout_update (rinfo);

-
/* Fill in a minimaly temporary rip_info structure, for a future
rip_distance_apply() use) */
memset (&rinfotmp, 0, sizeof (rinfotmp));
IPV4_ADDR_COPY (&rinfotmp.from, &from->sin_addr);
rinfotmp.rp = rinfo->rp;

-
- /* Next, compare the metrics. If the datagram is from the same
- router as the existing route, and the new metric is different
- than the old one; or, if the new metric is lower than the old
- one, or if the tag has been changed; or if there is a route
- with a lower administrave distance; or an update of the
- distance on the actual route; do the following actions: */
- if ((same && rinfo->metric != rte->metric)
- || (rte->metric < rinfo->metric)
- || ((same)
- && (rinfo->metric == rte->metric)
- && ntohs (rte->tag) != rinfo->tag)
- || (rinfo->distance > rip_distance_apply (&rinfotmp))
- || ((rinfo->distance != rip_distance_apply (rinfo)) && same))
- {
+ /* Next, compare the existing route with new datagram. If the
+ datagram is from the same router as the existing route and
+ the distance, metric, tag or nexthop has been changed; or
+ if there is a route with a lower administrave distance or
+ metric; do the following actions: */
+
+ if ((same && (rinfo->distance != rip_distance_apply (rinfo) ||
+ rinfo->metric != rte->metric ||
+ ntohs(rinfo->tag) != rte->tag ||
+ !IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop))) ||
+ (rinfo->distance > rip_distance_apply (&rinfotmp)) ||
+ (rinfo->metric > rte->metric))
+ {
/* - Adopt the route from the datagram. That is, put the
new metric in, and adjust the next hop address (if
necessary). */

Hope this is the final version ;-)

Massage to the ripd/ChangeLog file:

2004-05-13 Michele 'mydecay' Marchetto <smarchetto1@tin.it>
Krzysztof Oledzki <oleq@ans.pl>

* Do not ignore updates of routes with different nexthop only
* Small code cleanup.

Best regards,

Krzysztof Olêdzki

On Wed, 12 May 2004, Krzysztof Oledzki wrote:

> On Wed, 12 May 2004, Jean-Yves Simon wrote:

> Plase, wait about one week. I'm going to prepare a new one with some fixes
> and whitch can be applied to new quagga code with the rip_output_process()
> function messed by indent. :(

Look forward to it :) I'll address the indent issue seperately.

> > the patch changed the changelog because the ppl applying it weren't
> > doing it, and i already lost "credit" for some other patches ;)

> Heh, you are not alone ;-)

If you point out the ChangeLog entries and the quagga-user/quagga-dev
post (or bugzilla ID) such omissions can be fixed.

> Best regards,
>
> Krzysztof Olędzki

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Always try to do things in chronological order; it's less confusing that way.