Re: Detached address check

[Roy Marples <roy%marples.name@localhost>]

On 21/11/2017 10:02, Robert Swindells wrote:
> On Tue, November 21, 2017 9:26 am, Roy Marples wrote:
> > On 18/11/2017 13:37, Robert Swindells wrote:
> > > On Fri, November 17, 2017 4:57 pm, Robert Swindells wrote:
> > > > I feel that the changes made to check whether a sending address is
> > > > valid
> > > > are too strict.
> > > >
> > > > The check means that an attempt to connect from the address of an
> > > > interface that is marked detached to the same address will fail even
> > > > though there is still a route for this address to the loopback device.
> > > >
> > > > The change was introduced in:
> > > >
> > > > <http://mail-index.netbsd.org/source-changes/2016/09/15/msg077726.html>
> > >
> > > To clarify, I expect to be able to do:
> > >
> > > # ifconfig wm0 inet 192.168.0.1 netmask 0xffffff00 up
> > > # ping 192.168.0.1
> > >
> > > and for it to work every time, not just when wm0 has detected a
> > > carrier. In the traditional usage, the packets don't go through wm0
> > > anyway they go through lo0.
> >
> > Aside from ping, what else do you really want?
>
> I want all network programs to work.
>
> > > The RFC 4862 referenced in the commit is for IPv6, I don't think that
> > > IPv4 behaviour should change too.
> > >
> > > I'm not even convinced that I want this behaviour for IPv6, I would
> > > prefer that addresses set using ifconfig(8) should stay valid until
> > > I change them.
> >
> > What would you actually do with them though?
> > You have localhost - 127.0.0.1 which works just fine.
>
> I shouldn't need to use localhost to connect to the same machine, that
> isn't the way that UNIX networking has traditionally worked, there
> wouldn't be any need for the route from each interface address to lo0
> if we adopt your new programming model.

That's a fair comment.
I've attached a patch which should fix this for IPv4 at least.
I'll work on the IPv6 side later.

> I feel that setting DETACHED/TENTATIVE based on link status should
> be removed from IPv4 altogether. I have commented it out in my tree
> and my machine is working again.
>
> For IPv6, I would enable it with a sysctl whose value is only set to
> true when ipv6mode="autohost".

So you don't want in kernel Duplicate Address Detection?
We already have sysctls to set the DaD packets sent to zero which 
effectively addresses the TENTATIVE part. The above patch handles 
DETACHED (well, for IPv4 anyway).

> If this change makes it into NetBSD-8 then I think you are going to
> spend a lot of time answering user questions.

Well, the change has been in -current for over 2.5 years and I've not 
spent much time either answering question or fixing bugs with it so I 
disagree with that somewhat sweeping statement.

Roy

Index: sys/netinet/ip_input.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/ip_input.c,v
retrieving revision 1.362
diff -u -p -r1.362 ip_input.c
--- sys/netinet/ip_input.c	17 Nov 2017 07:37:12 -0000	1.362
+++ sys/netinet/ip_input.c	22 Nov 2017 11:33:15 -0000
@@ -343,7 +343,7 @@ ip_init(void)
 static struct in_ifaddr *
 ip_match_our_address(struct ifnet *ifp, struct ip *ip, int *downmatch)
 {
-	struct in_ifaddr *ia = NULL;
+	struct in_ifaddr *ia = NULL, *detached = NULL;
 	int checkif;
 
 	/*
@@ -371,15 +371,20 @@ ip_match_our_address(struct ifnet *ifp, 
 				continue;
 			if (checkif && ia->ia_ifp != ifp)
 				continue;
-			if ((ia->ia_ifp->if_flags & IFF_UP) != 0 &&
-			    (ia->ia4_flags & IN_IFF_DETACHED) == 0)
-				break;
-			else
+			if ((ia->ia_ifp->if_flags & IFF_UP) == 0) {
 				(*downmatch)++;
+				continue;
+			}
+			if (ia->ia4_flags & IN_IFF_DETACHED) {
+				if (detached == NULL)
+					detached = ia;
+				continue;
+			}
+			return ia;
 		}
 	}
 
-	return ia;
+	return detached;
 }
 
 static struct in_ifaddr *
@@ -663,14 +668,20 @@ ip_input(struct mbuf *m)
 	downmatch = 0;
 	s = pserialize_read_enter();
 	ia = ip_match_our_address(ifp, ip, &downmatch);
-	if (ia != NULL) {
+	if (ia != NULL &&
+	    (!(ia->ia4_flags & IN_IFF_DETACHED) ||
+	    ifp->if_flags & IFF_LOOPBACK))
+	{
 		pserialize_read_exit(s);
 		goto ours;
 	}
 
 	if (ifp->if_flags & IFF_BROADCAST) {
 		ia = ip_match_our_address_broadcast(ifp, ip);
-		if (ia != NULL) {
+		if (ia != NULL &&
+		    (!(ia->ia4_flags & IN_IFF_DETACHED) ||
+		    ifp->if_flags & IFF_LOOPBACK))
+		{
 			pserialize_read_exit(s);
 			goto ours;
 		}