| Message ID | CAH2r5mtUFEwpef+W9bY-bRyZKN4EJ+WjNxkmmQJo57Z8O_3vbA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb3: fix lease break problem introduced by compounding | expand |
пт, 28 сент. 2018 г. в 0:35, Steve French <smfrench@gmail.com>: > > Fixes problem (discovered by Aurelien) introduced by recent commit: > commit b24df3e30cbf48255db866720fb71f14bf9d2f39 > ("cifs: update receive_encrypted_standard to handle compounded responses") > > which broke the ability to respond to some lease breaks > (lease breaks being ignored is a problem since can block > server response for duration of the lease break timeout). > > Signed-off-by: Steve French <stfrench@microsoft.com> > --- > fs/cifs/connect.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index f765b20985cd..4307d635bc5d 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -969,6 +969,9 @@ cifs_demultiplex_thread(void *p) > > server->lstrp = jiffies; > > + if ((num_mids == 0) && (server->ops->is_oplock_break)) > + server->ops->is_oplock_break(bufs[0], server); > + > for (i = 0; i < num_mids; i++) { > if (mids[i] != NULL) { > mids[i]->resp_buf_size = server->pdu_size; > -- > 2.17.1 > > > -- > Thanks, > > Steve adding Ronnie... The problem here is that we don't increment num_mid for non-enctypted connections for lease/oplock breaks (no waiting MID). This should not affect encrypted connections - we always increment num_mid there event if MID is not found in the list of waiting MIDs. This itself looks correct but I think there is a cleaner way: if (server->ops->is_transform_hdr && server->ops->receive_transform && server->ops->is_transform_hdr(buf)) { length = server->ops->receive_transform(server, mids, bufs, &num_mids); <--- here we always get a correct num_mid } else { mids[0] = server->ops->find_mid(server, buf); bufs[0] = buf; if (mids[0]) <--- this check is not needed and we should set num_mids to 1 here, so the for loop below will handle it right num_mids = 1; if (!mids[0] || !mids[0]->receive) length = standard_receive3(server, mids[0]); else length = mids[0]->receive(server, mids[0]); } ..... server->lstrp = jiffies; for (i = 0; i < num_mids; i++) { if (mids[i] != NULL) { .... } else if (server->ops->is_oplock_break && server->ops->is_oplock_break(bufs[i], server)) { <--- if we eliminate the check above this if block will process lease/oplock breaks cifs_dbg(FYI, "Received oplock break\n"); ..... } So, I would prefer to remove the check mentioned above. Thoughts? -- Best regards, Pavel Shilovsky
I Think Pavel is right. Thanks for this. I base this on reading the code and his comment. My work laptop is still busted so I can not run proper tests until I fix it proper. On Sat, Sep 29, 2018 at 8:22 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > пт, 28 сент. 2018 г. в 0:35, Steve French <smfrench@gmail.com>: > > > > Fixes problem (discovered by Aurelien) introduced by recent commit: > > commit b24df3e30cbf48255db866720fb71f14bf9d2f39 > > ("cifs: update receive_encrypted_standard to handle compounded responses") > > > > which broke the ability to respond to some lease breaks > > (lease breaks being ignored is a problem since can block > > server response for duration of the lease break timeout). > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > --- > > fs/cifs/connect.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index f765b20985cd..4307d635bc5d 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -969,6 +969,9 @@ cifs_demultiplex_thread(void *p) > > > > server->lstrp = jiffies; > > > > + if ((num_mids == 0) && (server->ops->is_oplock_break)) > > + server->ops->is_oplock_break(bufs[0], server); > > + > > for (i = 0; i < num_mids; i++) { > > if (mids[i] != NULL) { > > mids[i]->resp_buf_size = server->pdu_size; > > -- > > 2.17.1 > > > > > > -- > > Thanks, > > > > Steve > > adding Ronnie... > > The problem here is that we don't increment num_mid for non-enctypted > connections for lease/oplock breaks (no waiting MID). This should not > affect encrypted connections - we always increment num_mid there > event if MID is not found in the list of waiting MIDs. > > This itself looks correct but I think there is a cleaner way: > > if (server->ops->is_transform_hdr && > server->ops->receive_transform && > server->ops->is_transform_hdr(buf)) { > length = server->ops->receive_transform(server, mids, bufs, > &num_mids); <--- here we always get a correct num_mid > } else { > mids[0] = server->ops->find_mid(server, buf); > bufs[0] = buf; > if (mids[0]) <--- this check is not needed and we should set num_mids > to 1 here, so the for loop below will handle it right > num_mids = 1; > > if (!mids[0] || !mids[0]->receive) > length = standard_receive3(server, mids[0]); > else > length = mids[0]->receive(server, mids[0]); > } > > ..... > > server->lstrp = jiffies; > > for (i = 0; i < num_mids; i++) { > if (mids[i] != NULL) { > > .... > > } else if (server->ops->is_oplock_break && > server->ops->is_oplock_break(bufs[i], server)) { <--- if we eliminate > the check above this if block will process lease/oplock breaks > cifs_dbg(FYI, "Received oplock break\n"); > ..... > } > > So, I would prefer to remove the check mentioned above. Thoughts? > > -- > Best regards, > Pavel Shilovsky
From 74b3197a7431f6f9bbbbbdc193d882d3564f7e72 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 28 Sep 2018 02:11:43 -0500
Subject: [PATCH] smb3: fix lease break problem introducted by compounding
Fixes problem introduced by recent commit:
commit b24df3e30cbf48255db866720fb71f14bf9d2f39
("cifs: update receive_encrypted_standard to handle compounded responses")
which broke the ability to respond to some lease breaks
(lease breaks being ignored is a problem since can block
server response for duration of the lease break timeout).
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f765b20985cd..4307d635bc5d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -969,6 +969,9 @@ cifs_demultiplex_thread(void *p)
server->lstrp = jiffies;
+ if ((num_mids == 0) && (server->ops->is_oplock_break))
+ server->ops->is_oplock_break(bufs[0], server);
+
for (i = 0; i < num_mids; i++) {
if (mids[i] != NULL) {
mids[i]->resp_buf_size = server->pdu_size;
--
2.17.1
Fixes problem (discovered by Aurelien) introduced by recent commit: commit b24df3e30cbf48255db866720fb71f14bf9d2f39 ("cifs: update receive_encrypted_standard to handle compounded responses") which broke the ability to respond to some lease breaks (lease breaks being ignored is a problem since can block server response for duration of the lease break timeout). Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/connect.c | 3 +++ 1 file changed, 3 insertions(+)