| Message ID | 4A8ADE80.6040306@suse.de |
|---|---|
| State | New |
| Headers | show |
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;
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,
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;
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(-)