| Message ID | 20241202232802.290935-3-vinicius.peixoto@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Azure: backport SMB lease key fixes | expand |
On 12/2/24 5:27 PM, Vinicius Peixoto wrote: > From: Meetakshi Setiya <msetiya@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@microsoft.com> > Signed-off-by: Steve French <stfrench@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@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.
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@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@microsoft.com> >> Signed-off-by: Steve French <stfrench@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@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
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@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@microsoft.com> >>> Signed-off-by: Steve French <stfrench@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@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@canonical.com> > > Thanks, > Vinicius >
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)