[PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks

[PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks

[Ronnie Sahlberg]

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.

[PATCH] cifs: fix credits leak for SMB1 oplock breaks

[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

Re: [PATCH 0/1] cifs: fix credits leak for SMB1 oplock breaks

[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

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[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

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[ronnie sahlberg]

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

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[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

[PATCH] cifs: fix credits leak for SMB1 oplock breaks

[Ronnie Sahlberg]

For SMB1 oplock breaks we would grab one credit while sending the PDU
but we would never relese the credit back since we will never receive a
response to this from the server. Eventuallt this would lead to a hang
once all credits are leaked.

Fix this by defining a new flag CIFS_NO_SRV_RSP which indicates that there
is no server response to this command and thus we need to add any credits back
immediately after sending the PDU.

CC: Stable <stable@vger.kernel.org> #v5.0+
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.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

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[ronnie sahlberg]

On Thu, Apr 25, 2019 at 5:28 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вт, 23 апр. 2019 г. в 17:45, ronnie sahlberg <ronniesahlberg@gmail.com>:
> >
> > On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> >
> > Thanks for the thoughtful review. Some comments below
> >
> > > пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>:
> > > >
> > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > ---
> > > >  fs/cifs/transport.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > > index 5c59c498f56a..3b80884ddf0f 100644
> > > > --- a/fs/cifs/transport.c
> > > > +++ b/fs/cifs/transport.c
> > > > @@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > > >                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> > > >                    int *resp_buf_type, struct kvec *resp_iov)
> > > >  {
> > > > -       int i, j, optype, rc = 0;
> > > > +       int i, j, optype, opmask, rc = 0;
> > > >         struct mid_q_entry *midQ[MAX_COMPOUND];
> > > >         bool cancelled_mid[MAX_COMPOUND] = {false};
> > > >         struct cifs_credits credits[MAX_COMPOUND] = {
> > > > @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > > >         char *buf;
> > > >
> > > >         optype = flags & CIFS_OP_MASK;
> > > > +       opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
> > > >
> > > >         for (i = 0; i < num_rqst; i++)
> > > >                 resp_buf_type[i] = CIFS_NO_BUFFER;  /* no response buf yet */
> > > > @@ -1073,8 +1074,13 @@ 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 ||
> > > > +           (optype == CIFS_OBREAK_OP &&
> > > > +            opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
> > > >                 for (i = 0; i < num_rqst; i++)
> > > >                         add_credits(ses->server, &credits[i], optype);
> > > >                 goto out;
> > > > --
> > > > 2.13.6
> > > >
> > > Thanks for looking into it. Let's me clarify the defines here because
> > > their names are not straightforward and require changes:
> > >
> > > #define   CIFS_ASYNC_OP         2    /* do not wait for response */
> > >
> > > meaning that we don't need a response - probably there won't be a one
> > > (in SMB1 Oplock case). This is confusing because we also have
> > > cifs_call_async that does asynchronous processing of the responses.
> >
> > CIFS_ASYNC_OP is overloaded.
> > In two places it means we will get a response eventually (in one of
> > which "eventually" might mean unbounded time) but don't wait for it.
> > In the third place it means that there is no response to this command.
>
> Agree, we should define another one.

I added a new flag for this in the respin I sent.
There are more things we need to do but that should wait until the
next merge window I think.

>
> >
> > >
> > > #define   CIFS_NO_RESP      0x040    /* no response buffer required */
> > >
> > > meaning that a response buffer is not needed but the error code is.
> > >
> > > The problem with SMB1 oplocks was introduced when credits calculation
> > > was moved to the demultiplex thread. The server might not respond to
> > > such a request at all - that's why it is a special case.
> > >
> > > So, CIFS_NO_RESP flags should be handled like we do today - no changes
> > > are needed here. We wait for a response but just do not return a
> > > buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
> > > to add credits back once we send such a request. Prior to credit
> > > movement to the demultiplex thread label "out" has the code to add
> > > credits back. So I think the only fix that is needed here is:
> > >
> > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > index 5c59c49..26e249d 100644
> > > --- a/fs/cifs/transport.c
> > > +++ b/fs/cifs/transport.c
> > > @@ -1095,8 +1095,11 @@ 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)
> > > +       if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
> > > +               for (i = 0; i < num_rqst; i++)
> > > +                       add_credits(ses->server, &credits[i], optype);
> > >                 goto out;
> > > +       }
> >
> > The reason I did it the way I did was that I wanted this change to
> > only affect the smb1 oplock breaks and leave all other commands and
> > behavior unchanged.
> > Maybe we need a new flag that means that there will never be a
> > response at all to this command so we must add all credits back
> > immediately once we have sent the pdu out on the wire.
> >
> > We use CIFS_ASYNC_OP for three different cases.
> > When sending an oplock break, in which case we know we will never get
> > a response from the server (and thus we need to add the credits back
> > somehow which is not the demultiplex loop)
> > but also when we send an Echo request or when we send a Notify request
> > both cases where we WILL eventually receive a reply back from the
> > server and where we should add the credits back in the demultiplex
> > loop.
>
> Only SMB1 oplocks uses compound_send_recv() to send CIFS_ASYNC_OP.
> Notify uses SendReceive() but Notify itself is not being called
> anywhere. Echo uses cifs_call_async which is a completely different
> code path.

This is all for a separate patch for the next merge window, but:

We can remove Notify() since it is not used.
Since this is the only callsite for SendReceive() that uses CIFS_ASYNC_OP
we can then also remove the check for it in SendReceive()

With the new flag I added we no longer need to check for CIFS_ASYNC_OP
in compound_send_recv()
either so I removed it in the patch.

This leaves CIFS_ASYNC_OP used in only two places and the only thing
it is used for is to indicate that
we do not want to block in wait_for_credits().
Thus possibly we should rename this to CIFS_NON_BLOCK.

The semantics then become :
* CIFS_NO_SRV_RSP :  indicating that we will never get a response.
I.e. smb1 oplock break.
* cifs_call_async() : this is what is used to request async processing
of the reply.
* CIFS_NON_BLOCK : in both/all cases used to indicate we don't want to
block in wait_for_credit()

and we might also rename CIFS_NO_RSP to CIFS_NO_SRV_RSP to make it
clearer what this flag indicates.


>
> > So if we add the credits back already here for all CIFS_ASYNC_OPs,
> > doesn't that mean that we could possibly add the credits back twice?
> > Once here and then once again in the demultiplex loop when we
> > get the response?
>
> You are right. In most cases we will manage to delete the mid going to
> "out" label but it is still possible that the server will respond too
> fast for SMB1 oplock and the demultiplex thread will add the credits
> twice. We need a special no-op callback to set for such mid in the
> same loop I propose, so we do not add credits twice.
>
> > Since CIFS_ASYNC_OP suggests that it is an async operation but that we
> > will eventually get a reply (well that is how I parse the name) I feel
> > uncomfortable with that change since it conflates async operations
> > with
> > send-only-and-never-get-a-response operations.
>
> Agree. CIFS_ASYNC_OP is being parsed for me as "return immediately
> after sending a request and handle a response asynchronously". We
> should use this one only in cifs_call_async(), I guess.
>
> >
> > Maybe we need a new flag to better describe the unique case for
> > requests we know will never get a reply ?
> > CIFS_NO_SERVER_RESPONSE  or something?
>
> Let's do CIFS_NO_SRV_RSP to make it shorter - it will be set by SMB1
> oplock only and compound_send_recv won't be handle CIFS_ASYNC_OP
> operations at all truly reflecting its name - we have cifs_call_async
> for those purposes.
>
> > What do you think?
> >
> > >
> > >         for (i = 0; i < num_rqst; i++) {
> > >                 rc = wait_for_response(ses->server, midQ[i]);
> > >
> > > It worked locally against Samba server for me.
> > >
> > > I would also suggest as a separate change to rename:
> > >
> > > CIFS_ASYNC_OP to CIFS_NO_WAIT, and
> >
> > Not sure about that. I think "async operation" and "don't wait for
> > reply" are pretty equivalent so I don't see a need to rename here.
> > What is unique with smb1 oplock releases is not that they are async (I
> > could argue that they are not) but that they are requests we send that
> > will never have a response pdu from the server.
>
> Ok. Another option is to let cifs_call_async know about the new
> CIFS_NO_SRV_RSP flag and pass a combination of
> CIFS_ASYNC_OP | CIFS_NO_SRV_RSP to the function that will just send a
> request and then delete a mid. No-op callback is still needed there.
>
> >
> > >
> > > CIFS_NO_RESP to CIFS_NO_RESP_BUF
> >
> > I think this would be a good rename. But lets put it in a different
> > patch that only does a rename but does not change any logic.
> >
>
> +1 for a separate patch. Probably RSP instead of RESP, but don't have
> strong preferences here.
>
> --
> Best regards,
> Pavel Shilovsky

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[Pavel Shilovsky]

вт, 23 апр. 2019 г. в 17:45, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
>
> Thanks for the thoughtful review. Some comments below
>
> > пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>:
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/transport.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > > index 5c59c498f56a..3b80884ddf0f 100644
> > > --- a/fs/cifs/transport.c
> > > +++ b/fs/cifs/transport.c
> > > @@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > >                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> > >                    int *resp_buf_type, struct kvec *resp_iov)
> > >  {
> > > -       int i, j, optype, rc = 0;
> > > +       int i, j, optype, opmask, rc = 0;
> > >         struct mid_q_entry *midQ[MAX_COMPOUND];
> > >         bool cancelled_mid[MAX_COMPOUND] = {false};
> > >         struct cifs_credits credits[MAX_COMPOUND] = {
> > > @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > >         char *buf;
> > >
> > >         optype = flags & CIFS_OP_MASK;
> > > +       opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
> > >
> > >         for (i = 0; i < num_rqst; i++)
> > >                 resp_buf_type[i] = CIFS_NO_BUFFER;  /* no response buf yet */
> > > @@ -1073,8 +1074,13 @@ 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 ||
> > > +           (optype == CIFS_OBREAK_OP &&
> > > +            opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
> > >                 for (i = 0; i < num_rqst; i++)
> > >                         add_credits(ses->server, &credits[i], optype);
> > >                 goto out;
> > > --
> > > 2.13.6
> > >
> > Thanks for looking into it. Let's me clarify the defines here because
> > their names are not straightforward and require changes:
> >
> > #define   CIFS_ASYNC_OP         2    /* do not wait for response */
> >
> > meaning that we don't need a response - probably there won't be a one
> > (in SMB1 Oplock case). This is confusing because we also have
> > cifs_call_async that does asynchronous processing of the responses.
>
> CIFS_ASYNC_OP is overloaded.
> In two places it means we will get a response eventually (in one of
> which "eventually" might mean unbounded time) but don't wait for it.
> In the third place it means that there is no response to this command.

Agree, we should define another one.

>
> >
> > #define   CIFS_NO_RESP      0x040    /* no response buffer required */
> >
> > meaning that a response buffer is not needed but the error code is.
> >
> > The problem with SMB1 oplocks was introduced when credits calculation
> > was moved to the demultiplex thread. The server might not respond to
> > such a request at all - that's why it is a special case.
> >
> > So, CIFS_NO_RESP flags should be handled like we do today - no changes
> > are needed here. We wait for a response but just do not return a
> > buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
> > to add credits back once we send such a request. Prior to credit
> > movement to the demultiplex thread label "out" has the code to add
> > credits back. So I think the only fix that is needed here is:
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 5c59c49..26e249d 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -1095,8 +1095,11 @@ 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)
> > +       if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
> > +               for (i = 0; i < num_rqst; i++)
> > +                       add_credits(ses->server, &credits[i], optype);
> >                 goto out;
> > +       }
>
> The reason I did it the way I did was that I wanted this change to
> only affect the smb1 oplock breaks and leave all other commands and
> behavior unchanged.
> Maybe we need a new flag that means that there will never be a
> response at all to this command so we must add all credits back
> immediately once we have sent the pdu out on the wire.
>
> We use CIFS_ASYNC_OP for three different cases.
> When sending an oplock break, in which case we know we will never get
> a response from the server (and thus we need to add the credits back
> somehow which is not the demultiplex loop)
> but also when we send an Echo request or when we send a Notify request
> both cases where we WILL eventually receive a reply back from the
> server and where we should add the credits back in the demultiplex
> loop.

Only SMB1 oplocks uses compound_send_recv() to send CIFS_ASYNC_OP.
Notify uses SendReceive() but Notify itself is not being called
anywhere. Echo uses cifs_call_async which is a completely different
code path.

> So if we add the credits back already here for all CIFS_ASYNC_OPs,
> doesn't that mean that we could possibly add the credits back twice?
> Once here and then once again in the demultiplex loop when we
> get the response?

You are right. In most cases we will manage to delete the mid going to
"out" label but it is still possible that the server will respond too
fast for SMB1 oplock and the demultiplex thread will add the credits
twice. We need a special no-op callback to set for such mid in the
same loop I propose, so we do not add credits twice.

> Since CIFS_ASYNC_OP suggests that it is an async operation but that we
> will eventually get a reply (well that is how I parse the name) I feel
> uncomfortable with that change since it conflates async operations
> with
> send-only-and-never-get-a-response operations.

Agree. CIFS_ASYNC_OP is being parsed for me as "return immediately
after sending a request and handle a response asynchronously". We
should use this one only in cifs_call_async(), I guess.

>
> Maybe we need a new flag to better describe the unique case for
> requests we know will never get a reply ?
> CIFS_NO_SERVER_RESPONSE  or something?

Let's do CIFS_NO_SRV_RSP to make it shorter - it will be set by SMB1
oplock only and compound_send_recv won't be handle CIFS_ASYNC_OP
operations at all truly reflecting its name - we have cifs_call_async
for those purposes.

> What do you think?
>
> >
> >         for (i = 0; i < num_rqst; i++) {
> >                 rc = wait_for_response(ses->server, midQ[i]);
> >
> > It worked locally against Samba server for me.
> >
> > I would also suggest as a separate change to rename:
> >
> > CIFS_ASYNC_OP to CIFS_NO_WAIT, and
>
> Not sure about that. I think "async operation" and "don't wait for
> reply" are pretty equivalent so I don't see a need to rename here.
> What is unique with smb1 oplock releases is not that they are async (I
> could argue that they are not) but that they are requests we send that
> will never have a response pdu from the server.

Ok. Another option is to let cifs_call_async know about the new
CIFS_NO_SRV_RSP flag and pass a combination of
CIFS_ASYNC_OP | CIFS_NO_SRV_RSP to the function that will just send a
request and then delete a mid. No-op callback is still needed there.

>
> >
> > CIFS_NO_RESP to CIFS_NO_RESP_BUF
>
> I think this would be a good rename. But lets put it in a different
> patch that only does a rename but does not change any logic.
>

+1 for a separate patch. Probably RSP instead of RESP, but don't have
strong preferences here.

--
Best regards,
Pavel Shilovsky

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[ronnie sahlberg]

On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>

Thanks for the thoughtful review. Some comments below

> пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/transport.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 5c59c498f56a..3b80884ddf0f 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >                    const int flags, const int num_rqst, struct smb_rqst *rqst,
> >                    int *resp_buf_type, struct kvec *resp_iov)
> >  {
> > -       int i, j, optype, rc = 0;
> > +       int i, j, optype, opmask, rc = 0;
> >         struct mid_q_entry *midQ[MAX_COMPOUND];
> >         bool cancelled_mid[MAX_COMPOUND] = {false};
> >         struct cifs_credits credits[MAX_COMPOUND] = {
> > @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >         char *buf;
> >
> >         optype = flags & CIFS_OP_MASK;
> > +       opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
> >
> >         for (i = 0; i < num_rqst; i++)
> >                 resp_buf_type[i] = CIFS_NO_BUFFER;  /* no response buf yet */
> > @@ -1073,8 +1074,13 @@ 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 ||
> > +           (optype == CIFS_OBREAK_OP &&
> > +            opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
> >                 for (i = 0; i < num_rqst; i++)
> >                         add_credits(ses->server, &credits[i], optype);
> >                 goto out;
> > --
> > 2.13.6
> >
> Thanks for looking into it. Let's me clarify the defines here because
> their names are not straightforward and require changes:
>
> #define   CIFS_ASYNC_OP         2    /* do not wait for response */
>
> meaning that we don't need a response - probably there won't be a one
> (in SMB1 Oplock case). This is confusing because we also have
> cifs_call_async that does asynchronous processing of the responses.

CIFS_ASYNC_OP is overloaded.
In two places it means we will get a response eventually (in one of
which "eventually" might mean unbounded time) but don't wait for it.
In the third place it means that there is no response to this command.

>
> #define   CIFS_NO_RESP      0x040    /* no response buffer required */
>
> meaning that a response buffer is not needed but the error code is.
>
> The problem with SMB1 oplocks was introduced when credits calculation
> was moved to the demultiplex thread. The server might not respond to
> such a request at all - that's why it is a special case.
>
> So, CIFS_NO_RESP flags should be handled like we do today - no changes
> are needed here. We wait for a response but just do not return a
> buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
> to add credits back once we send such a request. Prior to credit
> movement to the demultiplex thread label "out" has the code to add
> credits back. So I think the only fix that is needed here is:
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c59c49..26e249d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -1095,8 +1095,11 @@ 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)
> +       if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
> +               for (i = 0; i < num_rqst; i++)
> +                       add_credits(ses->server, &credits[i], optype);
>                 goto out;
> +       }

The reason I did it the way I did was that I wanted this change to
only affect the smb1 oplock breaks and leave all other commands and
behavior unchanged.
Maybe we need a new flag that means that there will never be a
response at all to this command so we must add all credits back
immediately once we have sent the pdu out on the wire.

We use CIFS_ASYNC_OP for three different cases.
When sending an oplock break, in which case we know we will never get
a response from the server (and thus we need to add the credits back
somehow which is not the demultiplex loop)
but also when we send an Echo request or when we send a Notify request
both cases where we WILL eventually receive a reply back from the
server and where we should add the credits back in the demultiplex
loop.
So if we add the credits back already here for all CIFS_ASYNC_OPs,
doesn't that mean that we could possibly add the credits back twice?
Once here and then once again in the demultiplex loop when we
get the response?


Since CIFS_ASYNC_OP suggests that it is an async operation but that we
will eventually get a reply (well that is how I parse the name) I feel
uncomfortable with that change since it conflates async operations
with
send-only-and-never-get-a-response operations.

Maybe we need a new flag to better describe the unique case for
requests we know will never get a reply ?
CIFS_NO_SERVER_RESPONSE  or something?


What do you think?

>
>         for (i = 0; i < num_rqst; i++) {
>                 rc = wait_for_response(ses->server, midQ[i]);
>
> It worked locally against Samba server for me.
>
> I would also suggest as a separate change to rename:
>
> CIFS_ASYNC_OP to CIFS_NO_WAIT, and

Not sure about that. I think "async operation" and "don't wait for
reply" are pretty equivalent so I don't see a need to rename here.
What is unique with smb1 oplock releases is not that they are async (I
could argue that they are not) but that they are requests we send that
will never have a response pdu from the server.

>
> CIFS_NO_RESP to CIFS_NO_RESP_BUF

I think this would be a good rename. But lets put it in a different
patch that only does a rename but does not change any logic.

>
> to reflect the exact meanings.
>
> --
> Best regards,
> Pavel Shilovsky

Re: [PATCH] cifs: fix credits leak for SMB1 oplock breaks

[Pavel Shilovsky]

пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/transport.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c59c498f56a..3b80884ddf0f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                    const int flags, const int num_rqst, struct smb_rqst *rqst,
>                    int *resp_buf_type, struct kvec *resp_iov)
>  {
> -       int i, j, optype, rc = 0;
> +       int i, j, optype, opmask, rc = 0;
>         struct mid_q_entry *midQ[MAX_COMPOUND];
>         bool cancelled_mid[MAX_COMPOUND] = {false};
>         struct cifs_credits credits[MAX_COMPOUND] = {
> @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>         char *buf;
>
>         optype = flags & CIFS_OP_MASK;
> +       opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
>
>         for (i = 0; i < num_rqst; i++)
>                 resp_buf_type[i] = CIFS_NO_BUFFER;  /* no response buf yet */
> @@ -1073,8 +1074,13 @@ 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 ||
> +           (optype == CIFS_OBREAK_OP &&
> +            opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
>                 for (i = 0; i < num_rqst; i++)
>                         add_credits(ses->server, &credits[i], optype);
>                 goto out;
> --
> 2.13.6
>
Thanks for looking into it. Let's me clarify the defines here because
their names are not straightforward and require changes:

#define   CIFS_ASYNC_OP         2    /* do not wait for response */

meaning that we don't need a response - probably there won't be a one
(in SMB1 Oplock case). This is confusing because we also have
cifs_call_async that does asynchronous processing of the responses.

#define   CIFS_NO_RESP      0x040    /* no response buffer required */

meaning that a response buffer is not needed but the error code is.

The problem with SMB1 oplocks was introduced when credits calculation
was moved to the demultiplex thread. The server might not respond to
such a request at all - that's why it is a special case.

So, CIFS_NO_RESP flags should be handled like we do today - no changes
are needed here. We wait for a response but just do not return a
buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
to add credits back once we send such a request. Prior to credit
movement to the demultiplex thread label "out" has the code to add
credits back. So I think the only fix that is needed here is:

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c59c49..26e249d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -1095,8 +1095,11 @@ 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)
+       if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
+               for (i = 0; i < num_rqst; i++)
+                       add_credits(ses->server, &credits[i], optype);
                goto out;
+       }

        for (i = 0; i < num_rqst; i++) {
                rc = wait_for_response(ses->server, midQ[i]);

It worked locally against Samba server for me.

I would also suggest as a separate change to rename:

CIFS_ASYNC_OP to CIFS_NO_WAIT, and

CIFS_NO_RESP to CIFS_NO_RESP_BUF

to reflect the exact meanings.

--
Best regards,
Pavel Shilovsky

[PATCH] cifs: fix credits leak for SMB1 oplock breaks

[Ronnie Sahlberg]

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

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5c59c498f56a..3b80884ddf0f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 		   const int flags, const int num_rqst, struct smb_rqst *rqst,
 		   int *resp_buf_type, struct kvec *resp_iov)
 {
-	int i, j, optype, rc = 0;
+	int i, j, optype, opmask, rc = 0;
 	struct mid_q_entry *midQ[MAX_COMPOUND];
 	bool cancelled_mid[MAX_COMPOUND] = {false};
 	struct cifs_credits credits[MAX_COMPOUND] = {
@@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
 	char *buf;
 
 	optype = flags & CIFS_OP_MASK;
+	opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
 
 	for (i = 0; i < num_rqst; i++)
 		resp_buf_type[i] = CIFS_NO_BUFFER;  /* no response buf yet */
@@ -1073,8 +1074,13 @@ 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 ||
+	    (optype == CIFS_OBREAK_OP &&
+	     opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
 		for (i = 0; i < num_rqst; i++)
 			add_credits(ses->server, &credits[i], optype);
 		goto out;
-- 
2.13.6