Re: [PATCH 3/3] CIFS: Do not miss cancelled OPEN responses

[ronnie sahlberg <ronniesahlberg@gmail.com>]

Very nice.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com
>

On Fri, Nov 22, 2019 at 5:35 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> When an OPEN command is cancelled we mark a mid as
> cancelled and let the demultiplex thread process it
> by closing an open handle. The problem is there is
> a race between a system call thread and the demultiplex
> thread and there may be a situation when the mid has
> been already processed before it is set as cancelled.
>
> Fix this by processing cancelled requests when mids
> are being destroyed which means that there is only
> one thread referencing a particular mid. Also set
> mids as cancelled unconditionally on their state.
>
> Cc: Stable <stable@vger.kernel.org>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>  fs/cifs/connect.c   |  6 ------
>  fs/cifs/transport.c | 10 ++++++++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index e63d16d8048a..59feb2de389e 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1229,12 +1229,6 @@ cifs_demultiplex_thread(void *p)
>                 for (i = 0; i < num_mids; i++) {
>                         if (mids[i] != NULL) {
>                                 mids[i]->resp_buf_size = server->pdu_size;
> -                               if ((mids[i]->mid_flags & MID_WAIT_CANCELLED) &&
> -                                   mids[i]->mid_state == MID_RESPONSE_RECEIVED &&
> -                                   server->ops->handle_cancelled_mid)
> -                                       server->ops->handle_cancelled_mid(
> -                                                       mids[i]->resp_buf,
> -                                                       server);
>
>                                 if (!mids[i]->multiRsp || mids[i]->multiEnd)
>                                         mids[i]->callback(mids[i]);
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index bb52751ba783..987ffcd5ca3a 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -93,8 +93,14 @@ static void _cifs_mid_q_entry_release(struct kref *refcount)
>         __u16 smb_cmd = le16_to_cpu(midEntry->command);
>         unsigned long now;
>         unsigned long roundtrip_time;
> -       struct TCP_Server_Info *server = midEntry->server;
>  #endif
> +       struct TCP_Server_Info *server = midEntry->server;
> +
> +       if (midEntry->resp_buf && (midEntry->mid_flags & MID_WAIT_CANCELLED) &&
> +           midEntry->mid_state == MID_RESPONSE_RECEIVED &&
> +           server->ops->handle_cancelled_mid)
> +               server->ops->handle_cancelled_mid(midEntry->resp_buf, server);
> +
>         midEntry->mid_state = MID_FREE;
>         atomic_dec(&midCount);
>         if (midEntry->large_buf)
> @@ -1115,8 +1121,8 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
>                                  midQ[i]->mid, le16_to_cpu(midQ[i]->command));
>                         send_cancel(server, &rqst[i], midQ[i]);
>                         spin_lock(&GlobalMid_Lock);
> +                       midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
>                         if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED) {
> -                               midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
>                                 midQ[i]->callback = cifs_cancelled_callback;
>                                 cancelled_mid[i] = true;
>                                 credits[i].value = 0;
> --
> 2.17.1
>