Fix SMB2_TREE_CONNECT requests with the wrong TreeId

Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Jan-Marek Glogowski]

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

Hi everyone,

I'm having problems with CIFS connections to our NAS. The vendor
inspected the problem and claimed the Linux CIFS module doesn't follow
the specifications, which causes the instabilities + shutdown problems
with our LiMux clients in our SMB2+ tests.

Quoting http://msdn.microsoft.com/en-us/library/cc246529.aspx:

"TreeId (4 bytes): Uniquely identifies the tree connect for the
command. This MUST be 0 for the SMB2 TREE_CONNECT Request."

I applied the attached "warn" diff, which resulted in the (expected)
following stack trace (albeit for an old Ubuntu kernel), but otherwise
fixes the problem.

[ 1815.635274] ------------[ cut here ]------------
[ 1815.635294] WARNING: CPU: 1 PID: 65 at
/tmp/cifs-3.13.0-100.147-patched/smb2pdu.c:164
small_smb2_init+0x246/0x560 [cifs]()
[ 1815.635295] smb2_reconnect: SMB2_TREE_CONNECT with tid != 0
[ 1815.635296] Modules linked in: nls_iso8859_1 usb_storage arc4 md4
nls_utf8 cifs(OX) fscache kav4fs_oas(OX) redirfs(OX) dm_crypt rfcomm
bnep bluetooth x86_pkg_temp_thermal intel_powerclamp coretemp
snd_hda_codec_hdmi snd_hda_codec_realtek kvm_intel kvm joydev
snd_hda_intel snd_hda_codec snd_hwdep crc32_pclmul snd_pcm
snd_page_alloc serio_raw snd_seq_midi snd_seq_midi_event lpc_ich ipmi_si
snd_rawmidi snd_seq snd_seq_device snd_timer parport_pc ppdev snd shpchp
lp mac_hid soundcore parport hid_generic usbhid hid i915 i2c_algo_bit
drm_kms_helper ahci drm r8169 libahci psmouse mii wmi video
[ 1815.635327] CPU: 1 PID: 65 Comm: kworker/1:2 Tainted: G        W  OX
3.13.0-100-generic #147-Ubuntu
[ 1815.635328] Hardware name: Acer Veriton M4630G/Veriton M4630G, BIOS
P11-C1L                 04/10/2015
[ 1815.635344] Workqueue: cifsiod cifs_writev_complete [cifs]
[ 1815.635346]  00000286 00000286 f65edd68 c165dcd2 f65edda8 f8cd0f78
f65edd98 c105798e
[ 1815.635350]  f8cd102c f65eddc4 00000041 f8cd0f78 000000a4 f8cb1df6
f8cb1df6 f4347800
[ 1815.635353]  00000044 f4452c00 f65eddb0 c10579e3 00000009 f65edda8
f8cd102c f65eddc4
[ 1815.635357] Call Trace:
[ 1815.635362]  [<c165dcd2>] dump_stack+0x58/0x72
[ 1815.635366]  [<c105798e>] warn_slowpath_common+0x7e/0xa0
[ 1815.635378]  [<f8cb1df6>] ? small_smb2_init+0x246/0x560 [cifs]
[ 1815.635390]  [<f8cb1df6>] ? small_smb2_init+0x246/0x560 [cifs]
[ 1815.635392]  [<c10579e3>] warn_slowpath_fmt+0x33/0x40
[ 1815.635404]  [<f8cb1df6>] small_smb2_init+0x246/0x560 [cifs]
[ 1815.635416]  [<f8ca1b80>] ? cifs_strtoUTF16+0xc0/0xf0 [cifs]
[ 1815.635427]  [<f8cb17f9>] SMB2_tcon+0xb9/0x470 [cifs]
[ 1815.635438]  [<f8cb2580>] ? SMB2_negotiate+0x470/0x470 [cifs]
[ 1815.635449]  [<f8cb1fc5>] small_smb2_init+0x415/0x560 [cifs]
[ 1815.635452]  [<c1092ca0>] ? prepare_to_wait_event+0xd0/0xd0
[ 1815.635464]  [<f8cb45c4>] smb2_async_writev+0x34/0x1d0 [cifs]
[ 1815.635475]  [<f8c7a8ae>] ? cifs_writedata_release+0x1e/0x30 [cifs]
[ 1815.635478]  [<c112d547>] ? clear_page_dirty_for_io+0x57/0xe0
[ 1815.635488]  [<f8c7de3a>] cifs_writev_complete+0x1aa/0x270 [cifs]
[ 1815.635491]  [<c10702ba>] process_one_work+0x11a/0x3c0
[ 1815.635493]  [<c1070f49>] worker_thread+0xf9/0x380
[ 1815.635495]  [<c1070e50>] ? rescuer_thread+0x380/0x380
[ 1815.635497]  [<c10767cb>] kthread+0x9b/0xb0
[ 1815.635500]  [<c166c037>] ret_from_kernel_thread+0x1b/0x28
[ 1815.635503]  [<c1076730>] ? kthread_create_on_node+0x140/0x140
[ 1815.635504] ---[ end trace d45909fef64cdfda ]---
[ 1815.655889] cifs_vfs_err: 2516 callbacks suppressed
[ 1815.655893] CIFS VFS: cifs_invalidate_mapping: could not invalidate
inode d92e3798

I don't know if we want to zero tcon for new connections / reconnects.
The "warn" diff was tested by a colleague and "works for him".

Thanks,

Jan-Marek

P.S. The 2nd patch without the warning was just compile-tested.

[-- Attachment #2: tree_connect_with_warn.diff --]
[-- Type: text/x-patch, Size: 371 bytes --]

--- smb2pdu.c.orig	2017-01-20 19:58:34.581064153 +0100
+++ smb2pdu.c	2017-01-20 20:19:23.124772222 +0100
@@ -172,7 +172,14 @@
 		return rc;
 
 	if (smb2_command == SMB2_TREE_CONNECT)
+	{
+		if (tcon->tid != 0)
+		{
+			WARN(1, "%s: SMB2_TREE_CONNECT with tid != 0\n", __func__);
+			tcon->tid = 0;
+		}
 		return rc;
+	}
 
 	if (tcon->tidStatus == CifsExiting) {
 		/*



[-- Attachment #3: 0001-Reset-TreeId-to-zero-on-SMB2_TREE_CONNECT.patch --]
[-- Type: text/x-patch, Size: 945 bytes --]

>From 249ab089b1c83dfbe22a1a0b37395ccd394ae1ad Mon Sep 17 00:00:00 2001
From: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
Date: Fri, 10 Feb 2017 16:43:46 +0100
Subject: [PATCH] Reset TreeId to zero on SMB2_TREE_CONNECT

Currently the cifs module breaks the CIFS specs as described in
http://msdn.microsoft.com/en-us/library/cc246529.aspx:

"TreeId (4 bytes): Uniquely identifies the tree connect for the
command. This MUST be 0 for the SMB2 TREE_CONNECT Request."

Signed-off-by: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 8745722..ad1b555 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -169,7 +169,10 @@
 		return rc;
 
 	if (smb2_command == SMB2_TREE_CONNECT)
+	{
+		tcon->tid = 0;
 		return rc;
+	}
 
 	if (tcon->tidStatus == CifsExiting) {
 		/*
-- 
1.9.1

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Aurélien Aptel]

Hi Jan-Marek,

Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org> writes:
> I'm having problems with CIFS connections to our NAS. The vendor
> inspected the problem and claimed the Linux CIFS module doesn't follow
> the specifications, which causes the instabilities + shutdown problems
> with our LiMux clients in our SMB2+ tests.
>
> Quoting http://msdn.microsoft.com/en-us/library/cc246529.aspx:
>
> "TreeId (4 bytes): Uniquely identifies the tree connect for the
> command. This MUST be 0 for the SMB2 TREE_CONNECT Request."
>
> I applied the attached "warn" diff, which resulted in the (expected)
> following stack trace (albeit for an old Ubuntu kernel), but otherwise
> fixes the problem.

I looks like the tid should be initialized to 0 already. Could you
provide a network trace of the problem happening? See [1] if you need
help.

1: https://wiki.samba.org/index.php/Capture_Packets

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Aurélien Aptel]

Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org> writes:
> I also had a look at the CIFS code and the tcon->tid member is just used
> to set the SMB2 header TreeId field or is set from the the SMB2 header
> TreeId field. If tcon structs are reused, I couldn't see a code path to
> zero it.

wasn't thinking of the reconnection, good catch!

I'm still not sure why you got the stacktrace on the old kernel. I'll
try it out.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Jan-Marek Glogowski]

Am 14.02.2017 um 17:02 schrieb Aurélien Aptel:
> Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org> writes:
>> I also had a look at the CIFS code and the tcon->tid member is just used
>> to set the SMB2 header TreeId field or is set from the the SMB2 header
>> TreeId field. If tcon structs are reused, I couldn't see a code path to
>> zero it.
> 
> wasn't thinking of the reconnection, good catch!
> 
> I'm still not sure why you got the stacktrace on the old kernel. I'll
> try it out.

The trace starts with

[ 1815.635294] WARNING: CPU: 1 PID: 65 at
/tmp/cifs-3.13.0-100.147-patched/smb2pdu.c:164
small_smb2_init+0x246/0x560 [cifs]()

which is the place of the WARN call in my code. Since I also had a patch
for a newer kernel (Ubuntu 16.04 HWE for 14.04 AKA Linux 4.4), the
offsets are wrong in the attached patch.

I just re-checked my code. I didn't test the non-WARN variant and
assumed the code simply got inlined, so the function name is wrong.

Jan-Marek

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Aurélien Aptel]

Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org> writes:
> which is the place of the WARN call in my code. Since I also had a patch
> for a newer kernel (Ubuntu 16.04 HWE for 14.04 AKA Linux 4.4), the
> offsets are wrong in the attached patch.
>
> I just re-checked my code. I didn't test the non-WARN variant and
> assumed the code simply got inlined, so the function name is wrong.

I've just realized WARN() itself dumps the stacktrace and that your first
email said "expected" (not "unexpected")... *sigh* I was tired that day.

The reconnect logic is a bit convulted but patching where you did seems
correct. Calling smb2_reconnect with the Tree Connect command will reset
the Tid. Calling it with any command other command will end up calling
smb2_reconnect again with the Tree Connect.

    smb2_reconnect(cmd, ...)
      if (cmd == SMB2_TREE_CONNECT)
          TID = 0, return
      else    
          SMB2_tcon()
            small_smb2_init(SMB2_TREE_CONNECT, ...)
              smb2_reconnect(SMB2_TREE_CONNECT, ...)

I think its clearer and more explicit if we reset the Tid everytime we
send a Tree Con request. So in SMB2_tcon(), before the SendReceive2().

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Jan-Marek Glogowski]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Am 15.02.2017 um 14:05 schrieb Aurélien Aptel:
> Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org> writes:
>> which is the place of the WARN call in my code. Since I also had a patch
>> for a newer kernel (Ubuntu 16.04 HWE for 14.04 AKA Linux 4.4), the
>> offsets are wrong in the attached patch.
>>
>> I just re-checked my code. I didn't test the non-WARN variant and
>> assumed the code simply got inlined, so the function name is wrong.
> 
> I've just realized WARN() itself dumps the stacktrace and that your first
> email said "expected" (not "unexpected")... *sigh* I was tired that day.
> 
> The reconnect logic is a bit convulted but patching where you did seems
> correct. Calling smb2_reconnect with the Tree Connect command will reset
> the Tid. Calling it with any command other command will end up calling
> smb2_reconnect again with the Tree Connect.
> 
>     smb2_reconnect(cmd, ...)
>       if (cmd == SMB2_TREE_CONNECT)
>           TID = 0, return
>       else    
>           SMB2_tcon()
>             small_smb2_init(SMB2_TREE_CONNECT, ...)
>               smb2_reconnect(SMB2_TREE_CONNECT, ...)
> 
> I think its clearer and more explicit if we reset the Tid everytime we
> send a Tree Con request. So in SMB2_tcon(), before the SendReceive2().

The code path is a little bit strange. I used SMB2_tcon as my "root"
when following the code paths, as it's externally used (AKA non-static)
and is assigned in the smb2ops.c as the function pointer.
So my view is a kind of "rotation" of your "call map".

My view results in SMB2_TREE_CONNECT to be just a special case of
small_smb2_init via smb2_reconnect and I expected small_smb2_init to do
the correct init. It should just modify the header (from the comment),
but it also changes the tcon, so we can't const it...

I actually never followed your "else" branch of smb2_reconnect, as it
doesn't matter for SMB2_TREE_CONNECT "init" in the SMB2_tcon call.

OTOH I don't see an easy way to make the reconnect more "sequential",
don't know a better word for non-recursive here.

I won't be able to test the patch today but hopefully tomorrow,
depending on the current LiMux stuff. I'm tired too. It compiled for my
old kernel and should work.

Jan-Marek

P.S. today I had the meeting with some people from the Hitachi NAS
support. They wanted to send me a link to a trace they made in their
test lab with our original bug. I can forward it when I get it, if still
needed.

[-- Attachment #2: 0001-Reset-TreeId-to-zero-on-SMB2_TREE_CONNECT.patch --]
[-- Type: text/x-patch, Size: 1149 bytes --]

>From be24636f5a7f26ef964f373ba0a79bc0553c4e4d Mon Sep 17 00:00:00 2001
From: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
Date: Fri, 10 Feb 2017 16:43:46 +0100
Subject: [PATCH] Reset TreeId to zero on SMB2_TREE_CONNECT

Currently the cifs module breaks the CIFS specs on reconnect as
described in http://msdn.microsoft.com/en-us/library/cc246529.aspx:

"TreeId (4 bytes): Uniquely identifies the tree connect for the
command. This MUST be 0 for the SMB2 TREE_CONNECT Request."

Signed-off-by: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 8745722..8f310672 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1115,7 +1115,8 @@ static inline void init_copy_chunk_defaults(struct cifs_tcon *tcon)
 		req->hdr.SessionId = ses->Suid;
 		/* if (ses->server->sec_mode & SECMODE_SIGN_REQUIRED)
 			req->hdr.Flags |= SMB2_FLAGS_SIGNED; */
-	}
+	} else
+		tcon->tid = 0;
 
 	iov[0].iov_base = (char *)req;
 	/* 4 for rfc1002 length field and 1 for pad */
-- 
1.9.1

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Jan-Marek Glogowski]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

Am 15.02.2017 um 15:24 schrieb Jan-Marek Glogowski:
> I won't be able to test the patch today but hopefully tomorrow,
> depending on the current LiMux stuff. I'm tired too. It compiled for my
> old kernel and should work.

Took some time to test the 2nd patch and it turned out it doesn't work.
The TreeId must be reset before small_smb2_init(SMB2_TREE_CONNECT, ...)
call, as this copies the tcon->tid into the smb2 header.

This is fixed in the attached patch.

Can I add "Reported-by" or "Tested-by" for myself and Suggested-by, and
Reviewed-by for you?

Do you want to test it?

Jan-Marek

[-- Attachment #2: 0001-Reset-TreeId-to-zero-on-SMB2_TREE_CONNECT.patch --]
[-- Type: text/x-patch, Size: 1104 bytes --]

>From faf5ea71a567fa9f68daf296e0fa27823f968ee8 Mon Sep 17 00:00:00 2001
From: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
Date: Fri, 10 Feb 2017 16:43:46 +0100
Subject: [PATCH] Reset TreeId to zero on SMB2_TREE_CONNECT

Currently the cifs module breaks the CIFS specs on reconnect as
described in http://msdn.microsoft.com/en-us/library/cc246529.aspx:

"TreeId (4 bytes): Uniquely identifies the tree connect for the
command. This MUST be 0 for the SMB2 TREE_CONNECT Request."

Signed-off-by: Jan-Marek Glogowski <glogow-iG8UcmBuX++ELgA04lAiVw@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 8745722..bdd3292 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1104,6 +1104,10 @@ static inline void init_copy_chunk_defaults(struct cifs_tcon *tcon)
 		return -EINVAL;
 	}
 
+	/* SMB2 TREE_CONNECT request must be called with TreeId == 0 */
+	if (tcon)
+		tcon->tid = 0;
+
 	rc = small_smb2_init(SMB2_TREE_CONNECT, tcon, (void **) &req);
 	if (rc) {
 		kfree(unc_path);
-- 
1.9.1

Re: Fix SMB2_TREE_CONNECT requests with the wrong TreeId

[Aurélien Aptel]

I have reproduced the issue and can confirm your latest patch fixes it.

Reviewed-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Tested-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)