Message ID | 20210312142646.32579-2-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
Series | CIFS open/close race fixes | expand |
On 12/03/2021 15:26, Tim Gardner wrote: > From: Pavel Shilovsky <pshilov@microsoft.com> > > BugLink: https://bugs.launchpad.net/bugs/1918714 > > 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> > Tested-by: Frank Sorenson <sorenson@redhat.com> > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com> > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > Signed-off-by: Steve French <stfrench@microsoft.com> > (backported from commit 9150c3adbf24d77cfba37f03639d4a908ca4ac25) > [ fs/cifs/smb2pdu.c has had significant changes, so the part of this > patch affecting fs/cifs/smb2pdu.c was pieced in where the logic looked > correct ] > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > > v2 - ran scripts/checkpatch.pl and corrected formatting errors. > > fs/cifs/smb2misc.c | 59 ++++++++++++++++++++++++++++++++++----------- > fs/cifs/smb2pdu.c | 11 +++++++-- > fs/cifs/smb2proto.h | 3 +++ > 3 files changed, 57 insertions(+), 16 deletions(-) > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index 0c6e5450ff76..d15ace6151d9 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -674,34 +674,65 @@ 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 = get_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 ab1df4311a6d..04e1cb66baf8 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2133,9 +2133,17 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, > rsp = (struct smb2_close_rsp *)rsp_iov.iov_base; > > if (rc != 0) { > + /* retry close in a worker thread if this one is interrupted */ > + if (rc == -EINTR) { > + int 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); > + } > cifs_stats_fail_inc(tcon, SMB2_CLOSE_HE); > goto close_exit; > - } > + } > Still not good here. The closing bracket should stay as it was. Best regards, Krzysztof
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index 0c6e5450ff76..d15ace6151d9 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -674,34 +674,65 @@ 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 = get_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 ab1df4311a6d..04e1cb66baf8 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2133,9 +2133,17 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, rsp = (struct smb2_close_rsp *)rsp_iov.iov_base; if (rc != 0) { + /* retry close in a worker thread if this one is interrupted */ + if (rc == -EINTR) { + int 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); + } cifs_stats_fail_inc(tcon, SMB2_CLOSE_HE); goto close_exit; - } + } /* BB FIXME - decode close response, update inode for caching */ @@ -2147,7 +2155,6 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, static int validate_buf(unsigned int offset, unsigned int buffer_length, struct smb2_hdr *hdr, unsigned int min_buf_size) - { unsigned int smb_len = be32_to_cpu(hdr->smb2_buf_length); char *end_of_smb = smb_len + 4 /* RFC1001 length field */ + (char *)hdr; diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 3b8e9c2e55bc..245a2595f1b1 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -180,6 +180,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);