| Message ID | 1287655930-16879-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Le jeudi 21 octobre 2010 à 13:12 +0300, Timo Teräs a écrit : > 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(); You must be kidding ? Really, this is a hot path... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2010 01:25 PM, Eric Dumazet wrote: > Le jeudi 21 octobre 2010 à 13:12 +0300, Timo Teräs a écrit : >> 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(); > > You must be kidding ? > > Really, this is a hot path... 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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Timo Teräs <timo.teras@iki.fi> Date: Thu, 21 Oct 2010 13:41:37 +0300 > 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... It is. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2010 01:50 PM, David Miller wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Thu, 21 Oct 2010 13:41:37 +0300 > >> 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... > > It is. Yet, almost immediately after that there is lock_sock() which can also sleep. How does that work then? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Timo Teräs <timo.teras@iki.fi> Date: Thu, 21 Oct 2010 13:58:08 +0300 > On 10/21/2010 01:50 PM, David Miller wrote: >> From: Timo Teräs <timo.teras@iki.fi> >> Date: Thu, 21 Oct 2010 13:41:37 +0300 >> >>> 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... >> >> It is. > > Yet, almost immediately after that there is lock_sock() which can also > sleep. How does that work then? 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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le jeudi 21 octobre 2010 à 13:58 +0300, Timo Teräs a écrit : > On 10/21/2010 01:50 PM, David Miller wrote: > > From: Timo Teräs <timo.teras@iki.fi> > > Date: Thu, 21 Oct 2010 13:41:37 +0300 > > > >> 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... > > > > It is. > > Yet, almost immediately after that there is lock_sock() which can also > sleep. How does that work then? > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2010 02:03 PM, David Miller wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Thu, 21 Oct 2010 13:58:08 +0300 > >> On 10/21/2010 01:50 PM, David Miller wrote: >>> From: Timo Teräs <timo.teras@iki.fi> >>> Date: Thu, 21 Oct 2010 13:41:37 +0300 >>> >>>> 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... >>> >>> It is. >> >> Yet, almost immediately after that there is lock_sock() which can also >> sleep. How does that work then? > > 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? 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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Timo Teräs <timo.teras@iki.fi> Date: Thu, 21 Oct 2010 14:29:22 +0300 > This is the third time asking, what would be a good way to fix the > problem described in the original commit log? 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2010 02:34 PM, David Miller wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Thu, 21 Oct 2010 14:29:22 +0300 > >> This is the third time asking, what would be a good way to fix the >> problem described in the original commit log? > > 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. 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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ + rtnl_lock(); + rtnl_unlock(); v4addr = addr->sin6_addr.s6_addr32[3]; chk_addr_ret = inet_addr_type(net, v4addr); if (!sysctl_ip_nonlocal_bind &&
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(-)