Re: [PATCH] cifs: fix broken mounts when a SSH tunnel is used (try #3)

On Wed, 19 Aug 2009 15:13:07 +0530
Suresh Jayaraman <[email protected]> wrote:

> Just noticed that there was a undesired fall-through bug with the
> previous version of the patch. Fixed it.
> 
> It seems there is a regression that got introduced while Jeff fixed         
> all the mount/umount races. While attempting to find whether a tcp
> session is already existing, we were not checking whether the "port"
> used are the same. When a second mount is attempted with a different
> "port=" option, it is being ignored. Because of this the cifs mounts
> that uses a SSH tunnel appears to be broken.
> 
> Steps to reproduce:
> 
> 1. create 2 shares
> # SSH Tunnel a SMB session
> 2. ssh -f -L 6111:127.0.0.1:445 root@localhost "sleep 86400"
> 3. ssh -f -L 6222:127.0.0.1:445 root@localhost "sleep 86400"
> 4. tcpdump -i lo 6111 &
> 5. mkdir -p /mnt/mnt1
> 6. mkdir -p /mnt/mnt2
> 7. mount.cifs //localhost/a /mnt/mnt1 -o username=guest,ip=127.0.0.1,port=6111
> #(shows tcpdump activity on port 6111)
> 8. mount.cifs //localhost/b /mnt/mnt2 -o username=guest,ip=127.0.0.1,port=6222
> #(shows tcpdump activity only on port 6111 and not on 6222
> 
> Fix by adding a check to compare the port _only_ if the user tries to
> override the tcp port with "port=" option, before deciding that an
> existing tcp session is found. Also, clean up a bit by replacing
> if-else if by a switch statment while at it as suggested by Jeff.
> 
> Signed-off-by: Suresh Jayaraman <[email protected]>
> ---
> 
>  fs/cifs/connect.c |   57 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 45 insertions(+), 12 deletions(-)
> 

A couple of minor comments below. In general, I find it better to
structure the code so that you end up with less indentation, and
unneeded else clauses, etc...

> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 1f3345d..bfc5fa9 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1377,7 +1377,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  }
>  
>  static struct TCP_Server_Info *
> -cifs_find_tcp_session(struct sockaddr_storage *addr)
> +cifs_find_tcp_session(struct sockaddr_storage *addr, unsigned short int port)
>  {
>  	struct list_head *tmp;
>  	struct TCP_Server_Info *server;
> @@ -1397,16 +1397,49 @@ cifs_find_tcp_session(struct sockaddr_storage *addr)
>  		if (server->tcpStatus == CifsNew)
>  			continue;
>  
> -		if (addr->ss_family == AF_INET &&
> -		    (addr4->sin_addr.s_addr !=
> -		     server->addr.sockAddr.sin_addr.s_addr))
> -			continue;
> -		else if (addr->ss_family == AF_INET6 &&
> -			 (!ipv6_addr_equal(&server->addr.sockAddr6.sin6_addr,
> -					   &addr6->sin6_addr) ||
> -			  server->addr.sockAddr6.sin6_scope_id !=
> -					   addr6->sin6_scope_id))
> -			continue;
> +		switch (addr->ss_family) {
> +		case AF_INET:
> +			/* user overrode default port? */
> +			if (port) {
			   ^^^^ this is rather awkward since you now have 2 copies of each address comparison per family. If you compare the address before checking for the port then this function will be slightly more efficient and will look cleaner.

> +				addr4->sin_port = htons(port);
> +				if (addr4->sin_addr.s_addr !=
> +				    server->addr.sockAddr.sin_addr.s_addr ||
> +				    addr4->sin_port !=
> +				    server->addr.sockAddr.sin_port)
> +					continue;
> +				else
> +					break;
				^^^ no need for the else's here, just move the break outside of the outer "if's".
> +			} else {
> +				if (addr4->sin_addr.s_addr !=
> +				    server->addr.sockAddr.sin_addr.s_addr)
> +					continue;
> +				else
> +					break;
> +			}
> +
> +		case AF_INET6:
> +			/* user overrode default port? */
> +			if (port) {
> +				addr6->sin6_port = htons(port);
> +				if (!ipv6_addr_equal(&server->addr.sockAddr6.sin6_addr,
> +				    &addr6->sin6_addr) ||
> +				    server->addr.sockAddr6.sin6_scope_id !=
> +				    addr6->sin6_scope_id ||
> +				    server->addr.sockAddr6.sin6_port !=
> +				    addr6->sin6_port)
> +					continue;
> +				else
> +					break;
> +			} else {
> +				if (!ipv6_addr_equal(&server->addr.sockAddr6.sin6_addr,
> +				    &addr6->sin6_addr) ||
> +				    server->addr.sockAddr6.sin6_scope_id !=
> +				    addr6->sin6_scope_id)
> +					continue;
> +				else
> +					break;
> +			}
> +		}

The same comments apply to the IPv6 stanza here too.

>  
>  		++server->srv_count;
>  		write_unlock(&cifs_tcp_ses_lock);
> @@ -1475,7 +1508,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>  	}
>  
>  	/* see if we already have a matching tcp_ses */
> -	tcp_ses = cifs_find_tcp_session(&addr);
> +	tcp_ses = cifs_find_tcp_session(&addr, volume_info->port);
>  	if (tcp_ses)
>  		return tcp_ses;
>  

-- 
Jeff Layton <[email protected]>