* [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks
@ 2019-04-26 0:32 Ronnie Sahlberg
2019-04-26 0:32 ` [PATCH] " Ronnie Sahlberg
2019-04-26 16:35 ` [PATCH 0/1] " Pavel Shilovsky
0 siblings, 2 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2019-04-26 0:32 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky
Pavel, List
Here is an update after the email discussions.
This is minimal patch that adds a new flag to specifically indicate
that there will not be a response to this PDU and thus we can immediately
add the credits back after sending in compound_send_recv()
This flag is only set for SMB1 oplock breaks so this will not affect anything
else.
Additionally, we no longer need to check the CIFS_ASYNC_OP flag in
compound_send_recv() so we can remove that conditional.
This is the smallest patch which fixes the actual smb1 oplock bug thus we should
be able to get it into an rc, and stable.
But we need more but that should wait until the next merge window.
The next steps we should do is:
Remove CIFSSMBNotify() completely and with that also the check for
CIFS_ASYNC_OP in SendReceive()
When that is done, the only thing that CIFS_ASYNC_OP does is make sure
we do not block in wait_for_credits().
Which means we could rename this flag to CIFS_NON_BLOCKING.
We should also rename CIFS_NO_RSP to CIFS_NO_RSP_BUF to make it more clear that
there will be a response status code but there won't be any or we don't care about any response buffer.
Thus the semantics will become:
* cifs_call_async() : if you want async handling of responses
* CIFS_NO_SRV_RSP: There won't be a response so do not wait for a reply. (only used by SMB1 oplocks)
* CIFS_ASYNC_OP/CIFS_NON_BLOCKING: do not block waiting for available credits.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cifs: fix credits leak for SMB1 oplock breaks
2019-04-26 0:32 [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks Ronnie Sahlberg
@ 2019-04-26 0:32 ` Ronnie Sahlberg
2019-04-26 16:38 ` Pavel Shilovsky
2019-04-26 16:35 ` [PATCH 0/1] " Pavel Shilovsky
1 sibling, 1 reply; 6+ messages in thread
From: Ronnie Sahlberg @ 2019-04-26 0:32 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 1 +
fs/cifs/cifssmb.c | 2 +-
fs/cifs/transport.c | 10 +++++-----
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 621640195713..7a65124f70f9 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1701,6 +1701,7 @@ static inline bool is_retryable_error(int error)
#define CIFS_HAS_CREDITS 0x0400 /* already has credits */
#define CIFS_TRANSFORM_REQ 0x0800 /* transform request before sending */
+#define CIFS_NO_SRV_RSP 0x1000 /* there is no server response */
/* Security Flags: indicate type of session setup needed */
#define CIFSSEC_MAY_SIGN 0x00001
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index f43747c062a7..6050851edcb8 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2540,7 +2540,7 @@ CIFSSMBLock(const unsigned int xid, struct cifs_tcon *tcon,
if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) {
/* no response expected */
- flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP;
+ flags = CIFS_NO_SRV_RSP | CIFS_ASYNC_OP | CIFS_OBREAK_OP;
pSMB->Timeout = 0;
} else if (waitFlag) {
flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c59c498f56a..5573e38b13f3 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1073,8 +1073,11 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
mutex_unlock(&ses->server->srv_mutex);
- if (rc < 0) {
- /* Sending failed for some reason - return credits back */
+ /*
+ * If sending failed for some reason or it is an oplock break that we
+ * will not receive a response to - return credits back
+ */
+ if (rc < 0 || (flags & CIFS_NO_SRV_RSP)) {
for (i = 0; i < num_rqst; i++)
add_credits(ses->server, &credits[i], optype);
goto out;
@@ -1095,9 +1098,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
smb311_update_preauth_hash(ses, rqst[0].rq_iov,
rqst[0].rq_nvec);
- if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
- goto out;
-
for (i = 0; i < num_rqst; i++) {
rc = wait_for_response(ses->server, midQ[i]);
if (rc != 0)
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks
2019-04-26 0:32 [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks Ronnie Sahlberg
2019-04-26 0:32 ` [PATCH] " Ronnie Sahlberg
@ 2019-04-26 16:35 ` Pavel Shilovsky
1 sibling, 0 replies; 6+ messages in thread
From: Pavel Shilovsky @ 2019-04-26 16:35 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 25 апр. 2019 г. в 18:42, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Pavel, List
>
> Here is an update after the email discussions.
> This is minimal patch that adds a new flag to specifically indicate
> that there will not be a response to this PDU and thus we can immediately
> add the credits back after sending in compound_send_recv()
> This flag is only set for SMB1 oplock breaks so this will not affect anything
> else.
>
> Additionally, we no longer need to check the CIFS_ASYNC_OP flag in
> compound_send_recv() so we can remove that conditional.
>
> This is the smallest patch which fixes the actual smb1 oplock bug thus we should
> be able to get it into an rc, and stable.
> But we need more but that should wait until the next merge window.
>
>
> The next steps we should do is:
> Remove CIFSSMBNotify() completely and with that also the check for
> CIFS_ASYNC_OP in SendReceive()
>
> When that is done, the only thing that CIFS_ASYNC_OP does is make sure
> we do not block in wait_for_credits().
> Which means we could rename this flag to CIFS_NON_BLOCKING.
>
> We should also rename CIFS_NO_RSP to CIFS_NO_RSP_BUF to make it more clear that
> there will be a response status code but there won't be any or we don't care about any response buffer.
>
>
> Thus the semantics will become:
> * cifs_call_async() : if you want async handling of responses
> * CIFS_NO_SRV_RSP: There won't be a response so do not wait for a reply. (only used by SMB1 oplocks)
> * CIFS_ASYNC_OP/CIFS_NON_BLOCKING: do not block waiting for available credits.
>
Agree, sounds like a good plan. Thanks for summarizing!
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks
2019-04-26 0:32 ` [PATCH] " Ronnie Sahlberg
@ 2019-04-26 16:38 ` Pavel Shilovsky
2019-04-27 8:08 ` ronnie sahlberg
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Shilovsky @ 2019-04-26 16:38 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
чт, 25 апр. 2019 г. в 18:43, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsglob.h | 1 +
> fs/cifs/cifssmb.c | 2 +-
> fs/cifs/transport.c | 10 +++++-----
> 3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 621640195713..7a65124f70f9 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1701,6 +1701,7 @@ static inline bool is_retryable_error(int error)
>
> #define CIFS_HAS_CREDITS 0x0400 /* already has credits */
> #define CIFS_TRANSFORM_REQ 0x0800 /* transform request before sending */
> +#define CIFS_NO_SRV_RSP 0x1000 /* there is no server response */
>
> /* Security Flags: indicate type of session setup needed */
> #define CIFSSEC_MAY_SIGN 0x00001
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index f43747c062a7..6050851edcb8 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2540,7 +2540,7 @@ CIFSSMBLock(const unsigned int xid, struct cifs_tcon *tcon,
>
> if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) {
> /* no response expected */
> - flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> + flags = CIFS_NO_SRV_RSP | CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> pSMB->Timeout = 0;
> } else if (waitFlag) {
> flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c59c498f56a..5573e38b13f3 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -1073,8 +1073,11 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>
> mutex_unlock(&ses->server->srv_mutex);
>
> - if (rc < 0) {
> - /* Sending failed for some reason - return credits back */
> + /*
> + * If sending failed for some reason or it is an oplock break that we
probably indicate that it is SMB1 oplock break?
> + * will not receive a response to - return credits back
> + */
> + if (rc < 0 || (flags & CIFS_NO_SRV_RSP)) {
> for (i = 0; i < num_rqst; i++)
> add_credits(ses->server, &credits[i], optype);
A no-op callback is still needed to be set here or above for
CIFS_NO_SRV_RSP requests. Otherwise we may end up adding credits
twice: here and in the demultiplex thread.
> goto out;
> @@ -1095,9 +1098,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> smb311_update_preauth_hash(ses, rqst[0].rq_iov,
> rqst[0].rq_nvec);
>
> - if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
> - goto out;
> -
> for (i = 0; i < num_rqst; i++) {
> rc = wait_for_response(ses->server, midQ[i]);
> if (rc != 0)
> --
> 2.13.6
>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks
2019-04-26 16:38 ` Pavel Shilovsky
@ 2019-04-27 8:08 ` ronnie sahlberg
2019-04-29 18:58 ` Pavel Shilovsky
0 siblings, 1 reply; 6+ messages in thread
From: ronnie sahlberg @ 2019-04-27 8:08 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Ronnie Sahlberg, linux-cifs, Steve French, Pavel Shilovsky
On Sat, Apr 27, 2019 at 2:38 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 25 апр. 2019 г. в 18:43, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> > fs/cifs/cifsglob.h | 1 +
> > fs/cifs/cifssmb.c | 2 +-
> > fs/cifs/transport.c | 10 +++++-----
> > 3 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 621640195713..7a65124f70f9 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1701,6 +1701,7 @@ static inline bool is_retryable_error(int error)
> >
> > #define CIFS_HAS_CREDITS 0x0400 /* already has credits */
> > #define CIFS_TRANSFORM_REQ 0x0800 /* transform request before sending */
> > +#define CIFS_NO_SRV_RSP 0x1000 /* there is no server response */
> >
> > /* Security Flags: indicate type of session setup needed */
> > #define CIFSSEC_MAY_SIGN 0x00001
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index f43747c062a7..6050851edcb8 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -2540,7 +2540,7 @@ CIFSSMBLock(const unsigned int xid, struct cifs_tcon *tcon,
> >
> > if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) {
> > /* no response expected */
> > - flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> > + flags = CIFS_NO_SRV_RSP | CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> > pSMB->Timeout = 0;
> > } else if (waitFlag) {
> > flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 5c59c498f56a..5573e38b13f3 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -1073,8 +1073,11 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >
> > mutex_unlock(&ses->server->srv_mutex);
> >
> > - if (rc < 0) {
> > - /* Sending failed for some reason - return credits back */
> > + /*
> > + * If sending failed for some reason or it is an oplock break that we
>
> probably indicate that it is SMB1 oplock break?
>
> > + * will not receive a response to - return credits back
> > + */
> > + if (rc < 0 || (flags & CIFS_NO_SRV_RSP)) {
> > for (i = 0; i < num_rqst; i++)
> > add_credits(ses->server, &credits[i], optype);
>
> A no-op callback is still needed to be set here or above for
> CIFS_NO_SRV_RSP requests. Otherwise we may end up adding credits
> twice: here and in the demultiplex thread.
I don't think we need one. Since there will never be a reply we will
never invoke the callback from the demultiplex thread.
>
> > goto out;
> > @@ -1095,9 +1098,6 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > smb311_update_preauth_hash(ses, rqst[0].rq_iov,
> > rqst[0].rq_nvec);
> >
> > - if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
> > - goto out;
> > -
> > for (i = 0; i < num_rqst; i++) {
> > rc = wait_for_response(ses->server, midQ[i]);
> > if (rc != 0)
> > --
> > 2.13.6
> >
>
>
> --
> Best regards,
> Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks
2019-04-27 8:08 ` ronnie sahlberg
@ 2019-04-29 18:58 ` Pavel Shilovsky
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Shilovsky @ 2019-04-29 18:58 UTC (permalink / raw)
To: ronnie sahlberg
Cc: Ronnie Sahlberg, linux-cifs, Steve French, Pavel Shilovsky
сб, 27 апр. 2019 г. в 01:08, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> On Sat, Apr 27, 2019 at 2:38 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > чт, 25 апр. 2019 г. в 18:43, Ronnie Sahlberg <lsahlber@redhat.com>:
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > > fs/cifs/cifsglob.h | 1 +
> > > fs/cifs/cifssmb.c | 2 +-
> > > fs/cifs/transport.c | 10 +++++-----
> > > 3 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 621640195713..7a65124f70f9 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -1701,6 +1701,7 @@ static inline bool is_retryable_error(int error)
> > >
> > > #define CIFS_HAS_CREDITS 0x0400 /* already has credits */
> > > #define CIFS_TRANSFORM_REQ 0x0800 /* transform request before sending */
> > > +#define CIFS_NO_SRV_RSP 0x1000 /* there is no server response */
> > >
> > > /* Security Flags: indicate type of session setup needed */
> > > #define CIFSSEC_MAY_SIGN 0x00001
> > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > > index f43747c062a7..6050851edcb8 100644
> > > --- a/fs/cifs/cifssmb.c
> > > +++ b/fs/cifs/cifssmb.c
> > > @@ -2540,7 +2540,7 @@ CIFSSMBLock(const unsigned int xid, struct cifs_tcon *tcon,
> > >
> > > if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) {
> > > /* no response expected */
> > > - flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> > > + flags = CIFS_NO_SRV_RSP | CIFS_ASYNC_OP | CIFS_OBREAK_OP;
> > > pSMB->Timeout = 0;
> > > } else if (waitFlag) {
> > > flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */
> > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > index 5c59c498f56a..5573e38b13f3 100644
> > > --- a/fs/cifs/transport.c
> > > +++ b/fs/cifs/transport.c
> > > @@ -1073,8 +1073,11 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > >
> > > mutex_unlock(&ses->server->srv_mutex);
> > >
> > > - if (rc < 0) {
> > > - /* Sending failed for some reason - return credits back */
> > > + /*
> > > + * If sending failed for some reason or it is an oplock break that we
> >
> > probably indicate that it is SMB1 oplock break?
> >
> > > + * will not receive a response to - return credits back
> > > + */
> > > + if (rc < 0 || (flags & CIFS_NO_SRV_RSP)) {
> > > for (i = 0; i < num_rqst; i++)
> > > add_credits(ses->server, &credits[i], optype);
> >
> > A no-op callback is still needed to be set here or above for
> > CIFS_NO_SRV_RSP requests. Otherwise we may end up adding credits
> > twice: here and in the demultiplex thread.
>
> I don't think we need one. Since there will never be a reply we will
> never invoke the callback from the demultiplex thread.
Ok, this is according to the spec. Having a no-op callback would save
us from the server error but it's probably not worth the attention.
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>.
I think this patch belongs to stable because the bug exists in 5.0.
Could you please add a description together with a stable tag and
resend?
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-29 18:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-26 0:32 [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks Ronnie Sahlberg
2019-04-26 0:32 ` [PATCH] " Ronnie Sahlberg
2019-04-26 16:38 ` Pavel Shilovsky
2019-04-27 8:08 ` ronnie sahlberg
2019-04-29 18:58 ` Pavel Shilovsky
2019-04-26 16:35 ` [PATCH 0/1] " Pavel Shilovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).