Message ID | 20210311181857.10268-2-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
Series | [1/3] CIFS: Close open handle after interrupted close | expand |
On 11/03/2021 19:18, 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> > --- > > fs/cifs/smb2misc.c | 59 ++++++++++++++++++++++++++++++++++----------- > fs/cifs/smb2pdu.c | 9 ++++++- > fs/cifs/smb2proto.h | 3 +++ > 3 files changed, 56 insertions(+), 15 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..03e4725390d4 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2133,6 +2133,14 @@ 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); > + } Hi Tim, The context looks good but you have here mismatched indentation. Best regards, Krzysztof
On 3/12/21 3:32 AM, Krzysztof Kozlowski wrote: > On 11/03/2021 19:18, 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> >> --- >> >> fs/cifs/smb2misc.c | 59 ++++++++++++++++++++++++++++++++++----------- >> fs/cifs/smb2pdu.c | 9 ++++++- >> fs/cifs/smb2proto.h | 3 +++ >> 3 files changed, 56 insertions(+), 15 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..03e4725390d4 100644 >> --- a/fs/cifs/smb2pdu.c >> +++ b/fs/cifs/smb2pdu.c >> @@ -2133,6 +2133,14 @@ 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); >> + } > > Hi Tim, > > The context looks good but you have here mismatched indentation. > Is that an ACK, or do you want me to fix the indentation ? Which line in particular do you think is wrong ? rtg ----------- Tim Gardner Canonical, Inc
On 12/03/2021 14:32, Tim Gardner wrote: > > > On 3/12/21 3:32 AM, Krzysztof Kozlowski wrote: >> On 11/03/2021 19:18, 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> >>> --- >>> >>> fs/cifs/smb2misc.c | 59 ++++++++++++++++++++++++++++++++++----------- >>> fs/cifs/smb2pdu.c | 9 ++++++- >>> fs/cifs/smb2proto.h | 3 +++ >>> 3 files changed, 56 insertions(+), 15 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..03e4725390d4 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -2133,6 +2133,14 @@ 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); >>> + } >> >> Hi Tim, >> >> The context looks good but you have here mismatched indentation. >> > > Is that an ACK, or do you want me to fix the indentation ? Which line in > particular do you think is wrong ? I propose to fix the indentation, so it's not the Ack. This entire block above is shown to me with spaces instead of tabs. 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..03e4725390d4 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2133,6 +2133,14 @@ 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; } @@ -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);