On Mon, Jul 24, 2017 at 04:59:46PM +0200, Remi Locherer wrote:
> On Fri, Jul 21, 2017 at 06:24:06PM +0200, Remi Locherer wrote:
> > On Fri, Jul 21, 2017 at 02:45:03PM +0200, Florian Riehm wrote:
> > > On 06/25/17 23:47, Remi Locherer wrote:
> > > > Hi,
> > > >
> > > > ospfd does not react nicely when running "sh /etc/netstart if".
> > > >
> > > > This is because adding the same address again do an interface results
> > > > in RTM_DELADDR and RTM_NEWADDR. ospfd handles the former but the later.
> > > > If this happens ospfd says "interface vether0:192.168.250.1 gone".
> > > > Adjacencies on that interface are down and ospfd can not recover.
> > > >
> > > > The below patch adds IMSG_IFADDRADD to deal with that. With it ospfd
> > > > logs the following after "ifconfig vether0 192.168.250.1/24" (same
> > > > address
> > > > as active before):
> > > >
> > >
> > > Hi Remi,
> > >
> > > thanks for your report and your patch.
> > > I think it is the right approach, but unfortunately it doesn't work in my
> > > setup.
> > > If I run 'sh /etc/netstart vio1' vio1 goes down and stays down.
> > > Please have a look to my config / logs. What is the differece between
> > > your and
> > > my test?
> >
> > I tested with an interface connected to stub network. Your output shows an
> > interface connected to a transit network. In my test setup I did not get the
> > error message: "if_join_group: error IP_ADD_MEMBERSHIP"
> >
> > I'll look into it and try to fix this.
>
> I could reproduce the behaviour you see with my patch when testing with
> vmm and vio interfaces. It looks like in the IFADDRDEL case the interface
> can not leave the multicast group.
>
> I do not see this when testing with vether, pair or vlan (over ix)
> interfaces. Could this be a bug with multicast handling in vio?
Today I did a test with an unpatched ospfd and a vio interface in vmm.
I started ospfd and waited till adjacency was up. Then I did
"ifconfig vio0 192.168.250.101/24" (same ip as set before).
The debug output from ospfd:
[...]
if_leave_group: error IP_DROP_MEMBERSHIP, interface vio0 address 224.0.0.5:
Can't assign requested address
if_leave_group: error IP_DROP_MEMBERSHIP, interface vio0 address 224.0.0.6:
Can't assign requested address
orig_rtr_lsa: area 0.0.0.0
orig_rtr_lsa: stub net, interface vio0
orig_rtr_lsa: stub net, interface vether0
if_act_elect: interface vio0 old dr 192.168.250.1 new dr 192.168.250.101, old
bdr 192.168.250.101 new bdr none
orig_rtr_lsa: area 0.0.0.0
[...]
Doing the same with a pair or vether interface does not produce the message
"if_leave_group: error IP_DROP_MEMBERSHIP ....".
My conclusion of this is that the error is problem with the vio driver and
not with my patch for ospfd.
Could we proceed with the proposed ospfd patch and attack the vio problem
separately?
> > > Beside that:
> > > - I would rename 'struct ifaddrdel' to 'struct ifaddr' and extend it. So
> > > we
> > > can use it in if_newaddr() and if_deladdr() and avoid 'struct
> > > ifaddrnew'.
> >
> > Makes sense.
>
> The below patch introduces struct ifaddr and removes struct ifaddrdel. It
> also introduces two additional debug messages in interface.c:
>
> if_act_reset: interface vether0 left group 224.0.0.5
> if_act_start: interface vether0 joined group 224.0.0.5
>
> I added those to see a bit better what's going on. But I would not commit
> them. They are now printed even if joining the group is not successful
> (the message before would tell it).
>
>
> Index: interface.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 interface.c
> --- interface.c 5 Dec 2015 12:20:13 -0000 1.81
> +++ interface.c 24 Jul 2017 14:55:58 -0000
> @@ -357,6 +357,8 @@ if_act_start(struct iface *iface)
> inet_aton(AllSPFRouters, &addr);
> if (if_join_group(iface, &addr))
> return (-1);
> + log_debug("if_act_start: interface %s joined group %s",
> + iface->name, AllSPFRouters);
> iface->state = IF_STA_POINTTOPOINT;
> break;
> case IF_TYPE_VIRTUALLINK:
> @@ -371,6 +373,8 @@ if_act_start(struct iface *iface)
> inet_aton(AllSPFRouters, &addr);
> if (if_join_group(iface, &addr))
> return (-1);
> + log_debug("if_act_start: interface %s joined group %s",
> + iface->name, AllSPFRouters);
> if (iface->priority == 0) {
> iface->state = IF_STA_DROTHER;
> } else {
> @@ -547,6 +551,8 @@ if_act_reset(struct iface *iface)
> /* try to cleanup */
> inet_aton(AllSPFRouters, &addr);
> if_leave_group(iface, &addr);
> + log_debug("if_act_reset: interface %s left group %s",
> + iface->name, AllSPFRouters);
> if (iface->state & IF_STA_DRORBDR) {
> inet_aton(AllDRouters, &addr);
> if_leave_group(iface, &addr);
> Index: kroute.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
> retrieving revision 1.107
> diff -u -p -r1.107 kroute.c
> --- kroute.c 27 Dec 2016 09:15:16 -0000 1.107
> +++ kroute.c 24 Jul 2017 14:55:58 -0000
> @@ -1046,6 +1046,7 @@ if_newaddr(u_short ifindex, struct socka
> {
> struct kif_node *kif;
> struct kif_addr *ka;
> + struct ifaddr ifn;
>
> if (ifa == NULL || ifa->sin_family != AF_INET)
> return;
> @@ -1066,6 +1067,12 @@ if_newaddr(u_short ifindex, struct socka
> ka->dstbrd.s_addr = INADDR_NONE;
>
> TAILQ_INSERT_TAIL(&kif->addrs, ka, entry);
> +
> + ifn.addr = ka->addr;
> + ifn.mask = ka->mask;
> + ifn.dst = ka->dstbrd;
> + ifn.ifindex = ifindex;
> + main_imsg_compose_ospfe(IMSG_IFADDRADD, 0, &ifn, sizeof(ifn));
> }
>
> void
> @@ -1074,7 +1081,7 @@ if_deladdr(u_short ifindex, struct socka
> {
> struct kif_node *kif;
> struct kif_addr *ka, *nka;
> - struct ifaddrdel ifc;
> + struct ifaddr ifc;
>
> if (ifa == NULL || ifa->sin_family != AF_INET)
> return;
> Index: ospfd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
> retrieving revision 1.97
> diff -u -p -r1.97 ospfd.h
> --- ospfd.h 24 Jan 2017 04:24:25 -0000 1.97
> +++ ospfd.h 24 Jul 2017 14:55:58 -0000
> @@ -132,6 +132,7 @@ enum imsg_type {
> IMSG_RECONF_REDIST,
> IMSG_RECONF_END,
> IMSG_DEMOTE,
> + IMSG_IFADDRADD,
> IMSG_IFADDRDEL
> };
>
> @@ -360,8 +361,10 @@ struct iface {
> u_int8_t passive;
> };
>
> -struct ifaddrdel {
> +struct ifaddr {
> struct in_addr addr;
> + struct in_addr mask;
> + struct in_addr dst;
> unsigned int ifindex;
> };
>
> Index: ospfe.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/ospfe.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 ospfe.c
> --- ospfe.c 24 Jan 2017 04:24:25 -0000 1.99
> +++ ospfe.c 24 Jul 2017 14:55:58 -0000
> @@ -278,7 +278,7 @@ ospfe_dispatch_main(int fd, short event,
> {
> static struct area *narea;
> static struct iface *niface;
> - struct ifaddrdel *ifc;
> + struct ifaddr *ifc, *ifn;
> struct imsg imsg;
> struct imsgev *iev = bula;
> struct imsgbuf *ibuf = &iev->ibuf;
> @@ -349,9 +349,31 @@ ospfe_dispatch_main(int fd, short event,
> }
> }
> break;
> + case IMSG_IFADDRADD:
> + if (imsg.hdr.len != IMSG_HEADER_SIZE +
> + sizeof(struct ifaddr))
> + fatalx("IFADDRADD imsg with wrong len");
> + ifn = imsg.data;
> +
> + LIST_FOREACH(area, &oeconf->area_list, entry) {
> + LIST_FOREACH(iface, &area->iface_list, entry) {
> + if (ifn->ifindex == iface->ifindex &&
> + ifn->addr.s_addr ==
> + iface->addr.s_addr) {
> + iface->mask = ifn->mask;
> + iface->dst = ifn->dst;
> + if_fsm(iface, IF_EVT_UP);
> + log_warnx("interface %s:%s "
> + "returned", iface->name,
> + inet_ntoa(iface->addr));
> + break;
> + }
> + }
> + }
> + break;
> case IMSG_IFADDRDEL:
> if (imsg.hdr.len != IMSG_HEADER_SIZE +
> - sizeof(struct ifaddrdel))
> + sizeof(struct ifaddr))
> fatalx("IFADDRDEL imsg with wrong len");
> ifc = imsg.data;
>
