Re: [PATCH 1/3] CIFS: Close open handle after interrupted close

[Steve French <smfrench@gmail.com>]

Merged these three (and one of Aurelien's recent patches for
multichannel - need updates to that, also waiting on Paulo's DFS fix)
into cifs-2.6.git for-next (and also the buildbot's gitub for-next
branch)


On Thu, Nov 21, 2019 at 1:35 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> If Close command is interrupted before sending a request
> to the server the client ends up leaking an open file
> handle. This wastes server resources and can potentially
> block applications that try to remove the file or any
> directory containing this file.
>
> Fix this by putting the close command into a worker queue,
> so another thread retries it later.
>
> Cc: Stable <stable@vger.kernel.org>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>  fs/cifs/smb2misc.c  | 59 ++++++++++++++++++++++++++++++++++-----------
>  fs/cifs/smb2pdu.c   | 16 +++++++++++-
>  fs/cifs/smb2proto.h |  3 +++
>  3 files changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 527c9efd3de0..80a8f4b2daab 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -725,36 +725,67 @@ smb2_cancelled_close_fid(struct work_struct *work)
>         kfree(cancelled);
>  }
>
> +/* Caller should already has an extra reference to @tcon */
> +static int
> +__smb2_handle_cancelled_close(struct cifs_tcon *tcon, __u64 persistent_fid,
> +                             __u64 volatile_fid)
> +{
> +       struct close_cancelled_open *cancelled;
> +
> +       cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
> +       if (!cancelled)
> +               return -ENOMEM;
> +
> +       cancelled->fid.persistent_fid = persistent_fid;
> +       cancelled->fid.volatile_fid = volatile_fid;
> +       cancelled->tcon = tcon;
> +       INIT_WORK(&cancelled->work, smb2_cancelled_close_fid);
> +       WARN_ON(queue_work(cifsiod_wq, &cancelled->work) == false);
> +
> +       return 0;
> +}
> +
> +int
> +smb2_handle_cancelled_close(struct cifs_tcon *tcon, __u64 persistent_fid,
> +                           __u64 volatile_fid)
> +{
> +       int rc;
> +
> +       cifs_dbg(FYI, "%s: tc_count=%d\n", __func__, tcon->tc_count);
> +       spin_lock(&cifs_tcp_ses_lock);
> +       tcon->tc_count++;
> +       spin_unlock(&cifs_tcp_ses_lock);
> +
> +       rc = __smb2_handle_cancelled_close(tcon, persistent_fid, volatile_fid);
> +       if (rc)
> +               cifs_put_tcon(tcon);
> +
> +       return rc;
> +}
> +
>  int
>  smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server)
>  {
>         struct smb2_sync_hdr *sync_hdr = (struct smb2_sync_hdr *)buffer;
>         struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer;
>         struct cifs_tcon *tcon;
> -       struct close_cancelled_open *cancelled;
> +       int rc;
>
>         if (sync_hdr->Command != SMB2_CREATE ||
>             sync_hdr->Status != STATUS_SUCCESS)
>                 return 0;
>
> -       cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
> -       if (!cancelled)
> -               return -ENOMEM;
> -
>         tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId,
>                                   sync_hdr->TreeId);
> -       if (!tcon) {
> -               kfree(cancelled);
> +       if (!tcon)
>                 return -ENOENT;
> -       }
>
> -       cancelled->fid.persistent_fid = rsp->PersistentFileId;
> -       cancelled->fid.volatile_fid = rsp->VolatileFileId;
> -       cancelled->tcon = tcon;
> -       INIT_WORK(&cancelled->work, smb2_cancelled_close_fid);
> -       queue_work(cifsiod_wq, &cancelled->work);
> +       rc = __smb2_handle_cancelled_close(tcon, rsp->PersistentFileId,
> +                                          rsp->VolatileFileId);
> +       if (rc)
> +               cifs_put_tcon(tcon);
>
> -       return 0;
> +       return rc;
>  }
>
>  /**
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 0aa40129dfb5..2f541e9efba1 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2942,7 +2942,21 @@ int
>  SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
>            u64 persistent_fid, u64 volatile_fid)
>  {
> -       return SMB2_close_flags(xid, tcon, persistent_fid, volatile_fid, 0);
> +       int rc;
> +       int tmp_rc;
> +
> +       rc = SMB2_close_flags(xid, tcon, persistent_fid, volatile_fid, 0);
> +
> +       /* retry close in a worker thread if this one is interrupted */
> +       if (rc == -EINTR) {
> +               tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
> +                                                    volatile_fid);
> +               if (tmp_rc)
> +                       cifs_dbg(VFS, "handle cancelled close fid 0x%llx returned error %d\n",
> +                                persistent_fid, tmp_rc);
> +       }
> +
> +       return rc;
>  }
>
>  int
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 07ca72486cfa..e239f98093a9 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -203,6 +203,9 @@ extern int SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
>  extern int SMB2_oplock_break(const unsigned int xid, struct cifs_tcon *tcon,
>                              const u64 persistent_fid, const u64 volatile_fid,
>                              const __u8 oplock_level);
> +extern int smb2_handle_cancelled_close(struct cifs_tcon *tcon,
> +                                      __u64 persistent_fid,
> +                                      __u64 volatile_fid);
>  extern int smb2_handle_cancelled_mid(char *buffer,
>                                         struct TCP_Server_Info *server);
>  void smb2_cancelled_close_fid(struct work_struct *work);
> --
> 2.17.1
>


-- 
Thanks,

Steve