Re: [PATCH] smb3: fix lease break problem introduced by compounding

[Pavel Shilovsky <piastryyy@xxxxxxxxx>]

пт, 28 сент. 2018 г. в 0:35, Steve French <smfrench@xxxxxxxxx>:
>
> 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@xxxxxxxxxxxxx>
> ---
>  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