diff mbox series

cifs: fix a credits leak for compund commands

Message ID 20181010052906.23861-2-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show
Series cifs: fix a credits leak for compund commands | expand

Commit Message

Ronnie Sahlberg Oct. 10, 2018, 5:29 a.m. UTC 
When processing the mids for compounds we would only add credits based on
the last successful mid in the compound which would leak credits and
eventually triggering a re-connect.

Fix this by splitting the mid processing part into two loops instead of one
where the first loop just waits for all mids and then counts how many
credits we were granted for the whole compound.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/transport.c | 57 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 20 deletions(-)

Comments

Steve French Oct. 10, 2018, 5:53 a.m. UTC | #1 
this does address the problem with the previous version of the patch.
I did some tests on it just now

With your fix test 091 logs the following

[ 1007.477182] run fstests generic/091 at 2018-10-10 00:41:49
[ 1014.991772] server overflowed SMB3 credits


I also still see this oops on test 377 (which may be unrelated to your
patch, but is an oops ...)

[ 1432.339015] run fstests generic/377 at 2018-10-10 00:48:54
[ 1432.522155] CIFS: Attempting to mount //localhost/test
[ 1432.522180] CIFS VFS: mount options mfsymlinks and sfu both enabled
[ 1432.651023] CIFS: Attempting to mount //localhost/scratch
[ 1432.651090] CIFS VFS: mount options mfsymlinks and sfu both enabled
[ 1432.712166] CIFS: Attempting to mount //localhost/scratch
[ 1432.712203] CIFS VFS: mount options mfsymlinks and sfu both enabled
[ 1432.735373] usercopy: Kernel memory exposure attempt detected from
SLUB object 'kmalloc-16' (offset 0, size 30)!
[ 1432.735383] ------------[ cut here ]------------
[ 1432.735384] kernel BUG at mm/usercopy.c:102!
[ 1432.735390] invalid opcode: 0000 [#1] SMP PTI
[ 1432.735394] CPU: 6 PID: 7000 Comm: listxattr Tainted: G
OE     4.19.0-041900rc7-generic #201810071631
[ 1432.735396] Hardware name: LENOVO 20HJS01E00/20HJS01E00, BIOS
N1UET71W (1.45 ) 07/18/2018
[ 1432.735403] RIP: 0010:usercopy_abort+0x7a/0x7c
[ 1432.735406] Code: 0f 45 c6 51 48 89 f9 48 c7 c2 45 9d 50 8e 41 52
48 c7 c6 29 8a 4f 8e 48 c7 c7 10 9e 50 8e 48 0f 45 f2 48 89 c2 e8 0f
03 e6 ff <0f> 0b 4d 89 e0 31 c9 44 89 ea 31 f6 48 c7 c7 79 9d 50 8e e8
6e ff
[ 1432.735409] RSP: 0018:ffffa8df102bfe30 EFLAGS: 00010246
[ 1432.735412] RAX: 0000000000000064 RBX: ffff9dadcddccea0 RCX: 0000000000000000
[ 1432.735414] RDX: 0000000000000000 RSI: ffff9daddf796428 RDI: ffff9daddf796428
[ 1432.735416] RBP: ffffa8df102bfe48 R08: 00000000000b388d R09: 00000000000004ef
[ 1432.735418] R10: 0000000000000004 R11: ffffffff8ed933ed R12: 000000000000001e
[ 1432.735420] R13: 0000000000000001 R14: ffff9dadcddccebe R15: ffff9dad35a62240
[ 1432.735423] FS:  00007fbc0779b740(0000) GS:ffff9daddf780000(0000)
knlGS:0000000000000000
[ 1432.735425] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1432.735427] CR2: 000055d070dd9008 CR3: 000000072ca1e005 CR4: 00000000003606e0
[ 1432.735430] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1432.735431] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1432.735433] Call Trace:
[ 1432.735440]  __check_heap_object+0xdf/0x110
[ 1432.735444]  __check_object_size+0x103/0x18a
[ 1432.735447]  listxattr+0xa6/0xe0
[ 1432.735450]  path_listxattr+0x63/0xb0
[ 1432.735454]  __x64_sys_listxattr+0x1f/0x30
[ 1432.735458]  do_syscall_64+0x5a/0x110
[ 1432.735462]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1432.735465] RIP: 0033:0x7fbc06ea0839
[ 1432.735468] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40
00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24
08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1f f6 2c 00 f7 d8 64 89
01 48
[ 1432.735470] RSP: 002b:00007fffda1e2348 EFLAGS: 00000206 ORIG_RAX:
00000000000000c2
[ 1432.735473] RAX: ffffffffffffffda RBX: 0000000000000009 RCX: 00007fbc06ea0839
[ 1432.735475] RDX: 0000000000000009 RSI: 000055d070dd9260 RDI: 00007fffda1e3a82
[ 1432.735477] RBP: 00007fffda1e2458 R08: 0000000000000000 R09: 000055d06f950b00
[ 1432.735479] R10: 0000000000000002 R11: 0000000000000206 R12: 000055d070dd9260
[ 1432.735481] R13: 00007fffda1e2450 R14: 0000000000000000 R15: 0000000000000000
[ 1432.735484] Modules linked in: md4 nls_utf8 cifs(OE) ccm fscache
vmnet(OE) vmw_vsock_vmci_transport vsock vmw_vmci vmmon(OE)
thunderbolt rfcomm cmac bnep binfmt_misc nls_iso8859_1 arc4 intel_rapl
x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_codec_hdmi
kvm_intel crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc
uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 aesni_intel
snd_hda_codec_realtek videobuf2_common snd_hda_codec_generic
aes_x86_64 crypto_simd cryptd glue_helper snd_seq_dummy videodev media
snd_seq_oss iwlmvm snd_hda_intel btusb intel_cstate intel_rapl_perf
snd_hda_codec btrtl mac80211 btbcm idma64 virt_dma btintel
snd_seq_midi snd_hda_core snd_seq_midi_event bluetooth snd_hwdep
snd_rawmidi ecdh_generic snd_seq iwlwifi snd_pcm snd_seq_device mei_me
thinkpad_acpi
[ 1432.735536]  joydev input_leds rtsx_pci_ms intel_lpss_pci serio_raw
cfg80211 memstick wmi_bmof intel_wmi_thunderbolt nvram mei snd_timer
intel_lpss intel_pch_thermal snd soundcore mac_hid acpi_pad
sch_fq_codel nfsd auth_rpcgss nfs_acl parport_pc lockd grace ppdev lp
sunrpc parport ip_tables x_tables autofs4 btrfs xor zstd_compress
raid6_pq libcrc32c wacom hid_generic usbhid mmc_block i915 nouveau
kvmgt vfio_mdev mdev vfio_iommu_type1 vfio rtsx_pci_sdmmc kvm mxm_wmi
irqbypass ttm i2c_algo_bit drm_kms_helper syscopyarea sysfillrect
sysimgblt fb_sys_fops psmouse e1000e drm rtsx_pci wmi i2c_hid
pinctrl_sunrisepoint hid video pinctrl_intel [last unloaded: cifs]
[ 1432.735588] ---[ end trace 8a48b6964e24a1b1 ]---
[ 1432.735593] RIP: 0010:usercopy_abort+0x7a/0x7c
[ 1432.735595] Code: 0f 45 c6 51 48 89 f9 48 c7 c2 45 9d 50 8e 41 52
48 c7 c6 29 8a 4f 8e 48 c7 c7 10 9e 50 8e 48 0f 45 f2 48 89 c2 e8 0f
03 e6 ff <0f> 0b 4d 89 e0 31 c9 44 89 ea 31 f6 48 c7 c7 79 9d 50 8e e8
6e ff
[ 1432.735597] RSP: 0018:ffffa8df102bfe30 EFLAGS: 00010246
[ 1432.735600] RAX: 0000000000000064 RBX: ffff9dadcddccea0 RCX: 0000000000000000
[ 1432.735602] RDX: 0000000000000000 RSI: ffff9daddf796428 RDI: ffff9daddf796428
[ 1432.735604] RBP: ffffa8df102bfe48 R08: 00000000000b388d R09: 00000000000004ef
[ 1432.735606] R10: 0000000000000004 R11: ffffffff8ed933ed R12: 000000000000001e
[ 1432.735608] R13: 0000000000000001 R14: ffff9dadcddccebe R15: ffff9dad35a62240
[ 1432.735611] FS:  00007fbc0779b740(0000) GS:ffff9daddf780000(0000)
knlGS:0000000000000000
[ 1432.735613] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1432.735615] CR2: 000055d070dd9008 CR3: 000000072ca1e005 CR4: 00000000003606e0
[ 1432.735617] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1432.735619] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
On Wed, Oct 10, 2018 at 12:29 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> When processing the mids for compounds we would only add credits based on
> the last successful mid in the compound which would leak credits and
> eventually triggering a re-connect.
>
> Fix this by splitting the mid processing part into two loops instead of one
> where the first loop just waits for all mids and then counts how many
> credits we were granted for the whole compound.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 57 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index b48f43963da6..333729cf46cd 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -786,7 +786,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>         int i, j, rc = 0;
>         int timeout, optype;
>         struct mid_q_entry *midQ[MAX_COMPOUND];
> -       unsigned int credits = 1;
> +       unsigned int credits = 0;
>         char *buf;
>
>         timeout = flags & CIFS_TIMEOUT_MASK;
> @@ -851,17 +851,20 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>
>         mutex_unlock(&ses->server->srv_mutex);
>
> -       for (i = 0; i < num_rqst; i++) {
> -               if (rc < 0)
> -                       goto out;
> +       if (rc < 0)
> +               goto out;
>
> -               if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP))
> -                       smb311_update_preauth_hash(ses, rqst[i].rq_iov,
> -                                                  rqst[i].rq_nvec);
> +       /*
> +        * Compounding is never used during session establish.
> +        */
> +       if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP))
> +               smb311_update_preauth_hash(ses, rqst[0].rq_iov,
> +                                          rqst[0].rq_nvec);
>
> -               if (timeout == CIFS_ASYNC_OP)
> -                       goto out;
> +       if (timeout == CIFS_ASYNC_OP)
> +               goto out;
>
> +       for (i = 0; i < num_rqst; i++) {
>                 rc = wait_for_response(ses->server, midQ[i]);
>                 if (rc != 0) {
>                         cifs_dbg(FYI, "Cancelling wait for mid %llu\n",
> @@ -877,10 +880,21 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                         }
>                         spin_unlock(&GlobalMid_Lock);
>                 }
> +       }
> +
> +       for (i = 0; i < num_rqst; i++)
> +               if (midQ[i]->resp_buf)
> +                       credits += ses->server->ops->get_credits(midQ[i]);
> +       if (!credits)
> +               credits = 1;
> +
> +       for (i = 0; i < num_rqst; i++) {
> +               if (rc < 0)
> +                       goto out;
>
>                 rc = cifs_sync_mid_result(midQ[i], ses->server);
>                 if (rc != 0) {
> -                       add_credits(ses->server, 1, optype);
> +                       add_credits(ses->server, credits, optype);
>                         return rc;
>                 }
>
> @@ -901,23 +915,26 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                 else
>                         resp_buf_type[i] = CIFS_SMALL_BUFFER;
>
> -               if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP)) {
> -                       struct kvec iov = {
> -                               .iov_base = resp_iov[i].iov_base,
> -                               .iov_len = resp_iov[i].iov_len
> -                       };
> -                       smb311_update_preauth_hash(ses, &iov, 1);
> -               }
> -
> -               credits = ses->server->ops->get_credits(midQ[i]);
> -
>                 rc = ses->server->ops->check_receive(midQ[i], ses->server,
>                                                      flags & CIFS_LOG_ERROR);
>
>                 /* mark it so buf will not be freed by cifs_delete_mid */
>                 if ((flags & CIFS_NO_RESP) == 0)
>                         midQ[i]->resp_buf = NULL;
> +
>         }
> +
> +       /*
> +        * Compounding is never used during session establish.
> +        */
> +       if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP)) {
> +               struct kvec iov = {
> +                       .iov_base = resp_iov[0].iov_base,
> +                       .iov_len = resp_iov[0].iov_len
> +               };
> +               smb311_update_preauth_hash(ses, &iov, 1);
> +       }
> +
>  out:
>         /*
>          * This will dequeue all mids. After this it is important that the
> --
> 2.13.6
>
diff mbox series

Patch

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index b48f43963da6..333729cf46cd 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -786,7 +786,7 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	int i, j, rc = 0;
 	int timeout, optype;
 	struct mid_q_entry *midQ[MAX_COMPOUND];
-	unsigned int credits = 1;
+	unsigned int credits = 0;
 	char *buf;
 
 	timeout = flags & CIFS_TIMEOUT_MASK;
@@ -851,17 +851,20 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 
 	mutex_unlock(&ses->server->srv_mutex);
 
-	for (i = 0; i < num_rqst; i++) {
-		if (rc < 0)
-			goto out;
+	if (rc < 0)
+		goto out;
 
-		if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP))
-			smb311_update_preauth_hash(ses, rqst[i].rq_iov,
-						   rqst[i].rq_nvec);
+	/*
+	 * Compounding is never used during session establish.
+	 */
+	if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP))
+		smb311_update_preauth_hash(ses, rqst[0].rq_iov,
+					   rqst[0].rq_nvec);
 
-		if (timeout == CIFS_ASYNC_OP)
-			goto out;
+	if (timeout == CIFS_ASYNC_OP)
+		goto out;
 
+	for (i = 0; i < num_rqst; i++) {
 		rc = wait_for_response(ses->server, midQ[i]);
 		if (rc != 0) {
 			cifs_dbg(FYI, "Cancelling wait for mid %llu\n",
@@ -877,10 +880,21 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 			}
 			spin_unlock(&GlobalMid_Lock);
 		}
+	}
+
+	for (i = 0; i < num_rqst; i++)
+		if (midQ[i]->resp_buf)
+			credits += ses->server->ops->get_credits(midQ[i]);
+	if (!credits)
+		credits = 1;
+
+	for (i = 0; i < num_rqst; i++) {
+		if (rc < 0)
+			goto out;
 
 		rc = cifs_sync_mid_result(midQ[i], ses->server);
 		if (rc != 0) {
-			add_credits(ses->server, 1, optype);
+			add_credits(ses->server, credits, optype);
 			return rc;
 		}
 
@@ -901,23 +915,26 @@  compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		else
 			resp_buf_type[i] = CIFS_SMALL_BUFFER;
 
-		if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP)) {
-			struct kvec iov = {
-				.iov_base = resp_iov[i].iov_base,
-				.iov_len = resp_iov[i].iov_len
-			};
-			smb311_update_preauth_hash(ses, &iov, 1);
-		}
-
-		credits = ses->server->ops->get_credits(midQ[i]);
-
 		rc = ses->server->ops->check_receive(midQ[i], ses->server,
 						     flags & CIFS_LOG_ERROR);
 
 		/* mark it so buf will not be freed by cifs_delete_mid */
 		if ((flags & CIFS_NO_RESP) == 0)
 			midQ[i]->resp_buf = NULL;
+
 	}
+
+	/*
+	 * Compounding is never used during session establish.
+	 */
+	if ((ses->status == CifsNew) || (optype & CIFS_NEG_OP)) {
+		struct kvec iov = {
+			.iov_base = resp_iov[0].iov_base,
+			.iov_len = resp_iov[0].iov_len
+		};
+		smb311_update_preauth_hash(ses, &iov, 1);
+	}
+
 out:
 	/*
 	 * This will dequeue all mids. After this it is important that the