NAK/Cmnt: [SRU][n:linux-azure][PATCH 2/7] smb: client: do not defer close open handles to deleted files
Vinicius Peixoto
vinicius.peixoto at canonical.com
Wed Jan 8 14:54:35 UTC 2025
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()
Thanks,
Vinicius
More information about the kernel-team
mailing list