Bug 210726 - tcp connect() can return invalid EADDRINUSE (Eg: multiple jails with the same IP address)
Summary: tcp connect() can return invalid EADDRINUSE (Eg: multiple jails with the same...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mike Karels
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-30 19:12 UTC by aler
Modified: 2020-08-13 12:25 UTC (History)
12 users (show)

See Also:
koobs: mfc-stable12+
koobs: mfc-stable11-


Attachments
patch to fix incorrect EADDRINUSE (680 bytes, patch)
2016-07-01 15:05 UTC, aler
no flags Details | Diff
11.2-STABLE Patch for EADDRINUSE (668 bytes, patch)
2018-09-03 16:27 UTC, David King
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description aler 2016-06-30 19:12:01 UTC
Port autoselection on connect() without bind() (or with bind() with zero
sin_port) sometimes works wronly and gives already busy local port
number that will lead to EADDRINUSE on connection attempt. This all
happens when jails used.

How to fix:

src/sys/netinet/in_pcb.c, in_pcb_lport() function

calls to in_pcblookup_local() should have last argument NULL, not cred

that's because here we are not about getting some access but about
avoiding port number conflicts, so all inpcb's should be taken in account


This all applies to FreeBSD 9.x, 10.x and HEAD (possibly older versions too).

Patch:

diff -ur src-svn/10.2/sys/netinet/in_pcb.c src/sys/netinet/in_pcb.c
--- src-svn/10.2/sys/netinet/in_pcb.c   2016-03-15 22:58:38.088511000 +0300
+++ src/sys/netinet/in_pcb.c    2016-05-20 14:51:43.340568000 +0300
@@ -452,7 +452,7 @@
 #endif
 #ifdef INET
                        tmpinp = in_pcblookup_local(pcbinfo, laddr,
-                           lport, lookupflags, cred);
+                           lport, lookupflags, NULL /*cred*/);
 #endif
        } while (tmpinp != NULL);
Comment 1 aler 2016-07-01 15:05:05 UTC
Created attachment 172001 [details]
patch to fix incorrect EADDRINUSE
Comment 2 Jilles Tjoelker freebsd_committer 2016-07-03 12:08:19 UTC
Although this change does not seem wrong, it seems necessary only when using the same IP address in different jails, or in a jail and the outer system. Is that the case?

Also, if the change applies to the in_pcblookup_local() call, it also applies to the in6_pcblookup_local() call above it.
Comment 3 aler 2016-07-03 16:13:33 UTC
(In reply to Jilles Tjoelker from comment #2)

Yes, got that error with multiple jails on the same IP.

And most likely yes, it applies to in6 branch too (uploaded patch above contains in6).
Comment 4 aler 2017-06-19 12:56:32 UTC
Can this be processed more fast to make it patched in upcoming 10.4?
Comment 5 dev 2018-03-08 21:38:02 UTC
Bump.
Comment 6 David King 2018-08-15 10:56:10 UTC
bumping as well :)
Comment 7 David King 2018-09-03 16:27:10 UTC
Created attachment 196816 [details]
11.2-STABLE Patch for EADDRINUSE
Comment 8 David King 2018-09-03 16:29:01 UTC
I've tested this patch on 11.1 and 11.2 and it solves the EADDRINUSE error we were seeing with multiple jails  with the same IP address

Have also been running jails with different IP's on the same box and didn't see any new errors at all

Attached the updated patch for 11.2-STABLE

if someone could try to merge this back it would be great!
Comment 9 Sean Bruno freebsd_committer 2018-09-06 15:20:44 UTC
The analysis of this bug and the patch provided seem correct to my eye.  Adding some folks who have been poking around in the network stack at the moment to advise.
Comment 10 Bjoern A. Zeeb freebsd_committer 2018-09-06 15:30:11 UTC
Stupid question:  can someone write the short test case for this to reproduce the problem?
Comment 11 David King 2018-09-06 15:39:23 UTC
Not a script as such, however, to reproduce i create 2 jails with same IP,

reduce the ephemeral port range to increase the probability of hitting the issue
net.inet.ip.portrange.hifirst: 51000
net.inet.ip.portrange.hilast: 5200

and run the following script in each jail and compare the logs, i'm sure there is an easy way to do this with shell, but I couldn't get the low level error with curl

#!/usr/local/bin/python

import requests
import logging
import threading

logging.basicConfig(filename='/tmp/python.log',level=logging.INFO)

def makerequests():
  try:
    r = requests.get(<% choose a local URL %>)
  except Exception as e:
    logging.info(e)

for i in range(0,10000):
  t = threading.Thread(target=makerequests)
  t.start()
Comment 12 David King 2018-09-06 15:40:41 UTC
missed a zero on net.inet.ip.portrange.hilast: 52000
Comment 13 Michael Tuexen freebsd_committer 2018-09-06 15:58:45 UTC
What is the semantic of the last argument of in_pcblookup_local()?
Comment 14 Bjoern A. Zeeb freebsd_committer 2018-09-06 16:45:52 UTC
Ok,

trying to summarise to get the exact case right as the suggested patch looks not quite right.  There are too many (corner) cases to consider.

two jails, same single IP address.

In each jail a program tries to establish a connection and has bound a local source address or not, but must not have bound a local port number.

On connect() to a local or remote address and port there may be a case that two applications in two different jails get an implicit bind to the same local port number out of which one succeeds and one fails?  So one connect call succeeds and one fails?


It is not yet fully understood if the same could possibly happen between the base system and a jail, in which case it is assumed that the connect() inside the jail would be the one always failing?
Comment 15 Bjoern A. Zeeb freebsd_committer 2018-09-06 16:46:31 UTC
I'll take the bug for now at least
Comment 16 aler 2018-12-10 12:15:52 UTC
(In reply to Bjoern A. Zeeb from comment #14)

> trying to summarise to get the exact case right as the suggested patch looks not quite right

I don't understand what's wrong with the patch.

> There are too many (corner) cases to consider.

All of them are covered by that single check: busy ports should be detected by system-wide used ports list, not jailed used ports list.

> In each jail a program tries to establish a connection and has bound a local source address or not, but must not have bound a local port number.

Yes.

> On connect() to a local or remote address and port there may be a case that two applications in two different jails get an implicit bind to the same local port number out of which one succeeds and one fails?  So one connect call succeeds and one fails?

No. Second implicit bind fails itself (searching "non-busy" port - found actually busy port - try to bind - fail) and throws a error through connect() that tried it.


> It is not yet fully understood if the same could possibly happen between the base system and a jail, in which case it is assumed that the connect() inside the jail would be the one always failing?

Yes, it can, when the implicit bind happens in jail. Already busy port can be anywhere outside that jail, so it may be in other jail on in host system.
Comment 17 aler 2019-04-08 21:59:21 UTC
bump
Comment 18 aler 2019-10-12 14:33:22 UTC
This bug is very easy to fix, why not to do it?
Comment 19 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-13 22:16:30 UTC
@All Please don't *solely* bump issues, unless providing additional information beyond what already exists.

^Triage: 

- Assignee timeout (> 1 year), reset assignee

@Reporter: Can you please provide:

- /etc/rc.conf / /etc/jail.conf configuration/setup that reproduces the issue (as an attachment)

@Anyone

An updated patch against CURRENT/head would be handy (please obsolete existing patches when attaching the updated one)
Comment 20 Mike Karels freebsd_committer 2020-08-13 02:32:28 UTC
I am reasonably sure that this bug is fixed in head and 12-STABLE.  The change that fixed it (r361228) has not been backported to 11-STABLE.  Can anyone verify that the bug does not exist in head or 12-STABLE?
Comment 21 Kubilay Kocak freebsd_committer freebsd_triage 2020-08-13 08:16:26 UTC
^Triage: 

 - Assign to committer that (reportedly) resolved
 - Track merges / non-merges

@Mike Do you have the reference stable/12 merge revision handy? Just add a comment with 'base r<revision>' to have it auto link
Comment 22 Mike Karels freebsd_committer 2020-08-13 12:21:32 UTC
base r362446 in stable/12 should have the fix.
Comment 23 Kubilay Kocak freebsd_committer freebsd_triage 2020-08-13 12:25:38 UTC
^Triage: Close resolved.

@Reporter, If the issue is still reproducible after updating to a FreeBSD version/revision that contain the fix, please re-open this issue with additional information