Restore using reserve ports for client connections

PATCH In review Steve Dickson 4 participants 10 messages Apr 10, 2018 – Apr 11, 2018

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2018-04-10 21:30 UTC #0 lore raw

PATCH 1/2 Restore using reserve ports for client connections

Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.

For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Signed-off-by Steve Dickson <[email protected]>

---
 src/clnt_generic.c | 3 +--
 src/rpc_soc.c      | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

src/clnt_generic.c +1 -2

index e5a314f..774292b 100644
@@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig *nconf,
 		servtype = nconf->nc_semantics;
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
-		if (__binddynport(fd) == -1)
-			goto err;
+		bindresvport(fd, NULL);
 	} else {
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;

src/rpc_soc.c +1 -2

index af6c482..f32a27c 100644
@@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {

Signature · 1 line

2.14.3

Steve Dickson

author

2018-04-10 21:30 UTC #1 lore raw

PATCH 2/2 Restore creating listening connection to server connections

Commit 46e04a73 changed svc_com_create() to avoid using reserve ports but removed the listen() call which could break, undocumented expectations, of legacy server apps.

Reserve ports are not needed for listening ports, so the new __binddynport() is used to avoid squatting on ports that are assigned to other network services, but it is important to say backwards compatible with the legacy glibc code so the the listen() call is added back.

Signed-off-by Steve Dickson <[email protected]>

---
 src/rpc_soc.c | 7 +++++++
 1 file changed, 7 insertions(+)

src/rpc_soc.c +7 -0

index f32a27c..cf15216 100644
@@ -331,6 +331,13 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd = TRUE;
 	}
 
+	if (__binddynport(fd) == -1) {
+		if (madefd)
+			(void)close(fd);
+		(void) syslog(LOG_ERR,
+			"svc%s_create: could not bind connection: %m", netid);
+	}
+	listen(fd, SOMAXCONN);
 	svc = svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
 	(void) freenetconfigent(nconf);
 	if (svc == NULL) {

Signature · 1 line

2.14.3

Chuck Lever

2018-04-10 22:29 UTC #2 lore raw

Re: [PATCH 2/2] Restore creating listening connection to server connections

Quoted 7 lines · Steve Dickson

=20
Commit 46e04a73 changed svc_com_create() to avoid using reserve ports but removed the listen() call which could break, undocumented expectations, of legacy

Quoted 8 lines

server apps.
=20
Reserve ports are not needed for listening ports, so the new __binddynport() is used to avoid squatting on ports that are assigned to other network services, but it is important to say backwards compatible with the legacy glibc code so the the listen() call is added back.

svc_tli_create should already perform a call to __binddynport() and to listen(2). That's why those calls were removed from svc_com_create().

222 if (madefd || !__rpc_sockisbound(fd)) {
223 if (bindaddr =3D=3D NULL) {
224 if (__binddynport(fd) =3D=3D -1) {
225 warnx(
226 "svc_tli_create: could not bind to =
anonymous port");
227 goto freedata;
228 }
229 listen(fd, SOMAXCONN);
230 } else {
231 if (bind(fd,
232 (struct sockaddr *)bindaddr->addr.buf,
233 (socklen_t)si.si_alen) < 0) {
234 warnx(
235 "svc_tli_create: could not bind to requested =
address");
236 goto freedata;
237 }
238 listen(fd, (int)bindaddr->qlen);
239 }
240 =20
241 }

I expect that __rpc_sockisbound(fd) should be false if svc_tli_create() is called from svc_com_create(). Can you tell why it isn't?

Quoted 13 lines

Signed-off-by Steve Dickson <[email protected]>
---
src/rpc_soc.c | 7 +++++++
1 file changed, 7 insertions(+)
=20
src/rpc_soc.c +0 -0
index f32a27c..cf15216 100644
@@ -331,6 +331,13 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd =3D TRUE;
 	}
=20

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid)

Quoted 5 lines

src/rpc_soc.c +5 -0

+	if (__binddynport(fd) =3D=3D -1) {
+		if (madefd)
+			(void)close(fd);
+		(void) syslog(LOG_ERR,
+			"svc%s_create: could not bind connection: %m", =

netid);

Quoted 10 lines

src/rpc_soc.c +2 -0
+	}
+	listen(fd, SOMAXCONN);
	svc =3D svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
	(void) freenetconfigent(nconf);
	if (svc =3D=3D NULL) {
--=20
2.14.3
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

Steve Dickson

author

2018-04-11 13:39 UTC #3 lore raw

Re: [PATCH 2/2] Restore creating listening connection to server connections

Hey,

Quoted 16 lines · Chuck Lever

Quoted 9 lines · Steve Dickson
Commit 46e04a73 changed svc_com_create() to avoid using reserve ports but removed the listen() call which could break, undocumented expectations, of legacy server apps.
Quoted 6 lines
Reserve ports are not needed for listening ports, so the new __binddynport() is used to avoid squatting on ports that are assigned to other network services, but it is important to say backwards compatible with the legacy glibc code so the the listen() call is added back.

Quoted 4 lines

svc_tli_create should already perform a call to __binddynport() and to listen(2). That's why those calls were removed from svc_com_create().

Fair enough... I did miss that call to svc_tli_create().

It appears the callers of svc_com_create() are no longer used at least in nfs-utils. It appears to be the old SUN RPC API and we using the TIRPC API.

steved.

Chuck Lever

2018-04-10 22:17 UTC #4 lore raw

In reply to Steve Dickson

Quoted 11 lines · Steve Dickson

=20
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.
=20
For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Hi Steve-

Which legacy apps use clnt_tli_create and require this behavior? There is no backwards compatibility requirement for this API.

src/clnt_generic.c clnt_tli_create(int fd, const struct netconfig =

Quoted 11 lines

Signed-off-by Steve Dickson <[email protected]>
---
src/clnt_generic.c | 3 +--
src/rpc_soc.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
=20
src/clnt_generic.c +0 -0
index e5a314f..774292b 100644
@@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig =

*nconf,

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, sendsz, =

Quoted 14 lines

src/clnt_generic.c +1 -2

 		servtype =3D nconf->nc_semantics;
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
-		if (__binddynport(fd) =3D=3D -1)
-			goto err;
+		bindresvport(fd, NULL);
 	} else {
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;

src/rpc_soc.c +0 -0

index af6c482..f32a27c 100644
@@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, =

recvsz, tp, flags)

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, sendsz, = · line 147

Quoted 1 line

src/rpc_soc.c +0 -0

 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =

sockaddr_in);

Quoted 8 lines

src/rpc_soc.c +0 -0

 	bindaddr.buf =3D raddr;

=20

src/rpc_soc.c +1 -2

-	if (__binddynport(fd) =3D=3D -1)
-		goto err;
+	bindresvport(fd, NULL);

	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
				sendsz, recvsz);
	if (cl) {

Quoted 5 lines

--=20
2.14.3
=20
=20
=

--------------------------------------------------------------------------=
----

Quoted 6 lines

Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Libtirpc-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever

Steve Dickson

author

2018-04-11 12:34 UTC #5 lore raw

Hey,

Quoted 8 lines · Chuck Lever

Quoted 8 lines · Steve Dickson
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.

Quoted 7 lines

Quoted 3 lines
For certain legacy apps, the client has to used
reserve port to be able to communicate with its
server so using of reserve ports is restored.
Hi Steve-
Which legacy apps use clnt_tli_create and require this behavior?

yphelper, yppush and ypxfr via the clnt_create() call. At least that's all I have found so far.

Quoted 1 line

There is no backwards compatibility requirement for this API.

Well unlike the server side, the client will not be squatting on these port for an absorbent amount time. 99% of the time they are UDP connection so the ports are immediately reusable, unlike TCP connection that get stuck in TIME_WAIT and there is a requirement for reserve port be used to talk to the server.

So I'm feeling pretty strong that there is no problem at all with the client using reserve ports for their short lived connection. Not using them would cause more problems as we have already seen.

steved.

Quoted 53 lines

Quoted 47 lines

Signed-off-by Steve Dickson <[email protected]>

---
src/clnt_generic.c | 3 +--
src/rpc_soc.c | 3 +--

2 files changed, 2 insertions(+), 4 deletions(-)

src/clnt_generic.c +1 -2

index e5a314f..774292b 100644
@@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig *nconf,
 		servtype = nconf->nc_semantics;
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
-		if (__binddynport(fd) == -1)
-			goto err;
+		bindresvport(fd, NULL);
 	} else {
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;

src/rpc_soc.c +1 -2

index af6c482..f32a27c 100644
@@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {

Signature · 10 lines

2.14.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Libtirpc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever

Thorsten Kukuk

2018-04-11 12:58 UTC #6 lore raw

Quoted 16 lines · Steve Dickson

Hey,
Quoted 12 lines · Chuck Lever
Quoted 11 lines · Steve Dickson
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.
For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Quoted 5 lines

Quoted 3 lines
Hi Steve-
Which legacy apps use clnt_tli_create and require this behavior?
yphelper, yppush and ypxfr via the clnt_create() call. At least that's all I have found so far.

Yes, and I need to change that like Solaris is doing it, only a question of time on my side :(

Thorsten

Signature · 3 lines

Thorsten Kukuk, Distinguished Engineer, Senior Architect SLES & CaaSP
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Steve Dickson

author

2018-04-11 14:07 UTC #7 lore raw

Quoted 18 lines · Thorsten Kukuk

Quoted 16 lines · Steve Dickson
Hey,
Quoted 12 lines · Chuck Lever
Quoted 11 lines · Steve Dickson
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.
For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Quoted 6 lines

Quoted 5 lines
Quoted 3 lines
Hi Steve-
Which legacy apps use clnt_tli_create and require this behavior?
yphelper, yppush and ypxfr via the clnt_create() call. At least that's all I have found so far.

Add rpcinfo to this list... Its used in the remote call code.

steved.

Quoted 5 lines

Yes, and I need to change that like Solaris is doing it, only a question of time on my side :(
Thorsten

Chuck Lever

2018-04-11 14:09 UTC #8 lore raw

Quoted 16 lines · Steve Dickson

=20
=20
=20
Quoted 9 lines · Thorsten Kukuk
On Wed, Apr 11, Steve Dickson wrote:
=20
Quoted 7 lines
=20
Hey,
=20
Quoted 3 lines · Chuck Lever
=20
=20
Quoted 1 line
On Apr 10, 2018, at 3:30 PM, Steve Dickson <[email protected]> =

Quoted 8 lines · wrote:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
=20
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.
=20
For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Quoted 8 lines

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
=20
Hi Steve-
=20
Which legacy apps use clnt_tli_create and require this behavior?
yphelper, yppush and ypxfr via the clnt_create() call. At least that's all I have found so far.
=20
Add rpcinfo to this list... Its used in the remote call code.

clnt_create(3) is used there, but does it require a reserved port?

Quoted 9 lines

steved.
=20
Quoted 5 lines
Yes, and I need to change that like Solaris is doing it, only a question of time on my side :(
=20
Thorsten
=20
=20
=

--------------------------------------------------------------------------=
----

Quoted 6 lines

Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Libtirpc-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever
[email protected]

Chuck Lever

2018-04-11 14:09 UTC #9 lore raw

In reply to Steve Dickson

Quoted 11 lines · Steve Dickson

=20
=20
Hey,
=20
Quoted 3 lines · Chuck Lever
=20
=20
Quoted 1 line
On Apr 10, 2018, at 3:30 PM, Steve Dickson <[email protected]> =

Quoted 8 lines · wrote:

Quoted 8 lines
Quoted 8 lines
=20
Commit 46e04a73 changed both clnt_com_create() and clnt_tli_create() to avoid using reserve ports when creating connection to the server.
=20
For certain legacy apps, the client has to used reserve port to be able to communicate with its server so using of reserve ports is restored.

Quoted 6 lines

Quoted 4 lines
=20
Hi Steve-
=20
Which legacy apps use clnt_tli_create and require this behavior?
yphelper, yppush and ypxfr via the clnt_create() call. At least that's all I have found so far.

clnt_create(3) unfortunately appears to be in glibc. So fair enough, but your patch description really does need to provide these details and examples, please.

Quoted 7 lines

Quoted 1 line
There is no backwards compatibility requirement for this API.
Well unlike the server side, the client will not be squatting on these port for an absorbent amount time. 99% of the time they are UDP connection so the ports are immediately reusable, unlike TCP connection that get stuck in TIME_WAIT and there is a requirement for reserve port be used to talk to the server.

I do not agree that this is a harmless default.

There is no guarantee that the RPC consumer is going to release the client immediately. That's the problem with continuing to use bindresvport(3) here: many callers might free the CLNT immediately, but some don't.

We have examples of callers -- in our own administrative tools -- that start up as root on purpose so they can create a long-lived client.

Some of them don't need to do that, and thus can consume a reserved port for no good reason.

And we know that a few applications do use TCP, even for short-lived clients, and that has a bad side-effect.

Look at mount.nfs, for example. It runs as root. If it uses TCP to contact the server's rpcbind and mountd, and uses these basic library client APIs, that leaves two reserved ports in TIME_WAIT for two minutes. A storm of NFS mount operations can easily consume all the reserved ports for a few minutes.

Maybe mount.nfs doesn't do that, but some other applications might. There's nothing in libtirpc to prevent it.

Quoted 3 lines

So I'm feeling pretty strong that there is no problem at all with the client using reserve ports for their short lived connection.

There is no guarantee the client will be short-lived, and in the common case a reserved port is an unneeded use of a scarce system resource. This is bad default behavior.

Quoted 2 lines

Not using them would cause more problems
as we have already seen.

This is not "causing problems." These legacy apps are using an undocumented behavior. I will note that we are basically fixing libtirpc to address an application where patches are available for this to be done correctly.

The best that can be done for the moment is try to reduce the likelihood that bindresvport(3) will land on a well-known port, without reducing the reliability of success. I will look into it.

Quoted 4 lines

steved.
=20
Quoted 2 lines
=20
=20

Quoted 11 lines

Quoted 11 lines
src/clnt_generic.c clnt_tli_create(int fd, const struct netconfig =
Quoted 11 lines
Signed-off-by Steve Dickson <[email protected]>
---
src/clnt_generic.c | 3 +--
src/rpc_soc.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
=20
src/clnt_generic.c +0 -0
index e5a314f..774292b 100644
@@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig =

*nconf,

Quoted 6 lines

src/clnt_generic.c +1 -2

 		servtype =3D nconf->nc_semantics;
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
-		if (__binddynport(fd) =3D=3D -1)
-			goto err;
+		bindresvport(fd, NULL);

Quoted 8 lines

Quoted 8 lines
src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, =
Quoted 8 lines
src/clnt_generic.c +0 -0
 	} else {
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
src/rpc_soc.c +0 -0
index af6c482..f32a27c 100644
@@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, =

sendsz, recvsz, tp, flags)

Quoted 1 line

Quoted 1 line
src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, = · line 147
Quoted 1 line
src/rpc_soc.c +0 -0
 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =

sockaddr_in);

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
src/rpc_soc.c +0 -0
 	bindaddr.buf =3D raddr;
=20
src/rpc_soc.c +1 -2
-	if (__binddynport(fd) =3D=3D -1)
-		goto err;
+	bindresvport(fd, NULL);

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
				sendsz, recvsz);
	if (cl) {
--=20
2.14.3
=20
=20
=

--------------------------------------------------------------------------=
----

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Libtirpc-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

Quoted 6 lines

Quoted 6 lines
=20
--
Chuck Lever
=20
=20
=20

--
Chuck Lever