| Message ID | 20190426003223.4763-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix credits leak for SMB1 oplock breaks | expand |
чт, 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
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
сб, 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
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)
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(-)