[PATCH 1/2] Restore using reserve ports for client connections

[PATCH 1/2] Restore using reserve ports for client connections

[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.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 src/clnt_generic.c | 3 +--
 src/rpc_soc.c      | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/clnt_generic.c b/src/clnt_generic.c
index e5a314f..774292b 100644
--- a/src/clnt_generic.c
+++ b/src/clnt_generic.c
@@ -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;
diff --git a/src/rpc_soc.c b/src/rpc_soc.c
index af6c482..f32a27c 100644
--- a/src/rpc_soc.c
+++ b/src/rpc_soc.c
@@ -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) {
-- 
2.14.3

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

[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.

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 <steved@redhat.com>
---
 src/rpc_soc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/rpc_soc.c b/src/rpc_soc.c
index f32a27c..cf15216 100644
--- a/src/rpc_soc.c
+++ b/src/rpc_soc.c
@@ -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) {
-- 
2.14.3

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

[Chuck Lever]

> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
>=20
> 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.
>=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?


> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> src/rpc_soc.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>=20
> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
> index f32a27c..cf15216 100644
> --- a/src/rpc_soc.c
> +++ b/src/rpc_soc.c
> @@ -331,6 +331,13 @@ svc_com_create(fd, sendsize, recvsize, netid)
> 		madefd =3D TRUE;
> 	}
>=20
> +	if (__binddynport(fd) =3D=3D -1) {
> +		if (madefd)
> +			(void)close(fd);
> +		(void) syslog(LOG_ERR,
> +			"svc%s_create: could not bind connection: %m", =
netid);
> +	}
> +	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" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

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

[Steve Dickson]

Hey,

On 04/10/2018 06:29 PM, Chuck Lever wrote:
> 
> 
>> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
>>
>> 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.
> 
> 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.

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Chuck Lever]

> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
>=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.


> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> src/clnt_generic.c | 3 +--
> src/rpc_soc.c      | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>=20
> diff --git a/src/clnt_generic.c b/src/clnt_generic.c
> index e5a314f..774292b 100644
> --- a/src/clnt_generic.c
> +++ b/src/clnt_generic.c
> @@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig =
*nconf,
> 		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;
> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
> index af6c482..f32a27c 100644
> --- a/src/rpc_soc.c
> +++ b/src/rpc_soc.c
> @@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, =
recvsz, tp, flags)
> 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =
sockaddr_in);
> 	bindaddr.buf =3D raddr;
>=20
> -	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) {
> --=20
> 2.14.3
>=20
>=20
> =
--------------------------------------------------------------------------=
----
> 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
> Libtirpc-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Steve Dickson]

Hey,

On 04/10/2018 06:17 PM, Chuck Lever wrote:
> 
> 
>> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
>>
>> 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.
> 
> 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.

> 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.
 
> 
> 
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>> src/clnt_generic.c | 3 +--
>> src/rpc_soc.c      | 3 +--
>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/clnt_generic.c b/src/clnt_generic.c
>> index e5a314f..774292b 100644
>> --- a/src/clnt_generic.c
>> +++ b/src/clnt_generic.c
>> @@ -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;
>> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
>> index af6c482..f32a27c 100644
>> --- a/src/rpc_soc.c
>> +++ b/src/rpc_soc.c
>> @@ -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) {
>> -- 
>> 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
>> Libtirpc-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel
> 
> --
> Chuck Lever
> 
> 
> 

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Thorsten Kukuk]

On Wed, Apr 11, Steve Dickson wrote:

> 
> Hey,
> 
> On 04/10/2018 06:17 PM, Chuck Lever wrote:
> > 
> > 
> >> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
> >>
> >> 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.
> > 
> > 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

-- 
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)

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Steve Dickson]

On 04/11/2018 08:59 AM, Thorsten Kukuk wrote:
> On Wed, Apr 11, Steve Dickson wrote:
> 
>>
>> Hey,
>>
>> On 04/10/2018 06:17 PM, Chuck Lever wrote:
>>>
>>>
>>>> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> wrote:
>>>>
>>>> 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.
>>>
>>> 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.

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

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Chuck Lever]

> On Apr 11, 2018, at 8:07 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>=20
>=20
>=20
> On 04/11/2018 08:59 AM, Thorsten Kukuk wrote:
>> On Wed, Apr 11, Steve Dickson wrote:
>>=20
>>>=20
>>> Hey,
>>>=20
>>> On 04/10/2018 06:17 PM, Chuck Lever wrote:
>>>>=20
>>>>=20
>>>>> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> =
wrote:
>>>>>=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.
>>>>=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?


> steved.
>=20
>> Yes, and I need to change that like Solaris is doing it, only a
>> question of time on my side :(
>>=20
>>  Thorsten
>>=20
>=20
> =
--------------------------------------------------------------------------=
----
> 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
> Libtirpc-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever
chucklever@gmail.com

Re: [Libtirpc-devel] [PATCH 1/2] Restore using reserve ports for client connections

[Chuck Lever]

> On Apr 11, 2018, at 6:34 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>=20
>=20
> Hey,
>=20
> On 04/10/2018 06:17 PM, Chuck Lever wrote:
>>=20
>>=20
>>> On Apr 10, 2018, at 3:30 PM, Steve Dickson <steved@redhat.com> =
wrote:
>>>=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.
>>=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.


>> 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.


> 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.


> 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.


> steved.
>=20
>>=20
>>=20
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>> src/clnt_generic.c | 3 +--
>>> src/rpc_soc.c      | 3 +--
>>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>>=20
>>> diff --git a/src/clnt_generic.c b/src/clnt_generic.c
>>> index e5a314f..774292b 100644
>>> --- a/src/clnt_generic.c
>>> +++ b/src/clnt_generic.c
>>> @@ -341,8 +341,7 @@ clnt_tli_create(int fd, const struct netconfig =
*nconf,
>>> 		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;
>>> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
>>> index af6c482..f32a27c 100644
>>> --- a/src/rpc_soc.c
>>> +++ b/src/rpc_soc.c
>>> @@ -147,8 +147,7 @@ clnt_com_create(raddr, prog, vers, sockp, =
sendsz, recvsz, tp, flags)
>>> 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =
sockaddr_in);
>>> 	bindaddr.buf =3D raddr;
>>>=20
>>> -	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) {
>>> --=20
>>> 2.14.3
>>>=20
>>>=20
>>> =
--------------------------------------------------------------------------=
----
>>> 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
>>> Libtirpc-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20

--
Chuck Lever