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@xxxxxxxxx>:
>
> 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@xxxxxxxxx> wrote:
> >
> > пт, 15 нояб. 2019 г. в 10:07, Pavel Shilovsky <piastryyy@xxxxxxxxx>:
> > >
> > > ср, 13 нояб. 2019 г. в 22:17, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxx>
> > >
> > > --
> > > 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
