[PATCH] CIFS: Fix mounting share on non-standard ports

[PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

This patch fixes the following situation: we have the share (1) mounted
with a non-standard port and try to mount another share (2) with the standard
one (not specifying the port), the client doesn't connect to the new share (2)
and returns with existing connection (1). The patch also makes the client
prevent connecting to a wrong share on a reconnect if the client fails during
the re-establishing connection to a lost share.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/connect.c |   86 ++++++++++++++++++++++++++---------------------------
 1 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..6edf5ed 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1446,25 +1446,32 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
 {
 	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
 	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
+	unsigned short int port;
+
+	/* search for a connection with the default port if user doesn't
+	   specify the port manually */
+	port = htons(CIFS_PORT);
 
 	switch (addr->sa_family) {
 	case AF_INET:
+		if (addr4->sin_port)
+			port = addr4->sin_port;
 		if (addr4->sin_addr.s_addr !=
 		    server->addr.sockAddr.sin_addr.s_addr)
 			return false;
-		if (addr4->sin_port &&
-		    addr4->sin_port != server->addr.sockAddr.sin_port)
+		if (port != server->addr.sockAddr.sin_port)
 			return false;
 		break;
 	case AF_INET6:
+		if (addr6->sin6_port)
+			port = addr6->sin6_port;
 		if (!ipv6_addr_equal(&addr6->sin6_addr,
 				     &server->addr.sockAddr6.sin6_addr))
 			return false;
 		if (addr6->sin6_scope_id !=
 		    server->addr.sockAddr6.sin6_scope_id)
 			return false;
-		if (addr6->sin6_port &&
-		    addr6->sin6_port != server->addr.sockAddr6.sin6_port)
+		if (port != server->addr.sockAddr6.sin6_port)
 			return false;
 		break;
 	}
@@ -2126,7 +2133,7 @@ ipv4_connect(struct TCP_Server_Info *server)
 	int rc = 0;
 	int val;
 	bool connected = false;
-	__be16 orig_port = 0;
+	bool orig_port_error = false;
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
@@ -2148,32 +2155,30 @@ ipv4_connect(struct TCP_Server_Info *server)
 	if (rc < 0)
 		return rc;
 
-	/* user overrode default port */
+	/* user overrode default port or we perform reconnect */
 	if (server->addr.sockAddr.sin_port) {
 		rc = socket->ops->connect(socket, (struct sockaddr *)
 					  &server->addr.sockAddr,
 					  sizeof(struct sockaddr_in), 0);
 		if (rc >= 0)
 			connected = true;
+		else
+			orig_port_error = true;
+		/* in this case we don't have to try other port
+		   because we can mount another share */
 	}
 
-	if (!connected) {
-		/* save original port so we can retry user specified port
-			later if fall back ports fail this time  */
-		orig_port = server->addr.sockAddr.sin_port;
-
-		/* do not retry on the same port we just failed on */
-		if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
-			server->addr.sockAddr.sin_port = htons(CIFS_PORT);
-			rc = socket->ops->connect(socket,
-						(struct sockaddr *)
-						&server->addr.sockAddr,
-						sizeof(struct sockaddr_in), 0);
-			if (rc >= 0)
-				connected = true;
-		}
+	if (!orig_port_error && !connected) {
+		server->addr.sockAddr.sin_port = htons(CIFS_PORT);
+		rc = socket->ops->connect(socket,
+					(struct sockaddr *)
+					&server->addr.sockAddr,
+					sizeof(struct sockaddr_in), 0);
+		if (rc >= 0)
+			connected = true;
 	}
-	if (!connected) {
+
+	if (!orig_port_error && !connected) {
 		server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
 		rc = socket->ops->connect(socket, (struct sockaddr *)
 					      &server->addr.sockAddr,
@@ -2185,15 +2190,12 @@ ipv4_connect(struct TCP_Server_Info *server)
 	/* give up here - unless we want to retry on different
 		protocol families some day */
 	if (!connected) {
-		if (orig_port)
-			server->addr.sockAddr.sin_port = orig_port;
 		cFYI(1, "Error %d connecting to server via ipv4", rc);
 		sock_release(socket);
 		server->ssocket = NULL;
 		return rc;
 	}
 
-
 	/*
 	 * Eventually check for other socket options to change from
 	 *  the default. sock_setsockopt not used because it expects
@@ -2291,7 +2293,7 @@ ipv6_connect(struct TCP_Server_Info *server)
 	int rc = 0;
 	int val;
 	bool connected = false;
-	__be16 orig_port = 0;
+	bool orig_port_error = false;
 	struct socket *socket = server->ssocket;
 
 	if (socket == NULL) {
@@ -2314,31 +2316,29 @@ ipv6_connect(struct TCP_Server_Info *server)
 	if (rc < 0)
 		return rc;
 
-	/* user overrode default port */
+	/* user overrode default port or we perform reconnect */
 	if (server->addr.sockAddr6.sin6_port) {
 		rc = socket->ops->connect(socket,
 				(struct sockaddr *) &server->addr.sockAddr6,
 				sizeof(struct sockaddr_in6), 0);
 		if (rc >= 0)
 			connected = true;
+		else
+			orig_port_error = true;
+		/* in this case we don't have to try other port
+		   because we can mount another share */
 	}
 
-	if (!connected) {
-		/* save original port so we can retry user specified port
-			later if fall back ports fail this time  */
-
-		orig_port = server->addr.sockAddr6.sin6_port;
-		/* do not retry on the same port we just failed on */
-		if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
-			server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
-			rc = socket->ops->connect(socket, (struct sockaddr *)
-					&server->addr.sockAddr6,
-					sizeof(struct sockaddr_in6), 0);
-			if (rc >= 0)
-				connected = true;
-		}
+	if (!orig_port_error && !connected) {
+		server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
+		rc = socket->ops->connect(socket, (struct sockaddr *)
+				&server->addr.sockAddr6,
+				sizeof(struct sockaddr_in6), 0);
+		if (rc >= 0)
+			connected = true;
 	}
-	if (!connected) {
+
+	if (!orig_port_error && !connected) {
 		server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
 		rc = socket->ops->connect(socket, (struct sockaddr *)
 				&server->addr.sockAddr6,
@@ -2350,8 +2350,6 @@ ipv6_connect(struct TCP_Server_Info *server)
 	/* give up here - unless we want to retry on different
 		protocol families some day */
 	if (!connected) {
-		if (orig_port)
-			server->addr.sockAddr6.sin6_port = orig_port;
 		cFYI(1, "Error %d connecting to server via ipv6", rc);
 		sock_release(socket);
 		server->ssocket = NULL;
-- 
1.7.2.3

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/9 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> This patch fixes the following situation: we have the share (1) mounted
> with a non-standard port and try to mount another share (2) with the standard
> one (not specifying the port), the client doesn't connect to the new share (2)
> and returns with existing connection (1). The patch also makes the client
> prevent connecting to a wrong share on a reconnect if the client fails during
> the re-establishing connection to a lost share.

What's about this patch?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Jeff Layton]

On Tue,  9 Nov 2010 19:50:25 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This patch fixes the following situation: we have the share (1) mounted
> with a non-standard port and try to mount another share (2) with the standard
> one (not specifying the port), the client doesn't connect to the new share (2)
> and returns with existing connection (1). The patch also makes the client
> prevent connecting to a wrong share on a reconnect if the client fails during
> the re-establishing connection to a lost share.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/connect.c |   86 ++++++++++++++++++++++++++---------------------------
>  1 files changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..6edf5ed 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1446,25 +1446,32 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
>  {
>  	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
>  	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
> +	unsigned short int port;
> +
> +	/* search for a connection with the default port if user doesn't
> +	   specify the port manually */
> +	port = htons(CIFS_PORT);
>  

I don't think this patch deals correctly with servers that are
listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
same server that don't specify a port, you'll end up with two sockets,
right?

Also, the logic in match_address is getting to be rather convoluted. It
might be good to break that out into a match_port function.

>  	switch (addr->sa_family) {
>  	case AF_INET:
> +		if (addr4->sin_port)
> +			port = addr4->sin_port;
>  		if (addr4->sin_addr.s_addr !=
>  		    server->addr.sockAddr.sin_addr.s_addr)
>  			return false;
> -		if (addr4->sin_port &&
> -		    addr4->sin_port != server->addr.sockAddr.sin_port)
> +		if (port != server->addr.sockAddr.sin_port)
>  			return false;
>  		break;
>  	case AF_INET6:
> +		if (addr6->sin6_port)
> +			port = addr6->sin6_port;
>  		if (!ipv6_addr_equal(&addr6->sin6_addr,
>  				     &server->addr.sockAddr6.sin6_addr))
>  			return false;
>  		if (addr6->sin6_scope_id !=
>  		    server->addr.sockAddr6.sin6_scope_id)
>  			return false;
> -		if (addr6->sin6_port &&
> -		    addr6->sin6_port != server->addr.sockAddr6.sin6_port)
> +		if (port != server->addr.sockAddr6.sin6_port)
>  			return false;
>  		break;
>  	}
> @@ -2126,7 +2133,7 @@ ipv4_connect(struct TCP_Server_Info *server)
>  	int rc = 0;
>  	int val;
>  	bool connected = false;
> -	__be16 orig_port = 0;
> +	bool orig_port_error = false;
>  	struct socket *socket = server->ssocket;
>  
>  	if (socket == NULL) {
> @@ -2148,32 +2155,30 @@ ipv4_connect(struct TCP_Server_Info *server)
>  	if (rc < 0)
>  		return rc;
>  
> -	/* user overrode default port */
> +	/* user overrode default port or we perform reconnect */
>  	if (server->addr.sockAddr.sin_port) {
>  		rc = socket->ops->connect(socket, (struct sockaddr *)
>  					  &server->addr.sockAddr,
>  					  sizeof(struct sockaddr_in), 0);
>  		if (rc >= 0)
>  			connected = true;
> +		else
> +			orig_port_error = true;
> +		/* in this case we don't have to try other port
> +		   because we can mount another share */
>  	}
>  
> -	if (!connected) {
> -		/* save original port so we can retry user specified port
> -			later if fall back ports fail this time  */
> -		orig_port = server->addr.sockAddr.sin_port;
> -
> -		/* do not retry on the same port we just failed on */
> -		if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
> -			server->addr.sockAddr.sin_port = htons(CIFS_PORT);
> -			rc = socket->ops->connect(socket,
> -						(struct sockaddr *)
> -						&server->addr.sockAddr,
> -						sizeof(struct sockaddr_in), 0);
> -			if (rc >= 0)
> -				connected = true;
> -		}
> +	if (!orig_port_error && !connected) {
> +		server->addr.sockAddr.sin_port = htons(CIFS_PORT);
> +		rc = socket->ops->connect(socket,
> +					(struct sockaddr *)
> +					&server->addr.sockAddr,
> +					sizeof(struct sockaddr_in), 0);
> +		if (rc >= 0)
> +			connected = true;
>  	}
> -	if (!connected) {
> +
> +	if (!orig_port_error && !connected) {
>  		server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
>  		rc = socket->ops->connect(socket, (struct sockaddr *)
>  					      &server->addr.sockAddr,
> @@ -2185,15 +2190,12 @@ ipv4_connect(struct TCP_Server_Info *server)
>  	/* give up here - unless we want to retry on different
>  		protocol families some day */
>  	if (!connected) {
> -		if (orig_port)
> -			server->addr.sockAddr.sin_port = orig_port;
>  		cFYI(1, "Error %d connecting to server via ipv4", rc);
>  		sock_release(socket);
>  		server->ssocket = NULL;
>  		return rc;
>  	}
>  
> -
>  	/*
>  	 * Eventually check for other socket options to change from
>  	 *  the default. sock_setsockopt not used because it expects
> @@ -2291,7 +2293,7 @@ ipv6_connect(struct TCP_Server_Info *server)
>  	int rc = 0;
>  	int val;
>  	bool connected = false;
> -	__be16 orig_port = 0;
> +	bool orig_port_error = false;
>  	struct socket *socket = server->ssocket;
>  
>  	if (socket == NULL) {
> @@ -2314,31 +2316,29 @@ ipv6_connect(struct TCP_Server_Info *server)
>  	if (rc < 0)
>  		return rc;
>  
> -	/* user overrode default port */
> +	/* user overrode default port or we perform reconnect */
>  	if (server->addr.sockAddr6.sin6_port) {
>  		rc = socket->ops->connect(socket,
>  				(struct sockaddr *) &server->addr.sockAddr6,
>  				sizeof(struct sockaddr_in6), 0);
>  		if (rc >= 0)
>  			connected = true;
> +		else
> +			orig_port_error = true;
> +		/* in this case we don't have to try other port
> +		   because we can mount another share */
>  	}
>  
> -	if (!connected) {
> -		/* save original port so we can retry user specified port
> -			later if fall back ports fail this time  */
> -
> -		orig_port = server->addr.sockAddr6.sin6_port;
> -		/* do not retry on the same port we just failed on */
> -		if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
> -			server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
> -			rc = socket->ops->connect(socket, (struct sockaddr *)
> -					&server->addr.sockAddr6,
> -					sizeof(struct sockaddr_in6), 0);
> -			if (rc >= 0)
> -				connected = true;
> -		}
> +	if (!orig_port_error && !connected) {
> +		server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
> +		rc = socket->ops->connect(socket, (struct sockaddr *)
> +				&server->addr.sockAddr6,
> +				sizeof(struct sockaddr_in6), 0);
> +		if (rc >= 0)
> +			connected = true;
>  	}
> -	if (!connected) {
> +
> +	if (!orig_port_error && !connected) {
>  		server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
>  		rc = socket->ops->connect(socket, (struct sockaddr *)
>  				&server->addr.sockAddr6,
> @@ -2350,8 +2350,6 @@ ipv6_connect(struct TCP_Server_Info *server)
>  	/* give up here - unless we want to retry on different
>  		protocol families some day */
>  	if (!connected) {
> -		if (orig_port)
> -			server->addr.sockAddr6.sin6_port = orig_port;
>  		cFYI(1, "Error %d connecting to server via ipv6", rc);
>  		sock_release(socket);
>  		server->ssocket = NULL;


ipv4_connect and ipv6_connect ought to be sharing a lot of code. It
would be good to do some cleanup here and try to consolidate these
functions more than they are today.

I envision a function that just handles setting up the socket and
leaves the caller to determine the port to use. Also, breaking out the
RFC1001 stuff into a separate function would be good too.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>
> I don't think this patch deals correctly with servers that are
> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
> same server that don't specify a port, you'll end up with two sockets,
> right?

Yes, you are right. I don't think that it is a big problem im
comparison with what we have today. Today we have totally broken code
- when I have two servers on the one host with different ports (e.x.
the first on one host and the second on the second but can be accesed
through the first with ssh forwarded port) and user wants to mount
both. In results, user mount only the first share twice - it can break
a lot of things!

>
> Also, the logic in match_address is getting to be rather convoluted. It
> might be good to break that out into a match_port function.
>
> ipv4_connect and ipv6_connect ought to be sharing a lot of code. It
> would be good to do some cleanup here and try to consolidate these
> functions more than they are today.
>
> I envision a function that just handles setting up the socket and
> leaves the caller to determine the port to use. Also, breaking out the
> RFC1001 stuff into a separate function would be good too.
>

You are absolutely right. I agree with you about copy-and-past code
for ipv4(v6)_connect function - good chance for cleanup. I am going to
do it as soon as I finished with strict cache mode. But for a while -
I don't think we can leave such a bug I described above.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>>
>> I don't think this patch deals correctly with servers that are
>> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
>> same server that don't specify a port, you'll end up with two sockets,
>> right?
>

And from another hand: If user doesn't specify the port we should
think that it means the 445 port. If user wants to mount 139 port, he
should specify this port manually. So, there is no error with the
patch in this case.

>From this point of view we should remove trying with 139 port if we
failed with 445 port. What do you think about it?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Jeff Layton]

On Sun, 14 Nov 2010 12:38:18 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> >>
> >> I don't think this patch deals correctly with servers that are
> >> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
> >> same server that don't specify a port, you'll end up with two sockets,
> >> right?
> >
> 
> And from another hand: If user doesn't specify the port we should
> think that it means the 445 port. If user wants to mount 139 port, he
> should specify this port manually. So, there is no error with the
> patch in this case.
> 
> From this point of view we should remove trying with 139 port if we
> failed with 445 port. What do you think about it?
> 

That sounds like a regression. The mount.cifs manpage says:

       port=arg
           sets the port number on the server to attempt to contact to
           negotiate CIFS support. If the CIFS server is not listening on this
           port or if it is not specified, the default ports will be tried
           i.e. port 445 is tried and if no response then port 139 is tried.


I think we ought to preserve that behavior. Perhaps if no port is
specified then match any TCP session that is on port 445 or port 139?

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Steve French]

On Sun, Nov 14, 2010 at 7:31 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Sun, 14 Nov 2010 12:38:18 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> > 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> >>
>> >> I don't think this patch deals correctly with servers that are
>> >> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
>> >> same server that don't specify a port, you'll end up with two sockets,
>> >> right?
>> >
>>
>> And from another hand: If user doesn't specify the port we should
>> think that it means the 445 port. If user wants to mount 139 port, he
>> should specify this port manually. So, there is no error with the
>> patch in this case.
>>
>> From this point of view we should remove trying with 139 port if we
>> failed with 445 port. What do you think about it?
>>
>
> That sounds like a regression. The mount.cifs manpage says:
>
>       port=arg
>           sets the port number on the server to attempt to contact to
>           negotiate CIFS support. If the CIFS server is not listening on this
>           port or if it is not specified, the default ports will be tried
>           i.e. port 445 is tried and if no response then port 139 is tried.
>
>
> I think we ought to preserve that behavior. Perhaps if no port is
> specified then match any TCP session that is on port 445 or port 139?

Right - sounds logical that if you don't specify a port, then we try
to match a connection on any existing port.  If there is no existing
port, and none was specified, then 445 first then 139.


-- 
Thanks,

Steve

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/15 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Sun, Nov 14, 2010 at 7:31 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> That sounds like a regression. The mount.cifs manpage says:
>>
>>       port=arg
>>           sets the port number on the server to attempt to contact to
>>           negotiate CIFS support. If the CIFS server is not listening on this
>>           port or if it is not specified, the default ports will be tried
>>           i.e. port 445 is tried and if no response then port 139 is tried.
>>
>>
>> I think we ought to preserve that behavior. Perhaps if no port is
>> specified then match any TCP session that is on port 445 or port 139?
>
> Right - sounds logical that if you don't specify a port, then we try
> to match a connection on any existing port.  If there is no existing
> port, and none was specified, then 445 first then 139.
>

Let's predict the following situation:

we have server with two smbd's: 1th on 445 port, and 2nd on 5000 port.
At first user mounts 2nd smbd with specifying 5000 port. Then it tries
to mount 1th without specifying any ports. If we consider your logic,
our client looks for an any existing connection, finds connection to
5000 port (2nd smbd) and mounts it. So, as the result, user thinks
that it successfully mounted the 1th share, but it is the 2nd - it is
not right.

If we should follow mount.cifs spec, I think we need to do:
1) if user doesn't specify a port we at first try to match a
connection with 445 port, then if we are not successful - with 139
port. If there are no connections with these ports, we return an
error.
2) if user specifies a port, we should try to match a connection only
with this port. If no connections with this port, we should return an
error.

I think that it is real problem. I faced with a real application
freenx server that uses CIFS for mounting many shares on different
ports (freenx clients want mount to mount its own share through ssh
port forwarding - many smbds with non-standard ports and local smbd
with standard port) on localhost.

You can try the following to reproduce the bug:
run smbd's on localhost and smbserver_host (another).

On the first console:
ssh -L5000:smbserver_host:445 smbserver_host
On the second:
mount -t cifs //localhost/share ~/test -oport=5000
mount -t cifs //localhost/share ~/test2

As the result, we can see that the second call connects to
smbserve_host rather than to localhost and return without any error -
so, user doesn't know that it has mounted smbserver_host instead of
localhost.

Even more, if we unmounted and mount them vice versa:
mount -t cifs //localhost/share ~/test2
mount -t cifs //localhost/share ~/test -oport=5000

it will be ok - two different connection to two different share, but
(!) if we losing the connection to port 5000, today's code reconnects
with share to localhost - wrong again! My patch number 1 fixes this
situation too.

I am ready to provide changes to the patches I posted later according
to mount.cifs spec as soon as we find an agreement in all the things
above.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/15 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Sun, 14 Nov 2010 12:38:18 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> > 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> >>
>> >> I don't think this patch deals correctly with servers that are
>> >> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
>> >> same server that don't specify a port, you'll end up with two sockets,
>> >> right?
>> >
>>
>> And from another hand: If user doesn't specify the port we should
>> think that it means the 445 port. If user wants to mount 139 port, he
>> should specify this port manually. So, there is no error with the
>> patch in this case.
>>
>> From this point of view we should remove trying with 139 port if we
>> failed with 445 port. What do you think about it?
>>
>
> That sounds like a regression. The mount.cifs manpage says:
>
>       port=arg
>           sets the port number on the server to attempt to contact to
>           negotiate CIFS support. If the CIFS server is not listening on this
>           port or if it is not specified, the default ports will be tried
>           i.e. port 445 is tried and if no response then port 139 is tried.
>
>
> I think we ought to preserve that behavior. Perhaps if no port is
> specified then match any TCP session that is on port 445 or port 139?
>

Jeff, according to my patches (try #2) I think we should replace it this with:

   port=arg
           sets the port number on the server to attempt to contact to
negotiate CIFS support.
           If the CIFS server is not listening on this port, we return
with error. If it is not specified,
           the default ports will be tried i.e. port 445 is tried and
if no response then port 139 is tried.


-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

Even more. I think it should be the following:

port=arg

sets the port number on the server to attempt to contact to negotiate
CIFS support. If this value is specified, looking for an existing
connection with this port and try to connect if no such a connection.
Return an error if it fails.

If this value isn't specified, looking for an existing connection with
445 or 139 port. If no such a connection, try to connect with 445 port
and if it fails - with 139 port. Return an error if both fail.
-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Jeff Layton]

On Tue, 16 Nov 2010 10:51:23 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/11/15 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Sun, 14 Nov 2010 12:38:18 +0300
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> 2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> > 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> >> >>
> >> >> I don't think this patch deals correctly with servers that are
> >> >> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
> >> >> same server that don't specify a port, you'll end up with two sockets,
> >> >> right?
> >> >
> >>
> >> And from another hand: If user doesn't specify the port we should
> >> think that it means the 445 port. If user wants to mount 139 port, he
> >> should specify this port manually. So, there is no error with the
> >> patch in this case.
> >>
> >> From this point of view we should remove trying with 139 port if we
> >> failed with 445 port. What do you think about it?
> >>
> >
> > That sounds like a regression. The mount.cifs manpage says:
> >
> >       port=arg
> >           sets the port number on the server to attempt to contact to
> >           negotiate CIFS support. If the CIFS server is not listening on this
> >           port or if it is not specified, the default ports will be tried
> >           i.e. port 445 is tried and if no response then port 139 is tried.
> >
> >
> > I think we ought to preserve that behavior. Perhaps if no port is
> > specified then match any TCP session that is on port 445 or port 139?
> >
> 
> Jeff, according to my patches (try #2) I think we should replace it this with:
> 
>    port=arg
>            sets the port number on the server to attempt to contact to
> negotiate CIFS support.
>            If the CIFS server is not listening on this port, we return
> with error. If it is not specified,
>            the default ports will be tried i.e. port 445 is tried and
> if no response then port 139 is tried.
> 
> 

Sounds correct to me. Care to do a manpage patch for mount.cifs?

Thanks,
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] CIFS: Fix mounting share on non-standard ports

[Pavel Shilovsky]

2010/11/16 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Tue, 16 Nov 2010 10:51:23 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2010/11/15 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Sun, 14 Nov 2010 12:38:18 +0300
>> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> 2010/11/13 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> >> > 2010/11/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> >> >>
>> >> >> I don't think this patch deals correctly with servers that are
>> >> >> listening on RFC1001_PORT but not CIFS_PORT. With two mounts to the
>> >> >> same server that don't specify a port, you'll end up with two sockets,
>> >> >> right?
>> >> >
>> >>
>> >> And from another hand: If user doesn't specify the port we should
>> >> think that it means the 445 port. If user wants to mount 139 port, he
>> >> should specify this port manually. So, there is no error with the
>> >> patch in this case.
>> >>
>> >> From this point of view we should remove trying with 139 port if we
>> >> failed with 445 port. What do you think about it?
>> >>
>> >
>> > That sounds like a regression. The mount.cifs manpage says:
>> >
>> >       port=arg
>> >           sets the port number on the server to attempt to contact to
>> >           negotiate CIFS support. If the CIFS server is not listening on this
>> >           port or if it is not specified, the default ports will be tried
>> >           i.e. port 445 is tried and if no response then port 139 is tried.
>> >
>> >
>> > I think we ought to preserve that behavior. Perhaps if no port is
>> > specified then match any TCP session that is on port 445 or port 139?
>> >
>>
>> Jeff, according to my patches (try #2) I think we should replace it this with:
>>
>>    port=arg
>>            sets the port number on the server to attempt to contact to
>> negotiate CIFS support.
>>            If the CIFS server is not listening on this port, we return
>> with error. If it is not specified,
>>            the default ports will be tried i.e. port 445 is tried and
>> if no response then port 139 is tried.
>>
>>
>
> Sounds correct to me. Care to do a manpage patch for mount.cifs?
>

Ok, I will provide a patch with this variant:
"port=arg

sets the port number on the server to attempt to contact to negotiate
CIFS support. If this value is specified, looking for an existing
connection with this port and try to connect if no such a connection.
Return an error if it fails.

If this value isn't specified, looking for an existing connection with
445 or 139 port. If no such a connection, try to connect with 445 port
and if it fails - with 139 port. Return an error if both fail."

-- 
Best regards,
Pavel Shilovsky.