Re: [PATCH - v2] mount.nfs: Fix fallback from tcp to udp

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Feb 24, 2014 at 02:23:49PM +1100, NeilBrown wrote:
> 
> Protocol negotiation in mount.nfs does not correctly negotiate with a
> server which only supports NFSv3 and UDP.
> 
> When mount.nfs attempts an NFSv4 mount and fails with ECONNREFUSED
> it does not fall back to NFSv3, as this is not recognised as a
> "does not support NFSv4" error.
> However ECONNREFUSED is a clear indication that the server doesn't
> support TCP, and ipso facto does not support NFSv4.
> So ECONNREFUSED should trigger a fallback from v4 to v2/3.
> 
> However ECONNREFUSED may simply indicate that NFSv4 isn't supported
> *yet*.  i.e. the server is still booting and isn't responding to NFS
> requests yet.  So if we subsequently find that NFSv3 is supported, we
> need to check with rpcbind to confirm that NFSv4 really isn't
> supported.
> If rpcbind reports that v4 is not supported after reporting that v3
> is, we can safely use v4.  If it reports that v4 is supported, we need

s/use v4/use v3/.

ACK to the idea, I haven't tried to review the code....

--b.

> to retry v4.
> 
> One line in this patch requires explanation.  It is
> +		save_pmap.pm_vers = 4;
> 
> This is needed as nfs_probe_nfsport is entered with pm_vers already
> set, by the line
> 		nfs_pmap->pm_vers = mntvers_to_nfs(*probe_vers);
> in nfs_probe_bothports().  This makes the passing of probe_nfs3_only
> and probe_nfs2_only to nfs_probe_port() in nfs_probe_nfsport()
> pointless, and the passing of probe_nfs4_only in the new code
> ineffectual, without resetting pm_vers.
> 
> The setting of pm_vers to mntvers_to_nfs() should probably be removed,
> but as we don't have any regression tests, doing this would be unwise.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> Reported-by: Carsten Ziepke <kieltux@gmail.com>
> 
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index 2fdd2c051be7..0521d5f6709f 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -149,6 +149,11 @@ static const unsigned int probe_tcp_first[] = {
>  	0,
>  };
>  
> +static const unsigned int probe_tcp_only[] = {
> +	IPPROTO_TCP,
> +	0,
> +};
> +
>  static const unsigned long probe_nfs2_only[] = {
>  	2,
>  	0,
> @@ -159,6 +164,11 @@ static const unsigned long probe_nfs3_only[] = {
>  	0,
>  };
>  
> +static const unsigned long probe_nfs4_only[] = {
> +	4,
> +	0,
> +};
> +
>  static const unsigned long probe_mnt1_first[] = {
>  	1,
>  	2,
> @@ -611,18 +621,34 @@ out_ok:
>   * returned; rpccreateerr.cf_stat is set to reflect the nature of the error.
>   */
>  static int nfs_probe_nfsport(const struct sockaddr *sap, const socklen_t salen,
> -				struct pmap *pmap)
> +			     struct pmap *pmap, int checkv4)
>  {
>  	if (pmap->pm_vers && pmap->pm_prot && pmap->pm_port)
>  		return 1;
>  
>  	if (nfs_mount_data_version >= 4) {
>  		const unsigned int *probe_proto;
> +		int ret;
> +		struct pmap save_pmap;
> +		struct sockaddr_storage save_sa;
>  
>  		probe_proto = nfs_default_proto();
> -
> -		return nfs_probe_port(sap, salen, pmap,
> -					probe_nfs3_only, probe_proto);
> +		memcpy(&save_pmap, pmap, sizeof(save_pmap));
> +		memcpy(&save_sa, sap, salen);
> +
> +		ret = nfs_probe_port(sap, salen, pmap,
> +				     probe_nfs3_only, probe_proto);
> +		if (!ret || !checkv4 || probe_proto != probe_tcp_first)
> +			return ret;
> +		save_pmap.pm_vers = 4;
> +		ret = nfs_probe_port((struct sockaddr*)&save_sa, salen, &save_pmap,
> +				     probe_nfs4_only, probe_tcp_only);
> +		if (ret) {
> +			rpc_createerr.cf_stat = RPC_FAILED;
> +			rpc_createerr.cf_error.re_errno = EAGAIN;
> +			return 0;
> +		}
> +		return 1;
>  	} else
>  		return nfs_probe_port(sap, salen, pmap,
>  					probe_nfs2_only, probe_udp_only);
> @@ -671,7 +697,7 @@ static int nfs_probe_version_fixed(const struct sockaddr *mnt_saddr,
>  			const socklen_t nfs_salen,
>  			struct pmap *nfs_pmap)
>  {
> -	if (!nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap))
> +	if (!nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap, 0))
>  		return 0;
>  	return nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap);
>  }
> @@ -686,6 +712,8 @@ static int nfs_probe_version_fixed(const struct sockaddr *mnt_saddr,
>   * @nfs_salen:	length of NFS server's address
>   * @nfs_pmap:	IN: partially filled-in NFS RPC service tuple;
>   *		OUT: fully filled-in NFS RPC service tuple
> + * @checkv4:	Flag indicating that if v3 is available we must also
> + *		check v4, and if that is available, set re_errno to EAGAIN.
>   *
>   * Returns 1 and fills in both @pmap structs if the requested service
>   * ports are unambiguous and pingable.  Otherwise zero is returned;
> @@ -696,7 +724,8 @@ int nfs_probe_bothports(const struct sockaddr *mnt_saddr,
>  			struct pmap *mnt_pmap,
>  			const struct sockaddr *nfs_saddr,
>  			const socklen_t nfs_salen,
> -			struct pmap *nfs_pmap)
> +			struct pmap *nfs_pmap,
> +			int checkv4)
>  {
>  	struct pmap save_nfs, save_mnt;
>  	const unsigned long *probe_vers;
> @@ -717,7 +746,7 @@ int nfs_probe_bothports(const struct sockaddr *mnt_saddr,
>  
>  	for (; *probe_vers; probe_vers++) {
>  		nfs_pmap->pm_vers = mntvers_to_nfs(*probe_vers);
> -		if (nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap) != 0) {
> +		if (nfs_probe_nfsport(nfs_saddr, nfs_salen, nfs_pmap, checkv4) != 0) {
>  			mnt_pmap->pm_vers = *probe_vers;
>  			if (nfs_probe_mntport(mnt_saddr, mnt_salen, mnt_pmap) != 0)
>  				return 1;
> @@ -757,7 +786,7 @@ int probe_bothports(clnt_addr_t *mnt_server, clnt_addr_t *nfs_server)
>  	return nfs_probe_bothports(mnt_addr, sizeof(mnt_server->saddr),
>  					&mnt_server->pmap,
>  					nfs_addr, sizeof(nfs_server->saddr),
> -					&nfs_server->pmap);
> +					&nfs_server->pmap, 0);
>  }
>  
>  /**
> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index 9c75856c9ca8..d7636d7c54a6 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -42,7 +42,7 @@ static const struct timeval RETRY_TIMEOUT = { 3, 0 };
>  int probe_bothports(clnt_addr_t *, clnt_addr_t *);
>  int nfs_probe_bothports(const struct sockaddr *, const socklen_t,
>  			struct pmap *, const struct sockaddr *,
> -			const socklen_t, struct pmap *);
> +			const socklen_t, struct pmap *, int);
>  int nfs_gethostbyname(const char *, struct sockaddr_in *);
>  int nfs_lookup(const char *hostname, const sa_family_t family,
>  		struct sockaddr *sap, socklen_t *salen);
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index a642394d2f5a..1f4782563572 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -484,7 +484,7 @@ static int nfs_construct_new_options(struct mount_options *options,
>   * FALSE is returned if some failure occurred.
>   */
>  static int
> -nfs_rewrite_pmap_mount_options(struct mount_options *options)
> +nfs_rewrite_pmap_mount_options(struct mount_options *options, int checkv4)
>  {
>  	union nfs_sockaddr nfs_address;
>  	struct sockaddr *nfs_saddr = &nfs_address.sa;
> @@ -534,7 +534,7 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options)
>  	 * negotiate.  Bail now if we can't contact it.
>  	 */
>  	if (!nfs_probe_bothports(mnt_saddr, mnt_salen, &mnt_pmap,
> -				 nfs_saddr, nfs_salen, &nfs_pmap)) {
> +				 nfs_saddr, nfs_salen, &nfs_pmap, checkv4)) {
>  		errno = ESPIPE;
>  		if (rpc_createerr.cf_stat == RPC_PROGNOTREGISTERED)
>  			errno = EOPNOTSUPP;
> @@ -595,7 +595,8 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
>  }
>  
>  static int nfs_do_mount_v3v2(struct nfsmount_info *mi,
> -		struct sockaddr *sap, socklen_t salen)
> +			     struct sockaddr *sap, socklen_t salen,
> +			     int checkv4)
>  {
>  	struct mount_options *options = po_dup(mi->options);
>  	int result = 0;
> @@ -637,7 +638,7 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi,
>  		printf(_("%s: trying text-based options '%s'\n"),
>  			progname, *mi->extra_opts);
>  
> -	if (!nfs_rewrite_pmap_mount_options(options))
> +	if (!nfs_rewrite_pmap_mount_options(options, checkv4))
>  		goto out_fail;
>  
>  	result = nfs_sys_mount(mi, options);
> @@ -653,13 +654,13 @@ out_fail:
>   * Returns TRUE if successful, otherwise FALSE.
>   * "errno" is set to reflect the individual error.
>   */
> -static int nfs_try_mount_v3v2(struct nfsmount_info *mi)
> +static int nfs_try_mount_v3v2(struct nfsmount_info *mi, int checkv4)
>  {
>  	struct addrinfo *ai;
>  	int ret = 0;
>  
>  	for (ai = mi->address; ai != NULL; ai = ai->ai_next) {
> -		ret = nfs_do_mount_v3v2(mi, ai->ai_addr, ai->ai_addrlen);
> +		ret = nfs_do_mount_v3v2(mi, ai->ai_addr, ai->ai_addrlen, checkv4);
>  		if (ret != 0)
>  			return ret;
>  
> @@ -793,7 +794,8 @@ static int nfs_autonegotiate(struct nfsmount_info *mi)
>  	result = nfs_try_mount_v4(mi);
>  	if (result)
>  		return result;
> -		
> +
> +check_errno:
>  	switch (errno) {
>  	case EPROTONOSUPPORT:
>  		/* A clear indication that the server or our
> @@ -807,12 +809,24 @@ static int nfs_autonegotiate(struct nfsmount_info *mi)
>  		/* Linux servers prior to 2.6.25 may return
>  		 * EPERM when NFS version 4 is not supported. */
>  		goto fall_back;
> +	case ECONNREFUSED:
> +		/* UDP-Only servers won't support v4, but maybe it
> +		 * just isn't ready yet.  So try v3, but double-check
> +		 * with rpcbind for v4. */
> +		result = nfs_try_mount_v3v2(mi, 1);
> +		if (result == 0 && errno == EAGAIN) {
> +			/* v4 server seems to be registered now. */
> +			result = nfs_try_mount_v4(mi);
> +			if (result == 0 && errno != ECONNREFUSED)
> +				goto check_errno;
> +		}
> +		return result;
>  	default:
>  		return result;
>  	}
>  
>  fall_back:
> -	return nfs_try_mount_v3v2(mi);
> +	return nfs_try_mount_v3v2(mi, 0);
>  }
>  
>  /*
> @@ -831,7 +845,7 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>  		break;
>  	case 2:
>  	case 3:
> -		result = nfs_try_mount_v3v2(mi);
> +		result = nfs_try_mount_v3v2(mi, 0);
>  		break;
>  	case 4:
>  		result = nfs_try_mount_v4(mi);