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

View: New views
3 Messages — Rating Filter:   Alert me  

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

by Suresh Jayaraman-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

One more try..

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 <sjayaraman@...>
---

 fs/cifs/connect.c |   45 +++++++++++++++++++++++++++++++++------------
 1 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1f3345d..4e47a9b 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,37 @@ 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:
+ if (addr4->sin_addr.s_addr ==
+    server->addr.sockAddr.sin_addr.s_addr) {
+ addr4->sin_port = htons(port);
+ /* user overrode default port? */
+ if (addr4->sin_port) {
+ if (addr4->sin_port !=
+    server->addr.sockAddr.sin_port)
+ continue;
+ }
+ break;
+ } else
+ continue;
+
+ case AF_INET6:
+ if (ipv6_addr_equal(&addr6->sin6_addr,
+    &server->addr.sockAddr6.sin6_addr) &&
+    (addr6->sin6_scope_id ==
+    server->addr.sockAddr6.sin6_scope_id)) {
+ addr6->sin6_port = htons(port);
+ /* user overrode default port? */
+ if (addr6->sin6_port) {
+       if (addr6->sin6_port !=
+   server->addr.sockAddr6.sin6_port)
+       continue;
+ }
+ break;
+ } else
+ continue;
+ }
 
  ++server->srv_count;
  write_unlock(&cifs_tcp_ses_lock);
@@ -1475,7 +1496,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;
 
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Jeff Layton-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Thu, 20 Aug 2009 13:03:34 +0530
Suresh Jayaraman <sjayaraman@...> wrote:

> One more try..
>
> 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 <sjayaraman@...>
> ---
>


Still a little more nested that I would have preferred...

I'd have probably had it do this for each family:

if (addrs don't match)
        continue;
if (ports don't match)
        continue
break;

...but that's a rather minor point.

Acked-by: Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

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

by Steve French-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Merged.

Note I fixed some checkpatch warnings thrown when I checked in this patch in a followon patch immediately after merging this.

On Thu, Aug 20, 2009 at 2:33 AM, Suresh Jayaraman <sjayaraman@...> wrote:
One more try..

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 <sjayaraman@...>
---

 fs/cifs/connect.c |   45 +++++++++++++++++++++++++++++++++------------
 1 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1f3345d..4e47a9b 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,37 @@ 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:
+                       if (addr4->sin_addr.s_addr ==
+                           server->addr.sockAddr.sin_addr.s_addr) {
+                               addr4->sin_port = htons(port);
+                               /* user overrode default port? */
+                               if (addr4->sin_port) {
+                                       if (addr4->sin_port !=
+                                           server->addr.sockAddr.sin_port)
+                                               continue;
+                               }
+                               break;
+                       } else
+                               continue;
+
+               case AF_INET6:
+                       if (ipv6_addr_equal(&addr6->sin6_addr,
+                           &server->addr.sockAddr6.sin6_addr) &&
+                           (addr6->sin6_scope_id ==
+                           server->addr.sockAddr6.sin6_scope_id)) {
+                               addr6->sin6_port = htons(port);
+                               /* user overrode default port? */
+                               if (addr6->sin6_port) {
+                                      if (addr6->sin6_port !=
+                                          server->addr.sockAddr6.sin6_port)
+                                              continue;
+                               }
+                               break;
+                       } else
+                               continue;
+               }

               ++server->srv_count;
               write_unlock(&cifs_tcp_ses_lock);
@@ -1475,7 +1496,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;




--
Thanks,

Steve

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client