From bdb97adcdf0993adbd2eef44b4533620d43792de Mon Sep 17 00:00:00 2001 From: Suresh Jayaraman Date: Thu, 20 Aug 2009 13:03:34 +0530 Subject: PATCH] cifs: fix broken mounts when a SSH tunnel is used (try #4) 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. Reviewed-by: Jeff Layton Reviewed-by: Shirish Pargaonkar Signed-off-by: Suresh Jayaraman Signed-off-by: Steve French --- fs/cifs/connect.c | 45 +++++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-) (limited to 'fs/cifs/connect.c') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 1f3345d7fa79..4e47a9b800ce 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; -- cgit v1.2.2 From ca43e3beee38623a3f80691b28623d6ecf214742 Mon Sep 17 00:00:00 2001 From: Steve French Date: Tue, 1 Sep 2009 17:20:50 +0000 Subject: [CIFS] Fix checkpatch warnings Also update version number to 1.61 Signed-off-by: Steve French --- fs/cifs/connect.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'fs/cifs/connect.c') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 4e47a9b800ce..d49682433c20 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1415,17 +1415,17 @@ cifs_find_tcp_session(struct sockaddr_storage *addr, unsigned short int port) case AF_INET6: if (ipv6_addr_equal(&addr6->sin6_addr, &server->addr.sockAddr6.sin6_addr) && - (addr6->sin6_scope_id == + (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 != + if (addr6->sin6_port != server->addr.sockAddr6.sin6_port) - continue; + continue; } break; - } else + } else continue; } @@ -2657,9 +2657,9 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, return -EIO; smb_buffer = cifs_buf_get(); - if (smb_buffer == NULL) { + if (smb_buffer == NULL) return -ENOMEM; - } + smb_buffer_response = smb_buffer; header_assemble(smb_buffer, SMB_COM_TREE_CONNECT_ANDX, -- cgit v1.2.2 From 3bc303c254335dbd7c7012cc1760b12f1d5514d3 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 21 Sep 2009 06:47:50 -0400 Subject: cifs: convert oplock breaks to use slow_work facility (try #4) This is the fourth respin of the patch to convert oplock breaks to use the slow_work facility. A customer of ours was testing a backport of one of the earlier patchsets, and hit a "Busy inodes after umount..." problem. An oplock break job had raced with a umount, and the superblock got torn down and its memory reused. When the oplock break job tried to dereference the inode->i_sb, the kernel oopsed. This patchset has the oplock break job hold an inode and vfsmount reference until the oplock break completes. With this, there should be no need to take a tcon reference (the vfsmount implicitly holds one already). Currently, when an oplock break comes in there's a chance that the oplock break job won't occur if the allocation of the oplock_q_entry fails. There are also some rather nasty races in the allocation and handling these structs. Rather than allocating oplock queue entries when an oplock break comes in, add a few extra fields to the cifsFileInfo struct. Get rid of the dedicated cifs_oplock_thread as well and queue the oplock break job to the slow_work thread pool. This approach also has the advantage that the oplock break jobs can potentially run in parallel rather than be serialized like they are today. Signed-off-by: Jeff Layton Signed-off-by: Steve French --- fs/cifs/connect.c | 1 - 1 file changed, 1 deletion(-) (limited to 'fs/cifs/connect.c') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index d49682433c20..43003e0bef18 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1670,7 +1670,6 @@ cifs_put_tcon(struct cifsTconInfo *tcon) CIFSSMBTDis(xid, tcon); _FreeXid(xid); - DeleteTconOplockQEntries(tcon); tconInfoFree(tcon); cifs_put_smb_ses(ses); } -- cgit v1.2.2 From 8347a5cdd1422eea0470ed586274c7f29e274b47 Mon Sep 17 00:00:00 2001 From: Steve French Date: Tue, 6 Oct 2009 18:31:29 +0000 Subject: [CIFS] Fixing to avoid invalid kfree() in cifs_get_tcp_session() trivial bug in fs/cifs/connect.c . The bug is caused by fail of extract_hostname() when mounting cifs file system. This is the situation when I noticed this bug. % sudo mount -t cifs //192.168.10.208 mountpoint -o options... Then my kernel says, [ 1461.807776] ------------[ cut here ]------------ [ 1461.807781] kernel BUG at mm/slab.c:521! [ 1461.807784] invalid opcode: 0000 [#2] PREEMPT SMP [ 1461.807790] last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:09:02.0/resource [ 1461.807793] CPU 0 [ 1461.807796] Modules linked in: nls_iso8859_1 usbhid sbp2 uhci_hcd ehci_hcd i2c_i801 ohci1394 ieee1394 psmouse serio_raw pcspkr sky2 usbcore evdev [ 1461.807816] Pid: 3446, comm: mount Tainted: G D 2.6.32-rc2-vanilla [ 1461.807820] RIP: 0010:[] [] kfree+0x63/0x156 [ 1461.807829] RSP: 0018:ffff8800b4f7fbb8 EFLAGS: 00010046 [ 1461.807832] RAX: ffffea00033fff98 RBX: ffff8800afbae7e2 RCX: 0000000000000000 [ 1461.807836] RDX: ffffea0000000000 RSI: 000000000000005c RDI: ffffffffffffffea [ 1461.807839] RBP: ffff8800b4f7fbf8 R08: 0000000000000001 R09: 0000000000000000 [ 1461.807842] R10: 0000000000000000 R11: ffff8800b4f7fbf8 R12: 00000000ffffffea [ 1461.807845] R13: ffff8800afb23000 R14: ffff8800b4f87bc0 R15: ffffffffffffffea [ 1461.807849] FS: 00007f52b6f187c0(0000) GS:ffff880007600000(0000) knlGS:0000000000000000 [ 1461.807852] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [ 1461.807855] CR2: 0000000000613000 CR3: 00000000af8f9000 CR4: 00000000000006f0 [ 1461.807858] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1461.807861] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [ 1461.807865] Process mount (pid: 3446, threadinfo ffff8800b4f7e000, task ffff8800950e4380) [ 1461.807867] Stack: [ 1461.807869] 0000000000000202 0000000000000282 ffff8800b4f7fbf8 ffff8800afbae7e2 [ 1461.807876] <0> 00000000ffffffea ffff8800afb23000 ffff8800b4f87bc0 ffff8800b4f7fc28 [ 1461.807884] <0> ffff8800b4f7fcd8 ffffffff81159f6d ffffffff81147bc2 ffffffff816bfb48 [ 1461.807892] Call Trace: [ 1461.807899] [] cifs_get_tcp_session+0x440/0x44b [ 1461.807904] [] ? find_nls+0x1c/0xe9 [ 1461.807909] [] cifs_mount+0x16bc/0x2167 [ 1461.807917] [] ? _spin_unlock+0x30/0x4b [ 1461.807923] [] cifs_get_sb+0xa5/0x1a8 [ 1461.807928] [] vfs_kern_mount+0x56/0xc9 [ 1461.807933] [] do_kern_mount+0x47/0xe7 [ 1461.807938] [] do_mount+0x712/0x775 [ 1461.807943] [] ? copy_mount_options+0xcf/0x132 [ 1461.807948] [] sys_mount+0x7f/0xbf [ 1461.807953] [] ? lockdep_sys_exit_thunk+0x35/0x67 [ 1461.807960] [] system_call_fastpath+0x16/0x1b [ 1461.807963] Code: 00 00 00 00 ea ff ff 48 c1 e8 0c 48 6b c0 68 48 01 d0 66 83 38 00 79 04 48 8b 40 10 66 83 38 00 79 04 48 8b 40 10 80 38 00 78 04 <0f> 0b eb fe 4c 8b 70 58 4c 89 ff 41 8b 76 4c e8 b8 49 fb ff e8 [ 1461.808022] RIP [] kfree+0x63/0x156 [ 1461.808027] RSP [ 1461.808031] ---[ end trace ffe26fcdc72c0ce4 ]--- The reason of this bug is that the error handling code of cifs_get_tcp_session() calls kfree() when corresponding kmalloc() failed. (The kmalloc() is called by extract_hostname().) Signed-off-by: Hitoshi Mitake CC: Stable Reviewed-by: Jeff Layton Signed-off-by: Steve French --- fs/cifs/connect.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs/cifs/connect.c') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 43003e0bef18..b09098079916 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1577,7 +1577,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info) out_err: if (tcp_ses) { - kfree(tcp_ses->hostname); + if (!IS_ERR(tcp_ses->hostname)) + kfree(tcp_ses->hostname); if (tcp_ses->ssocket) sock_release(tcp_ses->ssocket); kfree(tcp_ses); -- cgit v1.2.2 From f475f6775465283494346663f201ad04810d2e8a Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 6 Nov 2009 14:18:49 -0500 Subject: cifs: don't use CIFSGetSrvInodeNumber in is_path_accessible Because it's lighter weight, CIFS tries to use CIFSGetSrvInodeNumber to verify the accessibility of the root inode and then falls back to doing a full QPathInfo if that fails with -EOPNOTSUPP. I have at least a report of a server that returns NT_STATUS_INTERNAL_ERROR rather than something that translates to EOPNOTSUPP. Rather than trying to be clever with that call, just have is_path_accessible do a normal QPathInfo. That call is widely supported and it shouldn't increase the overhead significantly. Cc: Stable Signed-off-by: Jeff Layton Signed-off-by: Steve French --- fs/cifs/connect.c | 8 -------- 1 file changed, 8 deletions(-) (limited to 'fs/cifs/connect.c') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index b09098079916..63ea83ff687f 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2220,16 +2220,8 @@ is_path_accessible(int xid, struct cifsTconInfo *tcon, struct cifs_sb_info *cifs_sb, const char *full_path) { int rc; - __u64 inode_num; FILE_ALL_INFO *pfile_info; - rc = CIFSGetSrvInodeNumber(xid, tcon, full_path, &inode_num, - cifs_sb->local_nls, - cifs_sb->mnt_cifs_flags & - CIFS_MOUNT_MAP_SPECIAL_CHR); - if (rc != -EOPNOTSUPP) - return rc; - pfile_info = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL); if (pfile_info == NULL) return -ENOMEM; -- cgit v1.2.2