ospfd: type p2p
Hi tech@,
earlier this year I sent a diff that allowed to change an interface
from broadcast to point-to-point.
https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
It turned out that this was not sufficient. It made the adjacency
come up in p2p mode (no selection of DR or BDR) but didn't set a valid
next hop for routes learned over this p2p link. Actually the next hop was
0.0.0.0 which was never installed into the routing table.
This is because for P2P interfaces the neighbor address is not taken from
the received hello but from the "destination" parameter configured on the
interface. Since this is not set on a broadcast interface the address is
0.0.0.0.
My new diff changes this. Now also for P2P links the IP address of the
neighbor is taken from the hello packets (src address). This on it's own
would make it simpler to interfere with the routing from remote. One could
send unicast ospf hello messages and potentially disrupt the routing setup.
I believe I mitigated this with an additional check I committed in August:
only hello messages sent to the multicast address are now processed.
The config looks like this:
area 0.0.0.0 {
interface em0 {
type p2p
}
}
It would be nice to get test reports for this new feature (check the fib
and routing table!) and also test reports with real p2p2 interfaces (gif
or gre).
Of course OKs are also welcome. ;-)
Remi
Index: hello.c
===
RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v
retrieving revision 1.24
diff -u -p -r1.24 hello.c
--- hello.c 12 Aug 2019 20:21:58 - 1.24
+++ hello.c 21 Sep 2019 22:06:17 -
@@ -189,14 +189,13 @@ recv_hello(struct iface *iface, struct i
nbr->dr.s_addr = hello.d_rtr;
nbr->bdr.s_addr = hello.bd_rtr;
nbr->priority = hello.rtr_priority;
- /* XXX neighbor address shouldn't be stored on virtual links */
- nbr->addr.s_addr = src.s_addr;
+ nbr_update_addr(nbr->peerid, src);
}
if (nbr->addr.s_addr != src.s_addr) {
log_warnx("%s: neighbor ID %s changed its IP address",
__func__, inet_ntoa(nbr->id));
- nbr->addr.s_addr = src.s_addr;
+ nbr_update_addr(nbr->peerid, src);
}
nbr->options = hello.opts;
Index: lsupdate.c
===
RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v
retrieving revision 1.46
diff -u -p -r1.46 lsupdate.c
--- lsupdate.c 15 Jul 2019 18:26:39 - 1.46
+++ lsupdate.c 15 Aug 2019 21:10:13 -
@@ -470,7 +470,7 @@ ls_retrans_timer(int fd, short event, vo
/* ls_retrans_list_free retriggers the timer */
return;
} else if (nbr->iface->type == IF_TYPE_POINTOPOINT)
- memcpy(&addr, &nbr->iface->dst, sizeof(addr));
+ memcpy(&addr, &nbr->addr, sizeof(addr));
else
inet_aton(AllDRouters, &addr);
} else
Index: neighbor.c
===
RCS file: /cvs/src/usr.sbin/ospfd/neighbor.c,v
retrieving revision 1.48
diff -u -p -r1.48 neighbor.c
--- neighbor.c 9 Feb 2018 02:14:03 - 1.48
+++ neighbor.c 21 Sep 2019 15:28:43 -
@@ -312,6 +312,7 @@ nbr_new(u_int32_t nbr_id, struct iface *
bzero(&rn, sizeof(rn));
rn.id.s_addr = nbr->id.s_addr;
rn.area_id.s_addr = nbr->iface->area->id.s_addr;
+ rn.addr.s_addr = nbr->addr.s_addr;
rn.ifindex = nbr->iface->ifindex;
rn.state = nbr->state;
rn.self = self;
@@ -347,6 +348,23 @@ nbr_del(struct nbr *nbr)
LIST_REMOVE(nbr, hash);
free(nbr);
+}
+
+int
+nbr_update_addr(u_int32_t peerid, struct in_addr addr) {
+
+ struct nbr *nbr = NULL;
+
+ nbr = nbr_find_peerid(peerid);
+ if (nbr == NULL)
+ return (1);
+
+ /* XXX neighbor address shouldn't be stored on virtual links */
+ nbr->addr.s_addr = addr.s_addr;
+ ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, peerid, 0, &addr,
+ sizeof(addr));
+
+ return (0);
}
struct nbr *
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.108
diff -u -p -r1.108 ospfd.c
--- ospfd.c 16 May 2019 05:49:22 - 1.108
+++ ospfd.c 23 Jun 2019 21:06:44 -
@@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct
if_fsm(i, IF_EVT_UP);
}
+ if (i->p2p != xi->p2p) {
+ /* re-add interface to enable or disable DR election */
+ if (ospfd_process == PROC_OSPF_ENGINE)
+ if_fsm(i, IF_EVT_DOWN);
+ els
Re: ospfd: type p2p
On 25/10/2019 13:57, Remi Locherer wrote:
> Hi tech@,
>
> earlier this year I sent a diff that allowed to change an interface
> from broadcast to point-to-point.
>
> https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
>
> It turned out that this was not sufficient. It made the adjacency
> come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> next hop for routes learned over this p2p link. Actually the next hop was
> 0.0.0.0 which was never installed into the routing table.
>
> This is because for P2P interfaces the neighbor address is not taken from
> the received hello but from the "destination" parameter configured on the
> interface. Since this is not set on a broadcast interface the address is
> 0.0.0.0.
>
> My new diff changes this. Now also for P2P links the IP address of the
> neighbor is taken from the hello packets (src address). This on it's own
> would make it simpler to interfere with the routing from remote. One could
> send unicast ospf hello messages and potentially disrupt the routing setup.
> I believe I mitigated this with an additional check I committed in August:
> only hello messages sent to the multicast address are now processed.
>
> The config looks like this:
>
> area 0.0.0.0 {
> interface em0 {
> type p2p
> }
> }
>
> It would be nice to get test reports for this new feature (check the fib
> and routing table!) and also test reports with real p2p2 interfaces (gif
> or gre).
>
> Of course OKs are also welcome. ;-)
>
> Remi
Hi,
>From first test seems to work :)
looking forward test it for IPv6 as well
thanks
Giannis
>
>
>
> Index: hello.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 hello.c
> --- hello.c 12 Aug 2019 20:21:58 - 1.24
> +++ hello.c 21 Sep 2019 22:06:17 -
> @@ -189,14 +189,13 @@ recv_hello(struct iface *iface, struct i
> nbr->dr.s_addr = hello.d_rtr;
> nbr->bdr.s_addr = hello.bd_rtr;
> nbr->priority = hello.rtr_priority;
> - /* XXX neighbor address shouldn't be stored on virtual links */
> - nbr->addr.s_addr = src.s_addr;
> + nbr_update_addr(nbr->peerid, src);
> }
>
> if (nbr->addr.s_addr != src.s_addr) {
> log_warnx("%s: neighbor ID %s changed its IP address",
> __func__, inet_ntoa(nbr->id));
> - nbr->addr.s_addr = src.s_addr;
> + nbr_update_addr(nbr->peerid, src);
> }
>
> nbr->options = hello.opts;
> Index: lsupdate.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 lsupdate.c
> --- lsupdate.c15 Jul 2019 18:26:39 - 1.46
> +++ lsupdate.c15 Aug 2019 21:10:13 -
> @@ -470,7 +470,7 @@ ls_retrans_timer(int fd, short event, vo
> /* ls_retrans_list_free retriggers the timer */
> return;
> } else if (nbr->iface->type == IF_TYPE_POINTOPOINT)
> - memcpy(&addr, &nbr->iface->dst, sizeof(addr));
> + memcpy(&addr, &nbr->addr, sizeof(addr));
> else
> inet_aton(AllDRouters, &addr);
> } else
> Index: neighbor.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/neighbor.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 neighbor.c
> --- neighbor.c9 Feb 2018 02:14:03 - 1.48
> +++ neighbor.c21 Sep 2019 15:28:43 -
> @@ -312,6 +312,7 @@ nbr_new(u_int32_t nbr_id, struct iface *
> bzero(&rn, sizeof(rn));
> rn.id.s_addr = nbr->id.s_addr;
> rn.area_id.s_addr = nbr->iface->area->id.s_addr;
> + rn.addr.s_addr = nbr->addr.s_addr;
> rn.ifindex = nbr->iface->ifindex;
> rn.state = nbr->state;
> rn.self = self;
> @@ -347,6 +348,23 @@ nbr_del(struct nbr *nbr)
> LIST_REMOVE(nbr, hash);
>
> free(nbr);
> +}
> +
> +int
> +nbr_update_addr(u_int32_t peerid, struct in_addr addr) {
> +
> + struct nbr *nbr = NULL;
> +
> + nbr = nbr_find_peerid(peerid);
> + if (nbr == NULL)
> + return (1);
> +
> + /* XXX neighbor address shouldn't be stored on virtual links */
> + nbr->addr.s_addr = addr.s_addr;
> + ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, peerid, 0, &addr,
> + sizeof(addr));
> +
> + return (0);
> }
>
> struct nbr *
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 ospfd.c
> --- ospfd.c 16 May 2019 05:49:22 - 1.108
> +++ ospfd.c 23 Jun 2019 21:06:44 -
> @@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct
>
Re: ospfd: type p2p
On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote:
> On 25/10/2019 13:57, Remi Locherer wrote:
> > Hi tech@,
> >
> > earlier this year I sent a diff that allowed to change an interface
> > from broadcast to point-to-point.
> >
> > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
> >
> > It turned out that this was not sufficient. It made the adjacency
> > come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> > next hop for routes learned over this p2p link. Actually the next hop was
> > 0.0.0.0 which was never installed into the routing table.
> >
> > This is because for P2P interfaces the neighbor address is not taken from
> > the received hello but from the "destination" parameter configured on the
> > interface. Since this is not set on a broadcast interface the address is
> > 0.0.0.0.
> >
> > My new diff changes this. Now also for P2P links the IP address of the
> > neighbor is taken from the hello packets (src address). This on it's own
> > would make it simpler to interfere with the routing from remote. One could
> > send unicast ospf hello messages and potentially disrupt the routing setup.
> > I believe I mitigated this with an additional check I committed in August:
> > only hello messages sent to the multicast address are now processed.
> >
> > The config looks like this:
> >
> > area 0.0.0.0 {
> > interface em0 {
> > type p2p
> > }
> > }
> >
> > It would be nice to get test reports for this new feature (check the fib
> > and routing table!) and also test reports with real p2p2 interfaces (gif
> > or gre).
> >
> > Of course OKs are also welcome. ;-)
> >
> > Remi
>
>
> Hi,
>
> From first test seems to work :)
Thank you for testing!
>
> looking forward test it for IPv6 as well
Sure, I plan to also do this this for ospf6d.
Re: ospfd: type p2p
On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote:
> On 25/10/2019 13:57, Remi Locherer wrote:
> > Hi tech@,
> >
> > earlier this year I sent a diff that allowed to change an interface
> > from broadcast to point-to-point.
> >
> > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
> >
> > It turned out that this was not sufficient. It made the adjacency
> > come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> > next hop for routes learned over this p2p link. Actually the next hop was
> > 0.0.0.0 which was never installed into the routing table.
> >
> > This is because for P2P interfaces the neighbor address is not taken from
> > the received hello but from the "destination" parameter configured on the
> > interface. Since this is not set on a broadcast interface the address is
> > 0.0.0.0.
> >
> > My new diff changes this. Now also for P2P links the IP address of the
> > neighbor is taken from the hello packets (src address). This on it's own
> > would make it simpler to interfere with the routing from remote. One could
> > send unicast ospf hello messages and potentially disrupt the routing setup.
> > I believe I mitigated this with an additional check I committed in August:
> > only hello messages sent to the multicast address are now processed.
> >
> > The config looks like this:
> >
> > area 0.0.0.0 {
> > interface em0 {
> > type p2p
> > }
> > }
> >
> > It would be nice to get test reports for this new feature (check the fib
> > and routing table!) and also test reports with real p2p2 interfaces (gif
> > or gre).
> >
> > Of course OKs are also welcome. ;-)
> >
> > Remi
>
>
> Hi,
>
> From first test seems to work :)
>
> looking forward test it for IPv6 as well
>
> thanks
>
> Giannis
Anyone willing to OK this?
Index: hello.c
===
RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v
retrieving revision 1.24
diff -u -p -r1.24 hello.c
--- hello.c 12 Aug 2019 20:21:58 - 1.24
+++ hello.c 21 Sep 2019 22:06:17 -
@@ -189,14 +189,13 @@ recv_hello(struct iface *iface, struct i
nbr->dr.s_addr = hello.d_rtr;
nbr->bdr.s_addr = hello.bd_rtr;
nbr->priority = hello.rtr_priority;
- /* XXX neighbor address shouldn't be stored on virtual links */
- nbr->addr.s_addr = src.s_addr;
+ nbr_update_addr(nbr->peerid, src);
}
if (nbr->addr.s_addr != src.s_addr) {
log_warnx("%s: neighbor ID %s changed its IP address",
__func__, inet_ntoa(nbr->id));
- nbr->addr.s_addr = src.s_addr;
+ nbr_update_addr(nbr->peerid, src);
}
nbr->options = hello.opts;
Index: lsupdate.c
===
RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v
retrieving revision 1.46
diff -u -p -r1.46 lsupdate.c
--- lsupdate.c 15 Jul 2019 18:26:39 - 1.46
+++ lsupdate.c 15 Aug 2019 21:10:13 -
@@ -470,7 +470,7 @@ ls_retrans_timer(int fd, short event, vo
/* ls_retrans_list_free retriggers the timer */
return;
} else if (nbr->iface->type == IF_TYPE_POINTOPOINT)
- memcpy(&addr, &nbr->iface->dst, sizeof(addr));
+ memcpy(&addr, &nbr->addr, sizeof(addr));
else
inet_aton(AllDRouters, &addr);
} else
Index: neighbor.c
===
RCS file: /cvs/src/usr.sbin/ospfd/neighbor.c,v
retrieving revision 1.48
diff -u -p -r1.48 neighbor.c
--- neighbor.c 9 Feb 2018 02:14:03 - 1.48
+++ neighbor.c 21 Sep 2019 15:28:43 -
@@ -312,6 +312,7 @@ nbr_new(u_int32_t nbr_id, struct iface *
bzero(&rn, sizeof(rn));
rn.id.s_addr = nbr->id.s_addr;
rn.area_id.s_addr = nbr->iface->area->id.s_addr;
+ rn.addr.s_addr = nbr->addr.s_addr;
rn.ifindex = nbr->iface->ifindex;
rn.state = nbr->state;
rn.self = self;
@@ -347,6 +348,23 @@ nbr_del(struct nbr *nbr)
LIST_REMOVE(nbr, hash);
free(nbr);
+}
+
+int
+nbr_update_addr(u_int32_t peerid, struct in_addr addr) {
+
+ struct nbr *nbr = NULL;
+
+ nbr = nbr_find_peerid(peerid);
+ if (nbr == NULL)
+ return (1);
+
+ /* XXX neighbor address shouldn't be stored on virtual links */
+ nbr->addr.s_addr = addr.s_addr;
+ ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, peerid, 0, &addr,
+ sizeof(addr));
+
+ return (0);
}
struct nbr *
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.108
diff -u -p -r1.108 ospfd.c
--- ospfd.c 16 May 2019 05:49:22 - 1.108
+++ ospfd.c 23 Jun 2019 21:06:44 -
@@ -91
Re: ospfd: type p2p
On Fri, Nov 15, 2019 at 06:06:42PM +0100, Remi Locherer wrote:
> On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote:
> > On 25/10/2019 13:57, Remi Locherer wrote:
> > > Hi tech@,
> > >
> > > earlier this year I sent a diff that allowed to change an interface
> > > from broadcast to point-to-point.
> > >
> > > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
> > >
> > > It turned out that this was not sufficient. It made the adjacency
> > > come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> > > next hop for routes learned over this p2p link. Actually the next hop was
> > > 0.0.0.0 which was never installed into the routing table.
> > >
> > > This is because for P2P interfaces the neighbor address is not taken from
> > > the received hello but from the "destination" parameter configured on the
> > > interface. Since this is not set on a broadcast interface the address is
> > > 0.0.0.0.
> > >
> > > My new diff changes this. Now also for P2P links the IP address of the
> > > neighbor is taken from the hello packets (src address). This on it's own
> > > would make it simpler to interfere with the routing from remote. One could
> > > send unicast ospf hello messages and potentially disrupt the routing
> > > setup.
> > > I believe I mitigated this with an additional check I committed in August:
> > > only hello messages sent to the multicast address are now processed.
> > >
> > > The config looks like this:
> > >
> > > area 0.0.0.0 {
> > > interface em0 {
> > > type p2p
> > > }
> > > }
> > >
> > > It would be nice to get test reports for this new feature (check the fib
> > > and routing table!) and also test reports with real p2p2 interfaces (gif
> > > or gre).
> > >
> > > Of course OKs are also welcome. ;-)
> > >
> > > Remi
> >
> >
> > Hi,
> >
> > From first test seems to work :)
> >
> > looking forward test it for IPv6 as well
> >
> > thanks
> >
> > Giannis
>
>
> Anyone willing to OK this?
See inline.
> Index: hello.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 hello.c
> --- hello.c 12 Aug 2019 20:21:58 - 1.24
> +++ hello.c 21 Sep 2019 22:06:17 -
> @@ -189,14 +189,13 @@ recv_hello(struct iface *iface, struct i
> nbr->dr.s_addr = hello.d_rtr;
> nbr->bdr.s_addr = hello.bd_rtr;
> nbr->priority = hello.rtr_priority;
> - /* XXX neighbor address shouldn't be stored on virtual links */
> - nbr->addr.s_addr = src.s_addr;
> + nbr_update_addr(nbr->peerid, src);
> }
>
> if (nbr->addr.s_addr != src.s_addr) {
> log_warnx("%s: neighbor ID %s changed its IP address",
> __func__, inet_ntoa(nbr->id));
> - nbr->addr.s_addr = src.s_addr;
> + nbr_update_addr(nbr->peerid, src);
> }
>
> nbr->options = hello.opts;
> Index: lsupdate.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 lsupdate.c
> --- lsupdate.c15 Jul 2019 18:26:39 - 1.46
> +++ lsupdate.c15 Aug 2019 21:10:13 -
> @@ -470,7 +470,7 @@ ls_retrans_timer(int fd, short event, vo
> /* ls_retrans_list_free retriggers the timer */
> return;
> } else if (nbr->iface->type == IF_TYPE_POINTOPOINT)
> - memcpy(&addr, &nbr->iface->dst, sizeof(addr));
> + memcpy(&addr, &nbr->addr, sizeof(addr));
> else
> inet_aton(AllDRouters, &addr);
> } else
> Index: neighbor.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/neighbor.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 neighbor.c
> --- neighbor.c9 Feb 2018 02:14:03 - 1.48
> +++ neighbor.c21 Sep 2019 15:28:43 -
> @@ -312,6 +312,7 @@ nbr_new(u_int32_t nbr_id, struct iface *
> bzero(&rn, sizeof(rn));
> rn.id.s_addr = nbr->id.s_addr;
> rn.area_id.s_addr = nbr->iface->area->id.s_addr;
> + rn.addr.s_addr = nbr->addr.s_addr;
> rn.ifindex = nbr->iface->ifindex;
> rn.state = nbr->state;
> rn.self = self;
> @@ -347,6 +348,23 @@ nbr_del(struct nbr *nbr)
> LIST_REMOVE(nbr, hash);
>
> free(nbr);
> +}
> +
> +int
Since the callers of nbr_update_addr() don't use the return value make
this a void function.
> +nbr_update_addr(u_int32_t peerid, struct in_addr addr) {
> +
Fix bad placement of {
> + struct nbr *nbr = NULL;
> +
> + nbr = nbr_find_peerid(peerid);
> + if (nbr == NULL)
> + return (1);
Why passing peerid to this function to look up the neighbor again when the
functions are called like this?
nbr_update_addr(n
Re: ospfd: type p2p
On Sat, Nov 16, 2019 at 06:58:35AM +0100, Claudio Jeker wrote:
> On Fri, Nov 15, 2019 at 06:06:42PM +0100, Remi Locherer wrote:
> > On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote:
> > > On 25/10/2019 13:57, Remi Locherer wrote:
> > > > Hi tech@,
> > > >
> > > > earlier this year I sent a diff that allowed to change an interface
> > > > from broadcast to point-to-point.
> > > >
> > > > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
> > > >
> > > > It turned out that this was not sufficient. It made the adjacency
> > > > come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> > > > next hop for routes learned over this p2p link. Actually the next hop
> > > > was
> > > > 0.0.0.0 which was never installed into the routing table.
> > > >
> > > > This is because for P2P interfaces the neighbor address is not taken
> > > > from
> > > > the received hello but from the "destination" parameter configured on
> > > > the
> > > > interface. Since this is not set on a broadcast interface the address is
> > > > 0.0.0.0.
> > > >
> > > > My new diff changes this. Now also for P2P links the IP address of the
> > > > neighbor is taken from the hello packets (src address). This on it's own
> > > > would make it simpler to interfere with the routing from remote. One
> > > > could
> > > > send unicast ospf hello messages and potentially disrupt the routing
> > > > setup.
> > > > I believe I mitigated this with an additional check I committed in
> > > > August:
> > > > only hello messages sent to the multicast address are now processed.
> > > >
> > > > The config looks like this:
> > > >
> > > > area 0.0.0.0 {
> > > > interface em0 {
> > > > type p2p
> > > > }
> > > > }
> > > >
> > > > It would be nice to get test reports for this new feature (check the fib
> > > > and routing table!) and also test reports with real p2p2 interfaces (gif
> > > > or gre).
> > > >
> > > > Of course OKs are also welcome. ;-)
> > > >
> > > > Remi
> > >
> > >
> > > Hi,
> > >
> > > From first test seems to work :)
> > >
> > > looking forward test it for IPv6 as well
> > >
> > > thanks
> > >
> > > Giannis
> >
> >
> > Anyone willing to OK this?
>
> See inline.
>
[...]
>
> Another option is to just add this ospfe_imsg_compose_rde() to the two
> places where the addr is updated.
Right, that is actually simpler.
>
> > +
> > + return (0);
> > }
> >
> > struct nbr *
> > Index: ospfd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > retrieving revision 1.108
> > diff -u -p -r1.108 ospfd.c
> > --- ospfd.c 16 May 2019 05:49:22 - 1.108
> > +++ ospfd.c 23 Jun 2019 21:06:44 -
> > @@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct
> > if_fsm(i, IF_EVT_UP);
> > }
> >
> > + if (i->p2p != xi->p2p) {
> > + /* re-add interface to enable or disable DR election */
> > + if (ospfd_process == PROC_OSPF_ENGINE)
> > + if_fsm(i, IF_EVT_DOWN);
> > + else if (ospfd_process == PROC_RDE_ENGINE)
> > + rde_nbr_iface_del(i);
> > + LIST_REMOVE(i, entry);
> > + if_del(i);
> > + LIST_REMOVE(xi, entry);
> > + LIST_INSERT_HEAD(&a->iface_list, xi, entry);
> > + xi->area = a;
> > + if (ospfd_process == PROC_OSPF_ENGINE)
> > + xi->state = IF_STA_NEW;
> > + continue;
> > + }
>
> This is one big hammer. Would be nice to be a bit softer, also should this
> code be before
> log_debug("merge_interfaces: proc %d area %s merging "
> "interface %s", ospfd_process, inet_ntoa(a->id), i->name);
>
> Since it re-adds an interface. If so it should also have its own
> log_debug(). At least I think it makes little sense to merge most of the
> interface to just remove it and re-add it.
I somehow added this with my first attempt and didn't re-evaluate. It turns
out that a restart is actually sufficient.
>
> > +
> > strlcpy(i->dependon, xi->dependon,
> > sizeof(i->dependon));
> > i->depend_ok = xi->depend_ok;
>
> Unrelated but I feel this code should be moved up before
> if (i->passive != xi->passive) {
> ...
> }
Yes, I'll send a separate diff for that later.
OK for the new diff?
Index: hello.c
===
RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v
retrieving revision 1.24
diff -u -p -r1.24 hello.c
--- hello.c 12 Aug 2019 20:21:58 - 1.24
+++ hello.c 16 Nov 2019 14:52:10 -
@@ -191,12 +191,16 @@ recv_hello(struct iface *iface, struct i
nbr->priority = hello.rtr_priority
Re: ospfd: type p2p
On Sun, Nov 17, 2019 at 12:44:44PM +0100, Remi Locherer wrote:
> On Sat, Nov 16, 2019 at 06:58:35AM +0100, Claudio Jeker wrote:
> > On Fri, Nov 15, 2019 at 06:06:42PM +0100, Remi Locherer wrote:
> > > On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote:
> > > > On 25/10/2019 13:57, Remi Locherer wrote:
> > > > > Hi tech@,
> > > > >
> > > > > earlier this year I sent a diff that allowed to change an interface
> > > > > from broadcast to point-to-point.
> > > > >
> > > > > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2
> > > > >
> > > > > It turned out that this was not sufficient. It made the adjacency
> > > > > come up in p2p mode (no selection of DR or BDR) but didn't set a valid
> > > > > next hop for routes learned over this p2p link. Actually the next hop
> > > > > was
> > > > > 0.0.0.0 which was never installed into the routing table.
> > > > >
> > > > > This is because for P2P interfaces the neighbor address is not taken
> > > > > from
> > > > > the received hello but from the "destination" parameter configured on
> > > > > the
> > > > > interface. Since this is not set on a broadcast interface the address
> > > > > is
> > > > > 0.0.0.0.
> > > > >
> > > > > My new diff changes this. Now also for P2P links the IP address of the
> > > > > neighbor is taken from the hello packets (src address). This on it's
> > > > > own
> > > > > would make it simpler to interfere with the routing from remote. One
> > > > > could
> > > > > send unicast ospf hello messages and potentially disrupt the routing
> > > > > setup.
> > > > > I believe I mitigated this with an additional check I committed in
> > > > > August:
> > > > > only hello messages sent to the multicast address are now processed.
> > > > >
> > > > > The config looks like this:
> > > > >
> > > > > area 0.0.0.0 {
> > > > > interface em0 {
> > > > > type p2p
> > > > > }
> > > > > }
> > > > >
> > > > > It would be nice to get test reports for this new feature (check the
> > > > > fib
> > > > > and routing table!) and also test reports with real p2p2 interfaces
> > > > > (gif
> > > > > or gre).
> > > > >
> > > > > Of course OKs are also welcome. ;-)
> > > > >
> > > > > Remi
> > > >
> > > >
> > > > Hi,
> > > >
> > > > From first test seems to work :)
> > > >
> > > > looking forward test it for IPv6 as well
> > > >
> > > > thanks
> > > >
> > > > Giannis
> > >
> > >
> > > Anyone willing to OK this?
> >
> > See inline.
> >
>
> [...]
>
> >
> > Another option is to just add this ospfe_imsg_compose_rde() to the two
> > places where the addr is updated.
>
> Right, that is actually simpler.
>
> >
> > > +
> > > + return (0);
> > > }
> > >
> > > struct nbr *
> > > Index: ospfd.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > > retrieving revision 1.108
> > > diff -u -p -r1.108 ospfd.c
> > > --- ospfd.c 16 May 2019 05:49:22 - 1.108
> > > +++ ospfd.c 23 Jun 2019 21:06:44 -
> > > @@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct
> > > if_fsm(i, IF_EVT_UP);
> > > }
> > >
> > > + if (i->p2p != xi->p2p) {
> > > + /* re-add interface to enable or disable DR election */
> > > + if (ospfd_process == PROC_OSPF_ENGINE)
> > > + if_fsm(i, IF_EVT_DOWN);
> > > + else if (ospfd_process == PROC_RDE_ENGINE)
> > > + rde_nbr_iface_del(i);
> > > + LIST_REMOVE(i, entry);
> > > + if_del(i);
> > > + LIST_REMOVE(xi, entry);
> > > + LIST_INSERT_HEAD(&a->iface_list, xi, entry);
> > > + xi->area = a;
> > > + if (ospfd_process == PROC_OSPF_ENGINE)
> > > + xi->state = IF_STA_NEW;
> > > + continue;
> > > + }
> >
> > This is one big hammer. Would be nice to be a bit softer, also should this
> > code be before
> > log_debug("merge_interfaces: proc %d area %s merging "
> > "interface %s", ospfd_process, inet_ntoa(a->id), i->name);
> >
> > Since it re-adds an interface. If so it should also have its own
> > log_debug(). At least I think it makes little sense to merge most of the
> > interface to just remove it and re-add it.
>
> I somehow added this with my first attempt and didn't re-evaluate. It turns
> out that a restart is actually sufficient.
>
> >
> > > +
> > > strlcpy(i->dependon, xi->dependon,
> > > sizeof(i->dependon));
> > > i->depend_ok = xi->depend_ok;
> >
> > Unrelated but I feel this code should be moved up before
> > if (i->passive != xi->passive) {
> > ...
> > }
>
> Yes, I'll send a separate diff for that later.
>
> OK for the new diff?
>
>
>
> Index: hello.c
> =
Re: ospfd: type p2p
On 17/11/2019 13:44, Remi Locherer wrote: > Yes, I'll send a separate diff for that later. > > OK for the new diff? Works for me. G
