Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/1] cifs: fix race between compound_send_recv() and the demultiplex thread
@ 2019-11-14  1:42 Ronnie Sahlberg
  2019-11-14  1:42 ` [PATCH] " Ronnie Sahlberg
  0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-11-14  1:42 UTC (permalink / raw)
  To: linux-cifs

List, Pavel

Here is a small patch that fixes another race where we leak a handle on an interrupted open()
Please review.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-14  1:42 [PATCH 0/1] cifs: fix race between compound_send_recv() and the demultiplex thread Ronnie Sahlberg
@ 2019-11-14  1:42 ` " Ronnie Sahlberg
  0 siblings, 0 replies; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-11-14  1:42 UTC (permalink / raw)
  To: linux-cifs; +Cc: Ronnie Sahlberg

There is a race where the open() may be interrupted between when we receive the reply
but before we have invoked the callback in which case we never end up calling
handle_cancelled_mid() and thus leak an open handle on the server.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/connect.c   | 1 -
 fs/cifs/transport.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index ccaa8bad336f..802604a7e692 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
 			if (mids[i] != NULL) {
 				mids[i]->resp_buf_size = server->pdu_size;
 				if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
-				    mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
 				    server->ops->handle_cancelled_mid)
 					server->ops->handle_cancelled_mid(
 							mids[i]->resp_buf,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index ca3de62688d6..28018a7eccb2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1119,7 +1119,8 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 				 midQ[i]->mid, le16_to_cpu(midQ[i]->command));
 			send_cancel(server, &rqst[i], midQ[i]);
 			spin_lock(&GlobalMid_Lock);
-			if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
+			if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED ||
+			    midQ[i]->mid_state == MID_RESPONSE_RECEIVED) {
 				midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
 				midQ[i]->callback = cifs_cancelled_callback;
 				cancelled_mid[i] = true;
-- 
2.13.6


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-21  0:09       ` Steve French
@ 2019-11-21 19:34         ` Pavel Shilovsky
  0 siblings, 0 replies; 7+ messages in thread
From: Pavel Shilovsky @ 2019-11-21 19:34 UTC (permalink / raw)
  To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs

I figured out the problem: there is another race between the
demultiplex thread and a system call thread which leads to
mid->resp_buf being NULL when it is being accessed to get credits.
This is unrelated to this patch but it probably just increases
probability of this to happen.

Diving deeply into the code, I think this patch doesn't solve the
problem completely - there is still a possibility that the demultiplex
thread finished processing of the mid by the time the mid is set as
cancelled. Also we can't set cancelled callback in the case when a
response has been already processed because the cancelled callback
won't be called and the mid will be leaked.

I created a patch to fix the race another way: by moving handling of
cancelled mids into the mid destructor. By that time there should be
only one thread referencing the mid, so, a race should occur there. I
will post the patch to the list together with other patches.

--
Best regards,
Pavel Shilovsky

ср, 20 нояб. 2019 г. в 16:10, Steve French <smfrench@gmail.com>:
>
> temporarily removed this patch from cifs-2.6.git for-next to give time
> to respin the patch
>
> On Wed, Nov 20, 2019 at 5:57 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > пт, 15 нояб. 2019 г. в 10:07, Pavel Shilovsky <piastryyy@gmail.com>:
> > >
> > > ср, 13 нояб. 2019 г. в 22:17, Ronnie Sahlberg <lsahlber@redhat.com>:
> > > >
> > > > There is a race where the open() may be interrupted between when we receive the reply
> > > > but before we have invoked the callback in which case we never end up calling
> > > > handle_cancelled_mid() and thus leak an open handle on the server.
> > > >
> > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > ---
> > > >  fs/cifs/connect.c   | 1 -
> > > >  fs/cifs/transport.c | 2 +-
> > > >  2 files changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > > index ccaa8bad336f..802604a7e692 100644
> > > > --- a/fs/cifs/connect.c
> > > > +++ b/fs/cifs/connect.c
> > > > @@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
> > > >                         if (mids[i] != NULL) {
> > > >                                 mids[i]->resp_buf_size = server->pdu_size;
> > > >                                 if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
> > > > -                                   mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
> > > >                                     server->ops->handle_cancelled_mid)
> > > >                                         server->ops->handle_cancelled_mid(
> > > >                                                         mids[i]->resp_buf,
> > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > > index ca3de62688d6..0f219f7653f3 100644
> > > > --- a/fs/cifs/transport.c
> > > > +++ b/fs/cifs/transport.c
> > > > @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > > >                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
> > > >                         send_cancel(server, &rqst[i], midQ[i]);
> > > >                         spin_lock(&GlobalMid_Lock);
> > > > -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> > > > +                       if (is_interrupt_error(rc)) {
> > > >                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
> > > >                                 midQ[i]->callback = cifs_cancelled_callback;
> > > >                                 cancelled_mid[i] = true;
> > > > --
> > > > 2.13.6
> > > >
> > >
> > > It doesn't seem that RC may be anything other than -ERESTARTSYS but
> > > is_interrupt_error() should work.
> > >
> > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> >
> > Tested it out. The following part of this change
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index ca3de62688d6..0f219f7653f3 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid,
> > struct cifs_ses *ses,
> >                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
> >                         send_cancel(server, &rqst[i], midQ[i]);
> >                         spin_lock(&GlobalMid_Lock);
> > -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> > +                       if (is_interrupt_error(rc)) {
> >                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
> >                                 midQ[i]->callback = cifs_cancelled_callback;
> >                                 cancelled_mid[i] = true;
> >
> > is causing NULL-pointer dereference on my system:
> >
> > [681000.970523] BUG: kernel NULL pointer dereference, address: 000000000000000e
> > [681000.970526] #PF: supervisor read access in kernel mode
> > [681000.970527] #PF: error_code(0x0000) - not-present page
> > [681000.970528] PGD 0 P4D 0
> > [681000.970531] Oops: 0000 [#1] SMP PTI
> > [681000.970533] CPU: 3 PID: 15946 Comm: cifsd Tainted: G           OE
> >    5.3.7-050307-generic #201910180652
> > [681000.970534] Hardware name: Microsoft Corporation Virtual
> > Machine/Virtual Machine, BIOS 090008  12/07/2018
> > [681000.970554] RIP: 0010:smb2_get_credits+0x1f/0x30 [cifs]
> > [681000.970556] Code: 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
> > 8b 57 6c 55 48 89 e5 83 fa 04 74 09 31 c0 83 fa 10 74 02 5d c3 48 8b
> > 47 60 5d <0f> b7 40 0e c3 66 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44
> > 00 00
> > [681000.970558] RSP: 0018:ffffa9d680433df0 EFLAGS: 00010246
> > [681000.970559] RAX: 0000000000000000 RBX: ffff9a0ef1775000 RCX:
> > 0000000000000000
> > [681000.970560] RDX: 0000000000000004 RSI: 0000000000000000 RDI:
> > ffff9a0ef15c0780
> > [681000.970561] RBP: ffffa9d680433e18 R08: 0000000000000218 R09:
> > 00000000001bfc3a
> > [681000.970562] R10: 00000000000dfe1d R11: ffff9a0ef1b7cae0 R12:
> > ffff9a0ef15c0780
> > [681000.970563] R13: ffffa9d680433e80 R14: ffffa9d680433ea8 R15:
> > 0000000000000001
> > [681000.970565] FS:  0000000000000000(0000) GS:ffff9a0ef7b80000(0000)
> > knlGS:0000000000000000
> > [681000.970566] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [681000.970566] CR2: 000000000000000e CR3: 00000000e7e0a001 CR4:
> > 00000000003606e0
> > [681000.970569] Call Trace:
> > [681000.970585]  ? cifs_compound_callback+0x33/0x80 [cifs]
> > [681000.970599]  cifs_compound_last_callback+0x12/0x20 [cifs]
> > [681000.970611]  cifs_demultiplex_thread+0x6d4/0xc40 [cifs]
> > [681000.970615]  kthread+0x104/0x140
> > [681000.970627]  ? cifs_handle_standard+0x190/0x190 [cifs]
> > [681000.970629]  ? kthread_park+0x80/0x80
> > [681000.970631]  ret_from_fork+0x35/0x40
> >
> > Still haven't figured out why this is happening. Looking.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
>
>
>
> --
> Thanks,
>
> Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-20 23:57     ` Pavel Shilovsky
@ 2019-11-21  0:09       ` Steve French
  2019-11-21 19:34         ` Pavel Shilovsky
  0 siblings, 1 reply; 7+ messages in thread
From: Steve French @ 2019-11-21  0:09 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: Ronnie Sahlberg, linux-cifs

temporarily removed this patch from cifs-2.6.git for-next to give time
to respin the patch

On Wed, Nov 20, 2019 at 5:57 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> пт, 15 нояб. 2019 г. в 10:07, Pavel Shilovsky <piastryyy@gmail.com>:
> >
> > ср, 13 нояб. 2019 г. в 22:17, Ronnie Sahlberg <lsahlber@redhat.com>:
> > >
> > > There is a race where the open() may be interrupted between when we receive the reply
> > > but before we have invoked the callback in which case we never end up calling
> > > handle_cancelled_mid() and thus leak an open handle on the server.
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/connect.c   | 1 -
> > >  fs/cifs/transport.c | 2 +-
> > >  2 files changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > index ccaa8bad336f..802604a7e692 100644
> > > --- a/fs/cifs/connect.c
> > > +++ b/fs/cifs/connect.c
> > > @@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
> > >                         if (mids[i] != NULL) {
> > >                                 mids[i]->resp_buf_size = server->pdu_size;
> > >                                 if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
> > > -                                   mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
> > >                                     server->ops->handle_cancelled_mid)
> > >                                         server->ops->handle_cancelled_mid(
> > >                                                         mids[i]->resp_buf,
> > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > index ca3de62688d6..0f219f7653f3 100644
> > > --- a/fs/cifs/transport.c
> > > +++ b/fs/cifs/transport.c
> > > @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > >                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
> > >                         send_cancel(server, &rqst[i], midQ[i]);
> > >                         spin_lock(&GlobalMid_Lock);
> > > -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> > > +                       if (is_interrupt_error(rc)) {
> > >                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
> > >                                 midQ[i]->callback = cifs_cancelled_callback;
> > >                                 cancelled_mid[i] = true;
> > > --
> > > 2.13.6
> > >
> >
> > It doesn't seem that RC may be anything other than -ERESTARTSYS but
> > is_interrupt_error() should work.
> >
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > --
> > Best regards,
> > Pavel Shilovsky
>
> Tested it out. The following part of this change
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index ca3de62688d6..0f219f7653f3 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid,
> struct cifs_ses *ses,
>                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
>                         send_cancel(server, &rqst[i], midQ[i]);
>                         spin_lock(&GlobalMid_Lock);
> -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> +                       if (is_interrupt_error(rc)) {
>                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
>                                 midQ[i]->callback = cifs_cancelled_callback;
>                                 cancelled_mid[i] = true;
>
> is causing NULL-pointer dereference on my system:
>
> [681000.970523] BUG: kernel NULL pointer dereference, address: 000000000000000e
> [681000.970526] #PF: supervisor read access in kernel mode
> [681000.970527] #PF: error_code(0x0000) - not-present page
> [681000.970528] PGD 0 P4D 0
> [681000.970531] Oops: 0000 [#1] SMP PTI
> [681000.970533] CPU: 3 PID: 15946 Comm: cifsd Tainted: G           OE
>    5.3.7-050307-generic #201910180652
> [681000.970534] Hardware name: Microsoft Corporation Virtual
> Machine/Virtual Machine, BIOS 090008  12/07/2018
> [681000.970554] RIP: 0010:smb2_get_credits+0x1f/0x30 [cifs]
> [681000.970556] Code: 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
> 8b 57 6c 55 48 89 e5 83 fa 04 74 09 31 c0 83 fa 10 74 02 5d c3 48 8b
> 47 60 5d <0f> b7 40 0e c3 66 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44
> 00 00
> [681000.970558] RSP: 0018:ffffa9d680433df0 EFLAGS: 00010246
> [681000.970559] RAX: 0000000000000000 RBX: ffff9a0ef1775000 RCX:
> 0000000000000000
> [681000.970560] RDX: 0000000000000004 RSI: 0000000000000000 RDI:
> ffff9a0ef15c0780
> [681000.970561] RBP: ffffa9d680433e18 R08: 0000000000000218 R09:
> 00000000001bfc3a
> [681000.970562] R10: 00000000000dfe1d R11: ffff9a0ef1b7cae0 R12:
> ffff9a0ef15c0780
> [681000.970563] R13: ffffa9d680433e80 R14: ffffa9d680433ea8 R15:
> 0000000000000001
> [681000.970565] FS:  0000000000000000(0000) GS:ffff9a0ef7b80000(0000)
> knlGS:0000000000000000
> [681000.970566] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [681000.970566] CR2: 000000000000000e CR3: 00000000e7e0a001 CR4:
> 00000000003606e0
> [681000.970569] Call Trace:
> [681000.970585]  ? cifs_compound_callback+0x33/0x80 [cifs]
> [681000.970599]  cifs_compound_last_callback+0x12/0x20 [cifs]
> [681000.970611]  cifs_demultiplex_thread+0x6d4/0xc40 [cifs]
> [681000.970615]  kthread+0x104/0x140
> [681000.970627]  ? cifs_handle_standard+0x190/0x190 [cifs]
> [681000.970629]  ? kthread_park+0x80/0x80
> [681000.970631]  ret_from_fork+0x35/0x40
>
> Still haven't figured out why this is happening. Looking.
>
> --
> Best regards,
> Pavel Shilovsky



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-15 18:07   ` Pavel Shilovsky
@ 2019-11-20 23:57     ` Pavel Shilovsky
  2019-11-21  0:09       ` Steve French
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Shilovsky @ 2019-11-20 23:57 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French

пт, 15 нояб. 2019 г. в 10:07, Pavel Shilovsky <piastryyy@gmail.com>:
>
> ср, 13 нояб. 2019 г. в 22:17, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > There is a race where the open() may be interrupted between when we receive the reply
> > but before we have invoked the callback in which case we never end up calling
> > handle_cancelled_mid() and thus leak an open handle on the server.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/connect.c   | 1 -
> >  fs/cifs/transport.c | 2 +-
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index ccaa8bad336f..802604a7e692 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
> >                         if (mids[i] != NULL) {
> >                                 mids[i]->resp_buf_size = server->pdu_size;
> >                                 if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
> > -                                   mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
> >                                     server->ops->handle_cancelled_mid)
> >                                         server->ops->handle_cancelled_mid(
> >                                                         mids[i]->resp_buf,
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index ca3de62688d6..0f219f7653f3 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
> >                         send_cancel(server, &rqst[i], midQ[i]);
> >                         spin_lock(&GlobalMid_Lock);
> > -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> > +                       if (is_interrupt_error(rc)) {
> >                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
> >                                 midQ[i]->callback = cifs_cancelled_callback;
> >                                 cancelled_mid[i] = true;
> > --
> > 2.13.6
> >
>
> It doesn't seem that RC may be anything other than -ERESTARTSYS but
> is_interrupt_error() should work.
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky

Tested it out. The following part of this change

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index ca3de62688d6..0f219f7653f3 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid,
struct cifs_ses *ses,
                                 midQ[i]->mid, le16_to_cpu(midQ[i]->command));
                        send_cancel(server, &rqst[i], midQ[i]);
                        spin_lock(&GlobalMid_Lock);
-                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
+                       if (is_interrupt_error(rc)) {
                                midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
                                midQ[i]->callback = cifs_cancelled_callback;
                                cancelled_mid[i] = true;

is causing NULL-pointer dereference on my system:

[681000.970523] BUG: kernel NULL pointer dereference, address: 000000000000000e
[681000.970526] #PF: supervisor read access in kernel mode
[681000.970527] #PF: error_code(0x0000) - not-present page
[681000.970528] PGD 0 P4D 0
[681000.970531] Oops: 0000 [#1] SMP PTI
[681000.970533] CPU: 3 PID: 15946 Comm: cifsd Tainted: G           OE
   5.3.7-050307-generic #201910180652
[681000.970534] Hardware name: Microsoft Corporation Virtual
Machine/Virtual Machine, BIOS 090008  12/07/2018
[681000.970554] RIP: 0010:smb2_get_credits+0x1f/0x30 [cifs]
[681000.970556] Code: 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
8b 57 6c 55 48 89 e5 83 fa 04 74 09 31 c0 83 fa 10 74 02 5d c3 48 8b
47 60 5d <0f> b7 40 0e c3 66 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44
00 00
[681000.970558] RSP: 0018:ffffa9d680433df0 EFLAGS: 00010246
[681000.970559] RAX: 0000000000000000 RBX: ffff9a0ef1775000 RCX:
0000000000000000
[681000.970560] RDX: 0000000000000004 RSI: 0000000000000000 RDI:
ffff9a0ef15c0780
[681000.970561] RBP: ffffa9d680433e18 R08: 0000000000000218 R09:
00000000001bfc3a
[681000.970562] R10: 00000000000dfe1d R11: ffff9a0ef1b7cae0 R12:
ffff9a0ef15c0780
[681000.970563] R13: ffffa9d680433e80 R14: ffffa9d680433ea8 R15:
0000000000000001
[681000.970565] FS:  0000000000000000(0000) GS:ffff9a0ef7b80000(0000)
knlGS:0000000000000000
[681000.970566] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[681000.970566] CR2: 000000000000000e CR3: 00000000e7e0a001 CR4:
00000000003606e0
[681000.970569] Call Trace:
[681000.970585]  ? cifs_compound_callback+0x33/0x80 [cifs]
[681000.970599]  cifs_compound_last_callback+0x12/0x20 [cifs]
[681000.970611]  cifs_demultiplex_thread+0x6d4/0xc40 [cifs]
[681000.970615]  kthread+0x104/0x140
[681000.970627]  ? cifs_handle_standard+0x190/0x190 [cifs]
[681000.970629]  ? kthread_park+0x80/0x80
[681000.970631]  ret_from_fork+0x35/0x40

Still haven't figured out why this is happening. Looking.

--
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-14  6:16 ` [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread Ronnie Sahlberg
@ 2019-11-15 18:07   ` Pavel Shilovsky
  2019-11-20 23:57     ` Pavel Shilovsky
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Shilovsky @ 2019-11-15 18:07 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: linux-cifs

ср, 13 нояб. 2019 г. в 22:17, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> There is a race where the open() may be interrupted between when we receive the reply
> but before we have invoked the callback in which case we never end up calling
> handle_cancelled_mid() and thus leak an open handle on the server.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/connect.c   | 1 -
>  fs/cifs/transport.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index ccaa8bad336f..802604a7e692 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
>                         if (mids[i] != NULL) {
>                                 mids[i]->resp_buf_size = server->pdu_size;
>                                 if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
> -                                   mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
>                                     server->ops->handle_cancelled_mid)
>                                         server->ops->handle_cancelled_mid(
>                                                         mids[i]->resp_buf,
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index ca3de62688d6..0f219f7653f3 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
>                         send_cancel(server, &rqst[i], midQ[i]);
>                         spin_lock(&GlobalMid_Lock);
> -                       if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> +                       if (is_interrupt_error(rc)) {
>                                 midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
>                                 midQ[i]->callback = cifs_cancelled_callback;
>                                 cancelled_mid[i] = true;
> --
> 2.13.6
>

It doesn't seem that RC may be anything other than -ERESTARTSYS but
is_interrupt_error() should work.

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread
  2019-11-14  6:16 [PATCH 0/1] cifs: fix race between compound_send_recv() and the Ronnie Sahlberg
@ 2019-11-14  6:16 ` Ronnie Sahlberg
  2019-11-15 18:07   ` Pavel Shilovsky
  0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-11-14  6:16 UTC (permalink / raw)
  To: linux-cifs; +Cc: Ronnie Sahlberg

There is a race where the open() may be interrupted between when we receive the reply
but before we have invoked the callback in which case we never end up calling
handle_cancelled_mid() and thus leak an open handle on the server.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/connect.c   | 1 -
 fs/cifs/transport.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index ccaa8bad336f..802604a7e692 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1223,7 +1223,6 @@ cifs_demultiplex_thread(void *p)
 			if (mids[i] != NULL) {
 				mids[i]->resp_buf_size = server->pdu_size;
 				if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
-				    mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
 				    server->ops->handle_cancelled_mid)
 					server->ops->handle_cancelled_mid(
 							mids[i]->resp_buf,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index ca3de62688d6..0f219f7653f3 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1119,7 +1119,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 				 midQ[i]->mid, le16_to_cpu(midQ[i]->command));
 			send_cancel(server, &rqst[i], midQ[i]);
 			spin_lock(&GlobalMid_Lock);
-			if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
+			if (is_interrupt_error(rc)) {
 				midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
 				midQ[i]->callback = cifs_cancelled_callback;
 				cancelled_mid[i] = true;
-- 
2.13.6


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-14  1:42 [PATCH 0/1] cifs: fix race between compound_send_recv() and the demultiplex thread Ronnie Sahlberg
2019-11-14  1:42 ` [PATCH] " Ronnie Sahlberg
2019-11-14  6:16 [PATCH 0/1] cifs: fix race between compound_send_recv() and the Ronnie Sahlberg
2019-11-14  6:16 ` [PATCH] cifs: fix race between compound_send_recv() and the demultiplex thread Ronnie Sahlberg
2019-11-15 18:07   ` Pavel Shilovsky
2019-11-20 23:57     ` Pavel Shilovsky
2019-11-21  0:09       ` Steve French
2019-11-21 19:34         ` Pavel Shilovsky

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git