smb3: fix lease break problem introduced by compounding

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(+)

пт, 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