Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fd568705-8036-b556-5a43-9fb59b4cddd4@redhat.com>
Date:   Sat, 16 Jul 2022 13:20:21 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] rpcb_clnt.c add mechanism to try v2 protocol first
Content-Language: en-US
To:     Roberto Bergantinos Corpas <rbergant@redhat.com>,
        libtirpc-devel@lists.sourceforge.net
Cc:     linux-nfs@vger.kernel.org
References: <20220701144422.328923-1-rbergant@redhat.com>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <20220701144422.328923-1-rbergant@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/1/22 9:44 AM, Roberto Bergantinos Corpas wrote:
> There have been previous attempts to revert protocol tryout
> algorithm from v4,v3,v2 to previous v2,v4,v3 :
> 
> https://www.spinics.net/lists/linux-nfs/msg89228.html
> 
> Apart from GETADDR/NAT issue originating that proposed change,
> its possible that some legacy custom applications still use
> v2 of protocol with libtirpc.
> 
> The change proposed here, introduces an environment variable
> "RPCB_V2FIRST" so that, if defined, old behaviour is used.
> This is more flexible and allow us to selectively pick what
> application reverts to old behaviour instead of a system-wide
> change.
> 
> Example :
> 
> $ tcpdump -s0 -i ens3 port 111 -w /tmp/capture.pcap &> /dev/null &
> [1] 13016
> $ rpcinfo -T tcp 172.23.1.225 100005 &> /dev/null
> $ RPCB_V2FIRST=1 rpcinfo -T tcp 172.23.1.225 100005 &> /dev/null
> $ pkill tcpdump
> $ tshark -tad -nr /tmp/capture.pcap -Y portmap -T fields -e _ws.col.Info
> V4 GETADDR Call
> V4 GETADDR Reply (Call In 4)
> V2 GETPORT Call MOUNT(100005) V:0 TCP
> V2 GETPORT Reply (Call In 14) Port:20048
> 
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
Committed... (tag libtirpc-1-3-3-rc3)

steved.
> ---
>   man/rpcbind.3t        |  2 ++
>   src/rpcb_clnt.c       | 27 ++++++++++++++++++++++++---
>   tirpc/rpc/pmap_prot.h |  2 ++
>   3 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/man/rpcbind.3t b/man/rpcbind.3t
> index ec492cc..4cb271b 100644
> --- a/man/rpcbind.3t
> +++ b/man/rpcbind.3t
> @@ -187,6 +187,8 @@ in
>   .El
>   .Sh AVAILABILITY
>   These functions are part of libtirpc.
> +.Sh ENVIRONMENT
> +If RPCB_V2FIRST is defined, rpcbind protocol version tryout algorithm changes from v4,v2,v3 to v2,v4,v3.
>   .Sh SEE ALSO
>   .Xr rpc_clnt_calls 3 ,
>   .Xr rpc_svc_calls 3 ,
> diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
> index 0c34cb7..db3799e 100644
> --- a/src/rpcb_clnt.c
> +++ b/src/rpcb_clnt.c
> @@ -818,7 +818,8 @@ error:
>    * The algorithm used: If the transports is TCP or UDP, it first tries
>    * version 4 (srv4), then 3 and then fall back to version 2 (portmap).
>    * With this algorithm, we get performance as well as a plan for
> - * obsoleting version 2.
> + * obsoleting version 2. This behaviour is reverted to old algorithm
> + * if RPCB_V2FIRST environment var is defined
>    *
>    * For all other transports, the algorithm remains as 4 and then 3.
>    *
> @@ -839,6 +840,10 @@ __rpcb_findaddr_timed(program, version, nconf, host, clpp, tp)
>   #ifdef NOTUSED
>   	static bool_t check_rpcbind = TRUE;
>   #endif
> +
> +#ifdef PORTMAP
> +	static bool_t portmap_first = FALSE;
> +#endif
>   	CLIENT *client = NULL;
>   	RPCB parms;
>   	enum clnt_stat clnt_st;
> @@ -895,8 +900,18 @@ __rpcb_findaddr_timed(program, version, nconf, host, clpp, tp)
>   		parms.r_addr = (char *) &nullstring[0];
>   	}
>   
> -	/* First try from start_vers(4) and then version 3 (RPCBVERS) */
> +	/* First try from start_vers(4) and then version 3 (RPCBVERS), except
> +	 * if env. var RPCB_V2FIRST is defined */
> +
> +#ifdef PORTMAP
> +	if (getenv(V2FIRST)) {
> +		portmap_first = TRUE;
> +		LIBTIRPC_DEBUG(3, ("__rpcb_findaddr_timed: trying v2-port first\n"));
> +		goto portmap;
> +	}
> +#endif
>   
> +rpcbind:
>   	CLNT_CONTROL(client, CLSET_RETRY_TIMEOUT, (char *) &rpcbrmttime);
>   	for (vers = start_vers;  vers >= RPCBVERS; vers--) {
>   		/* Set the version */
> @@ -944,10 +959,16 @@ __rpcb_findaddr_timed(program, version, nconf, host, clpp, tp)
>   	}
>   
>   #ifdef PORTMAP 	/* Try version 2 for TCP or UDP */
> +	if (portmap_first)
> +		goto error; /* we tried all versions if reached here */
> +portmap:
>   	if (strcmp(nconf->nc_protofmly, NC_INET) == 0) {
>   		address = __try_protocol_version_2(program, version, nconf, host, tp);
>   		if (address == NULL)
> -			goto error;
> +			if (portmap_first)
> +				goto rpcbind;
> +			else
> +				goto error;
>   	}
>   #endif		/* PORTMAP */
>   
> diff --git a/tirpc/rpc/pmap_prot.h b/tirpc/rpc/pmap_prot.h
> index 75354ce..7718b8b 100644
> --- a/tirpc/rpc/pmap_prot.h
> +++ b/tirpc/rpc/pmap_prot.h
> @@ -84,6 +84,8 @@
>   #define PMAPPROC_DUMP		((u_long)4)
>   #define PMAPPROC_CALLIT		((u_long)5)
>   
> +#define V2FIRST		"RPCB_V2FIRST"
> +
>   struct pmap {
>   	long unsigned pm_prog;
>   	long unsigned pm_vers;