ACK/Cmnt: [SRU][n:linux-azure][PATCH 2/7] smb: client: do not defer close open handles to deleted files
John Cabaj
john.cabaj at canonical.com
Wed Jan 8 14:58:43 UTC 2025
On 1/8/25 8:54 AM, Vinicius Peixoto wrote:
> Hi John,
>
> On 1/7/25 19:40, John Cabaj wrote:
>> On 12/2/24 5:27 PM, Vinicius Peixoto wrote:
>>> From: Meetakshi Setiya <msetiya at microsoft.com>
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/2090880
>>>
>>> When a file/dentry has been deleted before closing all its open
>>> handles, currently, closing them can add them to the deferred
>>> close list. This can lead to problems in creating file with the
>>> same name when the file is re-created before the deferred close
>>> completes. This issue was seen while reusing a client's already
>>> existing lease on a file for compound operations and xfstest 591
>>> failed because of the deferred close handle that remained valid
>>> even after the file was deleted and was being reused to create a
>>> file with the same name. The server in this case returns an error
>>> on open with STATUS_DELETE_PENDING. Recreating the file would
>>> fail till the deferred handles are closed (duration specified in
>>> closetimeo).
>>>
>>> This patch fixes the issue by flagging all open handles for the
>>> deleted file (file path to be precise) by setting
>>> status_file_deleted to true in the cifsFileInfo structure. As per
>>> the information classes specified in MS-FSCC, SMB2 query info
>>> response from the server has a DeletePending field, set to true
>>> to indicate that deletion has been requested on that file. If
>>> this is the case, flag the open handles for this file too.
>>>
>>> When doing close in cifs_close for each of these handles, check the
>>> value of this boolean field and do not defer close these handles
>>> if the corresponding filepath has been deleted.
>>>
>>> Signed-off-by: Meetakshi Setiya <msetiya at microsoft.com>
>>> Signed-off-by: Steve French <stfrench at microsoft.com>
>>> (backported from commit ffceb7640cbfe6ea60e7769e107451d63a2fe3d3)
>>> [vpeixoto: fix a spurious context conflict in a neighboring line due
>>> to ffde9c7ea8cf9 ("smb3: retrying on failed server close"), that came
>>> from the stable tree (hence the conflict with the mainline change)]
>>> Signed-off-by: Vinicius Peixoto <vinicius.peixoto at canonical.com>
>>> ---
>>> fs/smb/client/cifsglob.h | 1 +
>>> fs/smb/client/cifsproto.h | 4 ++++
>>> fs/smb/client/file.c | 3 ++-
>>> fs/smb/client/inode.c | 28 +++++++++++++++++++++++++---
>>> fs/smb/client/misc.c | 34 ++++++++++++++++++++++++++++++++++
>>> fs/smb/client/smb2inode.c | 9 ++++++++-
>>> 6 files changed, 74 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>>> index 8cc0aed72ec1..93eb9d6cd457 100644
>>> --- a/fs/smb/client/cifsglob.h
>>> +++ b/fs/smb/client/cifsglob.h
>>> @@ -1415,6 +1415,7 @@ struct cifsFileInfo {
>>> bool swapfile:1;
>>> bool oplock_break_cancelled:1;
>>> bool offload:1; /* offload final part of _put to a wq */
>>> + bool status_file_deleted:1; /* file has been deleted */
>>> unsigned int oplock_epoch; /* epoch from the lease break */
>>> __u32 oplock_level; /* oplock/lease level from the lease break */
>>> int count;
>>> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>>> index fb997ffd4b50..8e0a348f1f66 100644
>>> --- a/fs/smb/client/cifsproto.h
>>> +++ b/fs/smb/client/cifsproto.h
>>> @@ -294,6 +294,10 @@ extern void cifs_close_all_deferred_files(struct
>>> cifs_tcon *cifs_tcon);
>>> extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon
>>> *cifs_tcon,
>>> const char *path);
>>> +
>>> +extern void cifs_mark_open_handles_for_deleted_file(struct inode
>>> *inode,
>>> + const char *path);
>>> +
>>> extern struct TCP_Server_Info *
>>> cifs_get_tcp_session(struct smb3_fs_context *ctx,
>>> struct TCP_Server_Info *primary_server);
>>> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>>> index d495e3511014..bb23ab1b1188 100644
>>> --- a/fs/smb/client/file.c
>>> +++ b/fs/smb/client/file.c
>>> @@ -501,6 +501,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct
>>> cifs_fid *fid, struct file *file,
>>> cfile->uid = current_fsuid();
>>> cfile->dentry = dget(dentry);
>>> cfile->f_flags = file->f_flags;
>>> + cfile->status_file_deleted = false;
>>> cfile->invalidHandle = false;
>>> cfile->deferred_close_scheduled = false;
>>> cfile->tlink = cifs_get_tlink(tlink);
>>> @@ -1167,7 +1168,7 @@ int cifs_close(struct inode *inode, struct file
>>> *file)
>>> if ((cifs_sb->ctx->closetimeo && cinode->oplock ==
>>> CIFS_CACHE_RHW_FLG)
>>> && cinode->lease_granted &&
>>> !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
>>> - dclose) {
>>> + dclose && !(cfile->status_file_deleted)) {
>>> if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode-
>>> >flags)) {
>>> inode_set_mtime_to_ts(inode,
>>> inode_set_ctime_current(inode));
>>> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
>>> index 1c8b1724b769..5a2c5aa994af 100644
>>> --- a/fs/smb/client/inode.c
>>> +++ b/fs/smb/client/inode.c
>>> @@ -819,6 +819,9 @@ cifs_get_file_info(struct file *filp)
>>> struct cifsFileInfo *cfile = filp->private_data;
>>> struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>>> struct TCP_Server_Info *server = tcon->ses->server;
>>> + struct dentry *dentry = filp->f_path.dentry;
>>> + void *page = alloc_dentry_path();
>>> + const unsigned char *path;
>>> if (!server->ops->query_file_info)
>>> return -ENOSYS;
>>> @@ -833,7 +836,14 @@ cifs_get_file_info(struct file *filp)
>>> data.symlink = true;
>>> data.reparse.tag = IO_REPARSE_TAG_SYMLINK;
>>> }
>>> + path = build_path_from_dentry(dentry, page);
>>> + if (IS_ERR(path)) {
>>> + free_dentry_path(page);
>>> + return PTR_ERR(path);
>>> + }
>>> cifs_open_info_to_fattr(&fattr, &data, inode->i_sb);
>>> + if (fattr.cf_flags & CIFS_FATTR_DELETE_PENDING)
>>> + cifs_mark_open_handles_for_deleted_file(inode, path);
>>> break;
>>> case -EREMOTE:
>>> cifs_create_junction_fattr(&fattr, inode->i_sb);
>>> @@ -863,6 +873,7 @@ cifs_get_file_info(struct file *filp)
>>> rc = cifs_fattr_to_inode(inode, &fattr, false);
>>> cgfi_exit:
>>> cifs_free_open_info(&data);
>>> + free_dentry_path(page);
>>> free_xid(xid);
>>> return rc;
>>> }
>>> @@ -1001,6 +1012,7 @@ static int reparse_info_to_fattr(struct
>>> cifs_open_info_data *data,
>>> struct kvec rsp_iov, *iov = NULL;
>>> int rsp_buftype = CIFS_NO_BUFFER;
>>> u32 tag = data->reparse.tag;
>>> + struct inode *inode = NULL;
>>> int rc = 0;
>>> if (!tag && server->ops->query_reparse_point) {
>>> @@ -1053,8 +1065,12 @@ static int reparse_info_to_fattr(struct
>>> cifs_open_info_data *data,
>>> if (tcon->posix_extensions)
>>> smb311_posix_info_to_fattr(fattr, data, sb);
>>> - else
>>> + else {
>>> cifs_open_info_to_fattr(fattr, data, sb);
>>> + inode = cifs_iget(sb, fattr);
>>> + if (inode && fattr->cf_flags & CIFS_FATTR_DELETE_PENDING)
>>> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
>>> + }
>>> out:
>>> fattr->cf_cifstag = data->reparse.tag;
>>> free_rsp_buf(rsp_buftype, rsp_iov.iov_base);
>>> @@ -1109,6 +1125,8 @@ static int cifs_get_fattr(struct
>>> cifs_open_info_data *data,
>>> full_path, fattr);
>>> } else {
>>> cifs_open_info_to_fattr(fattr, data, sb);
>>> + if (fattr->cf_flags & CIFS_FATTR_DELETE_PENDING)
>>> + cifs_mark_open_handles_for_deleted_file(*inode,
>>> full_path);
>>> }
>>> break;
>>> case -EREMOTE:
>>> @@ -1791,16 +1809,20 @@ int cifs_unlink(struct inode *dir, struct
>>> dentry *dentry)
>>> psx_del_no_retry:
>>> if (!rc) {
>>> - if (inode)
>>> + if (inode) {
>>> + cifs_mark_open_handles_for_deleted_file(inode, full_path);
>>> cifs_drop_nlink(inode);
>>> + }
>>> } else if (rc == -ENOENT) {
>>> d_drop(dentry);
>>> } else if (rc == -EBUSY) {
>>> if (server->ops->rename_pending_delete) {
>>> rc = server->ops->rename_pending_delete(full_path,
>>> dentry, xid);
>>> - if (rc == 0)
>>> + if (rc == 0) {
>>> + cifs_mark_open_handles_for_deleted_file(inode,
>>> full_path);
>>> cifs_drop_nlink(inode);
>>> + }
>>> }
>>> } else if ((rc == -EACCES) && (dosattr == 0) && inode) {
>>> attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
>>> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>>> index 5cf50262aa03..8528407571a9 100644
>>> --- a/fs/smb/client/misc.c
>>> +++ b/fs/smb/client/misc.c
>>> @@ -853,6 +853,40 @@ cifs_close_deferred_file_under_dentry(struct
>>> cifs_tcon *tcon, const char *path)
>>> free_dentry_path(page);
>>> }
>>> +/*
>>> + * If a dentry has been deleted, all corresponding open handles
>>> should know that
>>> + * so that we do not defer close them.
>>> + */
>>> +void cifs_mark_open_handles_for_deleted_file(struct inode *inode,
>>> + const char *path)
>>> +{
>>> + struct cifsFileInfo *cfile;
>>> + void *page;
>>> + const char *full_path;
>>> + struct cifsInodeInfo *cinode = CIFS_I(inode);
>>> +
>>> + page = alloc_dentry_path();
>>> + spin_lock(&cinode->open_file_lock);
>>> +
>>> + /*
>>> + * note: we need to construct path from dentry and compare only
>>> if the
>>> + * inode has any hardlinks. When number of hardlinks is 1, we
>>> can just
>>> + * mark all open handles since they are going to be from the
>>> same file.
>>> + */
>>> + if (inode->i_nlink > 1) {
>>> + list_for_each_entry(cfile, &cinode->openFileList, flist) {
>>> + full_path = build_path_from_dentry(cfile->dentry, page);
>>> + if (!IS_ERR(full_path) && strcmp(full_path, path) == 0)
>>> + cfile->status_file_deleted = true;
>>> + }
>>> + } else {
>>> + list_for_each_entry(cfile, &cinode->openFileList, flist)
>>> + cfile->status_file_deleted = true;
>>> + }
>>> + spin_unlock(&cinode->open_file_lock);
>>> + free_dentry_path(page);
>>> +}
>>> +
>>> /* parses DFS referral V3 structure
>>> * caller is responsible for freeing target_nodes
>>> * returns:
>>> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
>>> index f104860517db..660893ca4ea1 100644
>>> --- a/fs/smb/client/smb2inode.c
>>> +++ b/fs/smb/client/smb2inode.c
>>> @@ -561,8 +561,15 @@ static int smb2_compound_op(const unsigned int
>>> xid, struct cifs_tcon *tcon,
>>> case SMB2_OP_DELETE:
>>> if (rc)
>>> trace_smb3_delete_err(xid, ses->Suid, tcon->tid, rc);
>>> - else
>>> + else {
>>> + /*
>>> + * If dentry (hence, inode) is NULL, lease break is
>>> going to
>>> + * take care of degrading leases on handles for
>>> deleted files.
>>> + */
>>> + if (inode)
>>> + cifs_mark_open_handles_for_deleted_file(inode,
>>> full_path);
>>> trace_smb3_delete_done(xid, ses->Suid, tcon->tid);
>>> + }
>>> break;
>>> case SMB2_OP_MKDIR:
>>> if (rc)
>>
>> Upstream commit ec4535b2a1d709d3a1fbec26739c672f13c98a7b has this
>> commit as being fixed. We should see about including this commit as well.
>
> That commit is included in this patchset, no?
>
> [SRU][n:linux-azure][PATCH 5/7] smb: client: fix NULL ptr deref in
> cifs_mark_open_handles_for_deleted_file()
So it is, commit 772f87b7d436f139f57a98483ce1353c178b4ce3. Consider this
reply another ACK.
Acked-by: John Cabaj <john.cabaj at canonical.com>
>
> Thanks,
> Vinicius
>
More information about the kernel-team
mailing list