Otherwise we have race condition to user land:
 1. process A changes IP address
 2. kernel sends RTM_NEWADDR
 3. process B gets notification
 4. process B tries to bind() to new IP but that fails with
EADDRNOTAVAIL because FIB is not yet updated and inet_addr_type() in
inet_bind() does not recognize the IP as local
 5. kernel calls inetaddr_chain notifiers which updates FIB

IPv6 side seems to handle the notifications properly: bind()
immediately after RTM_NEWADDR succeeds as expected. This is because
ipv6_chk_addr() uses inet6_addr_lst which is updated before address
notification.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
 net/ipv4/af_inet.c  |    9 +++++++++
 net/ipv6/af_inet6.c |    4 +++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6a1100c..21200e4 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -466,6 +466,15 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (addr_len < sizeof(struct sockaddr_in))
 		goto out;
 
+	/* Acquire rtnl_lock to synchronize with possible simultaneous
+	 * IP-address changes. This is needed because when RTM_NEWADDR
+	 * is sent the new IP is not yet in FIB, but alas inet_addr_type
+	 * checks the address type using FIB. Acquiring rtnl lock once
+	 * makse sure that any address for which RTM_NEWADDR was sent
+	 * earlier exists also in FIB. */
+	rtnl_lock();
+	rtnl_unlock();
+
 	chk_addr_ret = inet_addr_type(sock_net(sk), addr->sin_addr.s_addr);
 
 	/* Not specified by any standard per-se, however it breaks too
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 56b9bf2..6fc37f4 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -300,7 +300,9 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 			goto out;
 		}
 
-		/* Reproduce AF_INET checks to make the bindings consitant */
+		/* Reproduce AF_INET checks to make the bindings consistent ...
[ message continues ]

You must be kidding ?

Really, this is a hot path...



--

[ message continues ]

Is inet_bind() called from non-userland context? If yes, then this is a
bad idea. Otherwise I don't think it's that hot path...

The other idea of doing notifier calls before RTM_NEWADDR sending is
worse because it changes ordering of userland visible netlink notifications.

This looked like the easiest way out. If this is unacceptable, I guess
we are left with changing inet_addr_type() to not use FIB.

Or is there better ideas?


--

[ message continues ]

From: Timo Teräs <timo.teras@iki.fi>

It is.
--

[ message continues ]

Yet, almost immediately after that there is lock_sock() which can also
sleep. How does that work then?

--

[ message continues ]

From: Timo Teräs <timo.teras@iki.fi>

RTNL interlocks globally with a heavy primitive, a mutex, lock_sock()
grabs a spinlcok which is local to the socket's context.

So if we have 100,000 sockets binding we'll suck with you're change
whereas the lock_sock() does not cause that problem.

Is this so difficult for you to comprehend?
--

[ message continues ]

I was confused with Dave's original reply "It is." as referring to that
inet_bind() can get called from non-userland context. But apparently you
just meant that "It is (bad idea regardless)."

I thought the problem was possible sleeping, and not contention. Which
became very obvious from Eric's example. I didn't realize that many do
bind()/recv()/send() as general workload.

Sorry for not seeing the obvious.

This is the third time asking, what would be a good way to fix the
problem described in the original commit log?

Changing RTM_NEWADDR after FIB update would break Netlink event
ordering. And this breaks performance. I can't really use RTN_LOCAL
RTM_NEWROUTE events since (at least IPv6 side) has incorrect ifindex.

Should inet_addr_type() be rewritten to not use FIB lookups?
--

[ message continues ]

From: Timo Teräs <timo.teras@iki.fi>

I kept your report in my inbox backlog and intended to think about
it as time permitted.

As the merge window has just openned up, for me time will not be
"permitted" for some time.
--

[ message continues ]

Fair enough. Just getting multiple "does not work" without single
mention of "will think about this later" felt frustrating.

I have one more alternative: Would it be acceptable if we did
rtnl_lock()/rtnl_unlock() only if inet_addr_type() earlier said the
address is unacceptable for binding. And then retry inet_addr_type(). Or
do you think that the error case EADDRNOTAVAIL is a hot path too?
--

[ message continues ]

Otherwise we have race condition to user land:
 1. process A: changes IP address
 2. process A: kernel sends RTM_NEWADDR (and schedules out)
 3. process B: gets notification
 4. process B: tries to bind() to new IP, but fails with EADDRNOTAVAIL
      because FIB is not yet updated and inet_addr_type() in inet_bind()
      does not recognize the IP as local
 5. process A: calls inetaddr_chain notifiers which updates FIB

Fix the error path to synchronize with configuration changes and retry
the address type check.

IPv6 side seems to handle the notifications properly: bind() immediately
after RTM_NEWADDR succeeds as expected.  This is because ipv6_chk_addr()
uses inet6_addr_lst which is updated before address notification.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
Since there was no reply to my question if this is ok, I interpreted it
as "maybe". So here's the code for review. Hopefully this helps determining
if this is an acceptable fix.

 net/ipv4/af_inet.c  |   18 ++++++++++++++++--
 net/ipv6/af_inet6.c |   13 ++++++++++---
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6a1100c..013ab11 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -481,8 +481,22 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	    addr->sin_addr.s_addr != htonl(INADDR_ANY) &&
 	    chk_addr_ret != RTN_LOCAL &&
 	    chk_addr_ret != RTN_MULTICAST &&
-	    chk_addr_ret != RTN_BROADCAST)
-		goto out;
+	    chk_addr_ret != RTN_BROADCAST) {
+		/* inet_addr_type() does a FIB lookup to check the
+		 * address type. Since FIB is updated after sending
+		 * RTM_NEWADDR notification, an application may end up
+		 * doing bind() before the FIB is updated. To avoid
+		 * returning a false negative, wait for possible ongoing
+		 * address changes to finish by acquiring rtnl lock and
+		 * retry the address type lookup. */
+		rtnl_lock();
+		rtnl_unlock();
+		chk_addr_ret = ...
[ message continues ]

Just say : no

Really Timo, this problem must get another fix.

I understand you need an urgent fix, you can use your patch in the
meantime, of course ;)



--

[ message continues ]

Fine. I figured you might feel this way. My final idea to fix this, is
to modify inet_addr_type() do the address type check using the ifa
lists. This probably involves making ifa lists rcu (did not take close
look on how ifa lists work in current ipv4 code). This is basically how
ipv6 side works. I'm not too sure how to go on with this, so I'll wait
up until someone more qualified gets the time to look at this.

I think for my immediate needs I might be able to get away with using
the kludge patch, enabling non-local binding or fixing my userland code
to listen RTM_NEWROUTE/RTN_LOCAL events (though, IPv6 link-local
addresses are special and would need RTM_NEWADDR handling still to get
the real device's ifindex).

Btw. why do IPv6 RTN_LOCAL routes have loopback interface as dst instead
of the real device? IPv4 RTN_LOCAL routes seem to have the real device
so this looks inconsistent at first sight. I guess IPv6 requires this
for a bunch of other things. This way it's just not really possible to
get link-local IPv6 routes.
--

[ message continues ]

I am not sure I understand your question. Maybe my answer wont be clear.

rtnl_lock() can take ages on some setups, because its using one single
and shared mutex. Its use should be restricted to administrative
purposes.

bind() is a pretty hot path on many workloads, this is hardly what we
call an administrative task.

lock_sock() uses a per socket lock, and it is a _bit_ more scalable, you
can have 4096 cpus all using bind()/recv()/send()/... at the same time,
it just works.



--

[ message continues ]