Patchwork cifs: Fix broken mounts when SSH tunnel is used

login
register
mail settings
Submitter Suresh Jayaraman
Date Aug. 18, 2009, 5:01 p.m.
Message ID <4A8ADE80.6040306@suse.de>
Download mbox | patch
Permalink /patch/31578/
State New
Headers show

Comments

Suresh Jayaraman - Aug. 18, 2009, 5:01 p.m.
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 this by adding a check to verify whether the port, before deciding that a
existing tcp session is found and can be used. 

Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/cifs/connect.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Jeff Layton - Aug. 18, 2009, 5:12 p.m.
On Tue, 18 Aug 2009 22:31:52 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> 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 this by adding a check to verify whether the port, before deciding that a
> existing tcp session is found and can be used. 
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/cifs/connect.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 1f3345d..c00159c 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1399,13 +1399,15 @@ cifs_find_tcp_session(struct sockaddr_storage *addr)
>  
>  		if (addr->ss_family == AF_INET &&
			^^^^
		The ss_family checks would look a lot cleaner as a
		switch statement. Can you fix that while you're at it?

>  		    (addr4->sin_addr.s_addr !=
> -		     server->addr.sockAddr.sin_addr.s_addr))
> +		     server->addr.sockAddr.sin_addr.s_addr ||
> +		     addr4->sin_port != server->addr.sockAddr.sin_port))
			^^^^
			I don't think addr4/addr6 have their ports set
			at this point. And in any case, you need to
			determine how to deal with the situation where
			someone hasn't set port= at all. In that case,
			this comparison will fail and you'll end up
			not sharing sockets when you could have.

>  			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))
> +					   addr6->sin6_scope_id ||
> +			  server->addr.sockAddr6.sin6_port != addr6->sin6_port))
>  			continue;
>  
>  		++server->srv_count;
Suresh Jayaraman - Aug. 19, 2009, 7:02 a.m.
Jeff Layton wrote:
> On Tue, 18 Aug 2009 22:31:52 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> 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 this by adding a check to verify whether the port, before deciding that a
>> existing tcp session is found and can be used. 
>>
>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>> ---
>>  fs/cifs/connect.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 1f3345d..c00159c 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -1399,13 +1399,15 @@ cifs_find_tcp_session(struct sockaddr_storage *addr)
>>  
>>  		if (addr->ss_family == AF_INET &&
> 			^^^^
> 		The ss_family checks would look a lot cleaner as a
> 		switch statement. Can you fix that while you're at it?

Yeah, sure.

>>  		    (addr4->sin_addr.s_addr !=
>> -		     server->addr.sockAddr.sin_addr.s_addr))
>> +		     server->addr.sockAddr.sin_addr.s_addr ||
>> +		     addr4->sin_port != server->addr.sockAddr.sin_port))
> 			^^^^
> 			I don't think addr4/addr6 have their ports set
> 			at this point. And in any case, you need to
> 			determine how to deal with the situation where
> 			someone hasn't set port= at all. In that case,
> 			this comparison will fail and you'll end up
> 			not sharing sockets when you could have.

Good catch. Yeah, I think I assumed port had been populated like
sin_addr.s_addr and scope_id.. I'll resend the patch after fixing it.


Thanks,

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1f3345d..c00159c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1399,13 +1399,15 @@  cifs_find_tcp_session(struct sockaddr_storage *addr)
 
 		if (addr->ss_family == AF_INET &&
 		    (addr4->sin_addr.s_addr !=
-		     server->addr.sockAddr.sin_addr.s_addr))
+		     server->addr.sockAddr.sin_addr.s_addr ||
+		     addr4->sin_port != server->addr.sockAddr.sin_port))
 			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))
+					   addr6->sin6_scope_id ||
+			  server->addr.sockAddr6.sin6_port != addr6->sin6_port))
 			continue;
 
 		++server->srv_count;