From: Meetakshi Setiya <[email protected]>
Currently, when a rename, unlink or set path size compound operation
is requested on a file that has a lot of dirty pages to be written
to the server, we do not send the lease key for these requests. As a
result, the server can assume that this request is from a new client, and
send a lease break notification to the same client, on the same
connection. As a response to the lease break, the client can consume
several credits to write the dirty pages to the server. Depending on the
server's credit grant implementation, the server can stop granting more
credits to this connection, and this can cause a deadlock (which can only
be resolved when the lease timer on the server expires).
One of the problems here is that the client is sending no lease key,
even if it has a lease for the file. This patch fixes the problem by
reusing the existing lease key on the file for rename, unlink and set path
size compound operations so that the client does not break its own lease.
A very trivial example could be a set of commands by a client that
maintains open handle (for write) to a file and then tries to copy the
contents of that file to another one, eg.,
tail -f /dev/null > myfile &
mv myfile myfile2
Presently, the network capture on the client shows that the move (or
rename) would trigger a lease break on the same client, for the same file.
With the lease key reused, the lease break request-response overhead is
eliminated, thereby reducing the roundtrips performed for this set of
operations.
The patch fixes the bug described above and also provides perf benefit.
Signed-off-by: Meetakshi Setiya <[email protected]>
---
fs/smb/client/cifsglob.h | 5 ++--
fs/smb/client/cifsproto.h | 6 +++--
fs/smb/client/cifssmb.c | 4 ++--
fs/smb/client/inode.c | 10 ++++----
fs/smb/client/smb2inode.c | 48 ++++++++++++++++++++++++---------------
fs/smb/client/smb2proto.h | 6 +++--
6 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 16befff4cbb4..50f7e47c2229 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -371,7 +371,8 @@ struct smb_version_operations {
struct cifs_open_info_data *data);
/* set size by path */
int (*set_path_size)(const unsigned int, struct cifs_tcon *,
- const char *, __u64, struct cifs_sb_info *, bool);
+ const char *, __u64, struct cifs_sb_info *, bool,
+ struct dentry *);
/* set size by file handle */
int (*set_file_size)(const unsigned int, struct cifs_tcon *,
struct cifsFileInfo *, __u64, bool);
@@ -401,7 +402,7 @@ struct smb_version_operations {
struct cifs_sb_info *);
/* unlink file */
int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
- struct cifs_sb_info *);
+ struct cifs_sb_info *, struct dentry *);
/* open, rename and delete file */
int (*rename_pending_delete)(const char *, struct dentry *,
const unsigned int);
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index a841bf4967fa..ef98c840791f 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -402,7 +402,8 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
__u32 pid_of_opener);
extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
const char *file_name, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_allocation);
+ struct cifs_sb_info *cifs_sb, bool set_allocation,
+ struct dentry *dentry);
extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
struct cifsFileInfo *cfile, __u64 size,
bool set_allocation);
@@ -438,7 +439,8 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
const struct nls_table *nls_codepage,
int remap_special_chars);
extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
- const char *name, struct cifs_sb_info *cifs_sb);
+ const char *name, struct cifs_sb_info *cifs_sb,
+ struct dentry *dentry);
int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
struct dentry *source_dentry,
const char *from_name, const char *to_name,
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 01e89070df5a..301189ee1335 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
int
CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
- struct cifs_sb_info *cifs_sb)
+ struct cifs_sb_info *cifs_sb, struct dentry *dentry)
{
DELETE_FILE_REQ *pSMB = NULL;
DELETE_FILE_RSP *pSMBr = NULL;
@@ -4993,7 +4993,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
int
CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
- bool set_allocation)
+ bool set_allocation, struct dentry *dentry)
{
struct smb_com_transaction2_spi_req *pSMB = NULL;
struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index d02f8ba29cb5..3073eac989ea 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1846,7 +1846,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
goto psx_del_no_retry;
}
- rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
+ rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
psx_del_no_retry:
if (!rc) {
@@ -2797,7 +2797,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
static int
cifs_set_file_size(struct inode *inode, struct iattr *attrs,
- unsigned int xid, const char *full_path)
+ unsigned int xid, const char *full_path, struct dentry *dentry)
{
int rc;
struct cifsFileInfo *open_file;
@@ -2848,7 +2848,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
*/
if (server->ops->set_path_size)
rc = server->ops->set_path_size(xid, tcon, full_path,
- attrs->ia_size, cifs_sb, false);
+ attrs->ia_size, cifs_sb, false, dentry);
else
rc = -ENOSYS;
cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
@@ -2938,7 +2938,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
rc = 0;
if (attrs->ia_valid & ATTR_SIZE) {
- rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
if (rc != 0)
goto out;
}
@@ -3105,7 +3105,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
}
if (attrs->ia_valid & ATTR_SIZE) {
- rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
if (rc != 0)
goto cifs_setattr_exit;
}
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 05818cd6d932..69f3442c5b96 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -98,7 +98,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
__u32 desired_access, __u32 create_disposition,
__u32 create_options, umode_t mode, struct kvec *in_iov,
int *cmds, int num_cmds, struct cifsFileInfo *cfile,
- struct kvec *out_iov, int *out_buftype)
+ struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
{
struct reparse_data_buffer *rbuf;
@@ -115,6 +115,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
int resp_buftype[MAX_COMPOUND];
struct smb2_query_info_rsp *qi_rsp = NULL;
struct cifs_open_info_data *idata;
+ struct inode *inode = NULL;
int flags = 0;
__u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
unsigned int size[2];
@@ -152,6 +153,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
goto finished;
}
+ /* if there is an existing lease, reuse it */
+ if (dentry) {
+ inode = d_inode(dentry);
+ if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
+ oplock = SMB2_OPLOCK_LEVEL_LEASE;
+ server->ops->get_lease_key(inode, &fid);
+ }
+ }
+
vars->oparms = (struct cifs_open_parms) {
.tcon = tcon,
.path = full_path,
@@ -747,7 +757,7 @@ int smb2_query_path_info(const unsigned int xid,
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
FILE_READ_ATTRIBUTES, FILE_OPEN,
create_options, ACL_NO_MODE, in_iov,
- cmds, 1, cfile, out_iov, out_buftype);
+ cmds, 1, cfile, out_iov, out_buftype, NULL);
hdr = out_iov[0].iov_base;
/*
* If first iov is unset, then SMB session was dropped or we've got a
@@ -779,7 +789,7 @@ int smb2_query_path_info(const unsigned int xid,
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
FILE_READ_ATTRIBUTES, FILE_OPEN,
create_options, ACL_NO_MODE, in_iov,
- cmds, num_cmds, cfile, NULL, NULL);
+ cmds, num_cmds, cfile, NULL, NULL, NULL);
break;
case -EREMOTE:
break;
@@ -811,7 +821,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
FILE_WRITE_ATTRIBUTES, FILE_CREATE,
CREATE_NOT_FILE, mode,
NULL, &(int){SMB2_OP_MKDIR}, 1,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
void
@@ -836,7 +846,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
FILE_WRITE_ATTRIBUTES, FILE_CREATE,
CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_INFO}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
if (tmprc == 0)
cifs_i->cifsAttrs = dosattrs;
}
@@ -850,25 +860,26 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
DELETE, FILE_OPEN, CREATE_NOT_FILE,
ACL_NO_MODE, NULL,
&(int){SMB2_OP_RMDIR}, 1,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
int
smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
- struct cifs_sb_info *cifs_sb)
+ struct cifs_sb_info *cifs_sb, struct dentry *dentry)
{
return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
ACL_NO_MODE, NULL,
&(int){SMB2_OP_DELETE}, 1,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, dentry);
}
static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
const char *from_name, const char *to_name,
struct cifs_sb_info *cifs_sb,
__u32 create_options, __u32 access,
- int command, struct cifsFileInfo *cfile)
+ int command, struct cifsFileInfo *cfile,
+ struct dentry *dentry)
{
struct kvec in_iov;
__le16 *smb2_to_name = NULL;
@@ -883,7 +894,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
FILE_OPEN, create_options, ACL_NO_MODE,
- &in_iov, &command, 1, cfile, NULL, NULL);
+ &in_iov, &command, 1, cfile, NULL, NULL, dentry);
smb2_rename_path:
kfree(smb2_to_name);
return rc;
@@ -902,7 +913,7 @@ int smb2_rename_path(const unsigned int xid,
cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
- co, DELETE, SMB2_OP_RENAME, cfile);
+ co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
}
int smb2_create_hardlink(const unsigned int xid,
@@ -915,13 +926,14 @@ int smb2_create_hardlink(const unsigned int xid,
return smb2_set_path_attr(xid, tcon, from_name, to_name,
cifs_sb, co, FILE_READ_ATTRIBUTES,
- SMB2_OP_HARDLINK, NULL);
+ SMB2_OP_HARDLINK, NULL, NULL);
}
int
smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
const char *full_path, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_alloc)
+ struct cifs_sb_info *cifs_sb, bool set_alloc,
+ struct dentry *dentry)
{
struct cifsFileInfo *cfile;
struct kvec in_iov;
@@ -934,7 +946,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
FILE_WRITE_DATA, FILE_OPEN,
0, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_EOF}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, dentry);
}
int
@@ -963,7 +975,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
FILE_WRITE_ATTRIBUTES, FILE_OPEN,
0, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_INFO}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
cifs_put_tlink(tlink);
return rc;
}
@@ -998,7 +1010,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
da, cd, co, ACL_NO_MODE, in_iov,
- cmds, 2, cfile, NULL, NULL);
+ cmds, 2, cfile, NULL, NULL, NULL);
if (!rc) {
rc = smb311_posix_get_inode_info(&new, full_path,
data, sb, xid);
@@ -1008,7 +1020,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
da, cd, co, ACL_NO_MODE, in_iov,
- cmds, 2, cfile, NULL, NULL);
+ cmds, 2, cfile, NULL, NULL, NULL);
if (!rc) {
rc = cifs_get_inode_info(&new, full_path,
data, sb, xid, NULL);
@@ -1036,7 +1048,7 @@ int smb2_query_reparse_point(const unsigned int xid,
FILE_READ_ATTRIBUTES, FILE_OPEN,
OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_GET_REPARSE}, 1,
- cfile, NULL, NULL);
+ cfile, NULL, NULL, NULL);
if (rc)
goto out;
diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
index b3069911e9dd..221143788a1c 100644
--- a/fs/smb/client/smb2proto.h
+++ b/fs/smb/client/smb2proto.h
@@ -75,7 +75,8 @@ int smb2_query_path_info(const unsigned int xid,
struct cifs_open_info_data *data);
extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
const char *full_path, __u64 size,
- struct cifs_sb_info *cifs_sb, bool set_alloc);
+ struct cifs_sb_info *cifs_sb, bool set_alloc,
+ struct dentry *dentry);
extern int smb2_set_file_info(struct inode *inode, const char *full_path,
FILE_BASIC_INFO *buf, const unsigned int xid);
extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
@@ -91,7 +92,8 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
const char *name, struct cifs_sb_info *cifs_sb);
extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
- const char *name, struct cifs_sb_info *cifs_sb);
+ const char *name, struct cifs_sb_info *cifs_sb,
+ struct dentry *dentry);
int smb2_rename_path(const unsigned int xid,
struct cifs_tcon *tcon,
struct dentry *source_dentry,
--
2.39.2
From: Meetakshi Setiya <[email protected]>
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 <[email protected]>
---
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 50f7e47c2229..a88c8328b29c 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1417,6 +1417,7 @@ struct cifsFileInfo {
bool invalidHandle:1; /* file closed via session abend */
bool swapfile:1;
bool oplock_break_cancelled:1;
+ 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 ef98c840791f..1f46e0db6e92 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -296,6 +296,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 b75282c204da..46951f403d31 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -483,6 +483,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);
@@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -893,6 +893,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;
@@ -907,7 +910,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);
@@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
rc = cifs_fattr_to_inode(inode, &fattr);
cgfi_exit:
cifs_free_open_info(&data);
+ free_dentry_path(page);
free_xid(xid);
return rc;
}
@@ -1075,6 +1086,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) {
@@ -1114,8 +1126,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);
@@ -1170,6 +1186,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:
@@ -1850,16 +1868,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 0748d7b757b9..9428a0db7718 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 69f3442c5b96..429d83d31280 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)
--
2.39.2
From: Meetakshi Setiya <[email protected]>
There is a shortcoming in the current implementation of the file
lease mechanism exposed when the lease keys were attempted to be
reused for unlink, rename and set_path_size operations for a client. As
per MS-SMB2, lease keys are associated with the file name. Linux smb
client maintains lease keys with the inode. If the file has any hardlinks,
it is possible that the lease for a file be wrongly reused for an
operation on the hardlink or vice versa. In these cases, the mentioned
compound operations fail with STATUS_INVALID_PARAMETER.
This patch adds a fallback to the old mechanism of not sending any
lease with these compound operations if the request with lease key fails
with STATUS_INVALID_PARAMETER.
Resending the same request without lease key should not hurt any
functionality, but might impact performance especially in cases where
the error is not because of the usage of wrong lease key and we might
end up doing an extra roundtrip.
Signed-off-by: Meetakshi Setiya <[email protected]>
---
fs/smb/client/smb2inode.c | 41 ++++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 429d83d31280..f697c14cd8c6 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -154,6 +154,17 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
}
/* if there is an existing lease, reuse it */
+
+ /*
+ * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2,
+ * lease keys are associated with the filepath. We are maintaining lease keys
+ * with the inode on the client. If the file has hardlinks, it is possible
+ * that the lease for a file be reused for an operation on its hardlink or
+ * vice versa.
+ * As a workaround, send request using an existing lease key and if the server
+ * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request
+ * again without the lease.
+ */
if (dentry) {
inode = d_inode(dentry);
if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
@@ -874,11 +885,20 @@ int
smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
struct cifs_sb_info *cifs_sb, struct dentry *dentry)
{
- return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+ int rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
ACL_NO_MODE, NULL,
&(int){SMB2_OP_DELETE}, 1,
NULL, NULL, NULL, dentry);
+ if (rc == -EINVAL) {
+ cifs_dbg(FYI, "invalid lease key, resending request without lease");
+ rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
+ CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
+ ACL_NO_MODE, NULL,
+ &(int){SMB2_OP_DELETE}, 1,
+ NULL, NULL, NULL, NULL);
+ }
+ return rc;
}
static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
@@ -919,8 +939,14 @@ int smb2_rename_path(const unsigned int xid,
drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb);
cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
- return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+ int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
+ if (rc == -EINVAL) {
+ cifs_dbg(FYI, "invalid lease key, resending request without lease");
+ rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
+ co, DELETE, SMB2_OP_RENAME, cfile, NULL);
+ }
+ return rc;
}
int smb2_create_hardlink(const unsigned int xid,
@@ -949,11 +975,20 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
in_iov.iov_base = &eof;
in_iov.iov_len = sizeof(eof);
cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
- return smb2_compound_op(xid, tcon, cifs_sb, full_path,
+ int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
FILE_WRITE_DATA, FILE_OPEN,
0, ACL_NO_MODE, &in_iov,
&(int){SMB2_OP_SET_EOF}, 1,
cfile, NULL, NULL, dentry);
+ if (rc == -EINVAL) {
+ cifs_dbg(FYI, "invalid lease key, resending request without lease");
+ rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
+ FILE_WRITE_DATA, FILE_OPEN,
+ 0, ACL_NO_MODE, &in_iov,
+ &(int){SMB2_OP_SET_EOF}, 1,
+ cfile, NULL, NULL, NULL);
+ }
+ return rc;
}
int
--
2.39.2
Updated version of the patch
https://lore.kernel.org/all/[email protected]/
Includes perf improvement by iterating over cinode's openFileList instead
of tcon openFileList. Also, compare the deleted path with the path of each
open file in the cinode only when the file has hardlinks. Else, all open
handles are from the same file only.
Thanks
Meetakshi
On Wed, Mar 6, 2024 at 9:14 AM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 50f7e47c2229..a88c8328b29c 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1417,6 +1417,7 @@ struct cifsFileInfo {
> bool invalidHandle:1; /* file closed via session abend */
> bool swapfile:1;
> bool oplock_break_cancelled:1;
> + 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 ef98c840791f..1f46e0db6e92 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -296,6 +296,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 b75282c204da..46951f403d31 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -483,6 +483,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);
> @@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -893,6 +893,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;
> @@ -907,7 +910,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);
> @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> rc = cifs_fattr_to_inode(inode, &fattr);
> cgfi_exit:
> cifs_free_open_info(&data);
> + free_dentry_path(page);
> free_xid(xid);
> return rc;
> }
> @@ -1075,6 +1086,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) {
> @@ -1114,8 +1126,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);
> @@ -1170,6 +1186,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:
> @@ -1850,16 +1868,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 0748d7b757b9..9428a0db7718 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 69f3442c5b96..429d83d31280 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)
> --
> 2.39.2
>
On its own, the patch is expected to regress xfstests generic 013,
002 and 591.
The fix for 591 is in part 2 of this patch series
https://lore.kernel.org/all/[email protected]/
The fix for 002 and 013 is in part 3 of this patch series
https://lore.kernel.org/all/[email protected]/
On Wed, Mar 6, 2024 at 9:13 AM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> Currently, when a rename, unlink or set path size compound operation
> is requested on a file that has a lot of dirty pages to be written
> to the server, we do not send the lease key for these requests. As a
> result, the server can assume that this request is from a new client, and
> send a lease break notification to the same client, on the same
> connection. As a response to the lease break, the client can consume
> several credits to write the dirty pages to the server. Depending on the
> server's credit grant implementation, the server can stop granting more
> credits to this connection, and this can cause a deadlock (which can only
> be resolved when the lease timer on the server expires).
> One of the problems here is that the client is sending no lease key,
> even if it has a lease for the file. This patch fixes the problem by
> reusing the existing lease key on the file for rename, unlink and set path
> size compound operations so that the client does not break its own lease.
>
> A very trivial example could be a set of commands by a client that
> maintains open handle (for write) to a file and then tries to copy the
> contents of that file to another one, eg.,
>
> tail -f /dev/null > myfile &
> mv myfile myfile2
>
> Presently, the network capture on the client shows that the move (or
> rename) would trigger a lease break on the same client, for the same file.
> With the lease key reused, the lease break request-response overhead is
> eliminated, thereby reducing the roundtrips performed for this set of
> operations.
>
> The patch fixes the bug described above and also provides perf benefit.
>
> Signed-off-by: Meetakshi Setiya <[email protected]>
> ---
> fs/smb/client/cifsglob.h | 5 ++--
> fs/smb/client/cifsproto.h | 6 +++--
> fs/smb/client/cifssmb.c | 4 ++--
> fs/smb/client/inode.c | 10 ++++----
> fs/smb/client/smb2inode.c | 48 ++++++++++++++++++++++++---------------
> fs/smb/client/smb2proto.h | 6 +++--
> 6 files changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 16befff4cbb4..50f7e47c2229 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -371,7 +371,8 @@ struct smb_version_operations {
> struct cifs_open_info_data *data);
> /* set size by path */
> int (*set_path_size)(const unsigned int, struct cifs_tcon *,
> - const char *, __u64, struct cifs_sb_info *, bool);
> + const char *, __u64, struct cifs_sb_info *, bool,
> + struct dentry *);
> /* set size by file handle */
> int (*set_file_size)(const unsigned int, struct cifs_tcon *,
> struct cifsFileInfo *, __u64, bool);
> @@ -401,7 +402,7 @@ struct smb_version_operations {
> struct cifs_sb_info *);
> /* unlink file */
> int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
> - struct cifs_sb_info *);
> + struct cifs_sb_info *, struct dentry *);
> /* open, rename and delete file */
> int (*rename_pending_delete)(const char *, struct dentry *,
> const unsigned int);
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index a841bf4967fa..ef98c840791f 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -402,7 +402,8 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
> __u32 pid_of_opener);
> extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> const char *file_name, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_allocation);
> + struct cifs_sb_info *cifs_sb, bool set_allocation,
> + struct dentry *dentry);
> extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifsFileInfo *cfile, __u64 size,
> bool set_allocation);
> @@ -438,7 +439,8 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
> const struct nls_table *nls_codepage,
> int remap_special_chars);
> extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
> - const char *name, struct cifs_sb_info *cifs_sb);
> + const char *name, struct cifs_sb_info *cifs_sb,
> + struct dentry *dentry);
> int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
> struct dentry *source_dentry,
> const char *from_name, const char *to_name,
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 01e89070df5a..301189ee1335 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
>
> int
> CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> - struct cifs_sb_info *cifs_sb)
> + struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> DELETE_FILE_REQ *pSMB = NULL;
> DELETE_FILE_RSP *pSMBr = NULL;
> @@ -4993,7 +4993,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
> int
> CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> - bool set_allocation)
> + bool set_allocation, struct dentry *dentry)
> {
> struct smb_com_transaction2_spi_req *pSMB = NULL;
> struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index d02f8ba29cb5..3073eac989ea 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1846,7 +1846,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
> goto psx_del_no_retry;
> }
>
> - rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
> + rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
>
> psx_del_no_retry:
> if (!rc) {
> @@ -2797,7 +2797,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
>
> static int
> cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> - unsigned int xid, const char *full_path)
> + unsigned int xid, const char *full_path, struct dentry *dentry)
> {
> int rc;
> struct cifsFileInfo *open_file;
> @@ -2848,7 +2848,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> */
> if (server->ops->set_path_size)
> rc = server->ops->set_path_size(xid, tcon, full_path,
> - attrs->ia_size, cifs_sb, false);
> + attrs->ia_size, cifs_sb, false, dentry);
> else
> rc = -ENOSYS;
> cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
> @@ -2938,7 +2938,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
> rc = 0;
>
> if (attrs->ia_valid & ATTR_SIZE) {
> - rc = cifs_set_file_size(inode, attrs, xid, full_path);
> + rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
> if (rc != 0)
> goto out;
> }
> @@ -3105,7 +3105,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
> }
>
> if (attrs->ia_valid & ATTR_SIZE) {
> - rc = cifs_set_file_size(inode, attrs, xid, full_path);
> + rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
> if (rc != 0)
> goto cifs_setattr_exit;
> }
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 05818cd6d932..69f3442c5b96 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -98,7 +98,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> __u32 desired_access, __u32 create_disposition,
> __u32 create_options, umode_t mode, struct kvec *in_iov,
> int *cmds, int num_cmds, struct cifsFileInfo *cfile,
> - struct kvec *out_iov, int *out_buftype)
> + struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
> {
>
> struct reparse_data_buffer *rbuf;
> @@ -115,6 +115,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> int resp_buftype[MAX_COMPOUND];
> struct smb2_query_info_rsp *qi_rsp = NULL;
> struct cifs_open_info_data *idata;
> + struct inode *inode = NULL;
> int flags = 0;
> __u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
> unsigned int size[2];
> @@ -152,6 +153,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> goto finished;
> }
>
> + /* if there is an existing lease, reuse it */
> + if (dentry) {
> + inode = d_inode(dentry);
> + if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> + oplock = SMB2_OPLOCK_LEVEL_LEASE;
> + server->ops->get_lease_key(inode, &fid);
> + }
> + }
> +
> vars->oparms = (struct cifs_open_parms) {
> .tcon = tcon,
> .path = full_path,
> @@ -747,7 +757,7 @@ int smb2_query_path_info(const unsigned int xid,
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> create_options, ACL_NO_MODE, in_iov,
> - cmds, 1, cfile, out_iov, out_buftype);
> + cmds, 1, cfile, out_iov, out_buftype, NULL);
> hdr = out_iov[0].iov_base;
> /*
> * If first iov is unset, then SMB session was dropped or we've got a
> @@ -779,7 +789,7 @@ int smb2_query_path_info(const unsigned int xid,
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> create_options, ACL_NO_MODE, in_iov,
> - cmds, num_cmds, cfile, NULL, NULL);
> + cmds, num_cmds, cfile, NULL, NULL, NULL);
> break;
> case -EREMOTE:
> break;
> @@ -811,7 +821,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
> FILE_WRITE_ATTRIBUTES, FILE_CREATE,
> CREATE_NOT_FILE, mode,
> NULL, &(int){SMB2_OP_MKDIR}, 1,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> void
> @@ -836,7 +846,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
> FILE_WRITE_ATTRIBUTES, FILE_CREATE,
> CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_INFO}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> if (tmprc == 0)
> cifs_i->cifsAttrs = dosattrs;
> }
> @@ -850,25 +860,26 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> DELETE, FILE_OPEN, CREATE_NOT_FILE,
> ACL_NO_MODE, NULL,
> &(int){SMB2_OP_RMDIR}, 1,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> int
> smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> - struct cifs_sb_info *cifs_sb)
> + struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
> CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
> ACL_NO_MODE, NULL,
> &(int){SMB2_OP_DELETE}, 1,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, dentry);
> }
>
> static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> const char *from_name, const char *to_name,
> struct cifs_sb_info *cifs_sb,
> __u32 create_options, __u32 access,
> - int command, struct cifsFileInfo *cfile)
> + int command, struct cifsFileInfo *cfile,
> + struct dentry *dentry)
> {
> struct kvec in_iov;
> __le16 *smb2_to_name = NULL;
> @@ -883,7 +894,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
> rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
> FILE_OPEN, create_options, ACL_NO_MODE,
> - &in_iov, &command, 1, cfile, NULL, NULL);
> + &in_iov, &command, 1, cfile, NULL, NULL, dentry);
> smb2_rename_path:
> kfree(smb2_to_name);
> return rc;
> @@ -902,7 +913,7 @@ int smb2_rename_path(const unsigned int xid,
> cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
>
> return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> - co, DELETE, SMB2_OP_RENAME, cfile);
> + co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
> }
>
> int smb2_create_hardlink(const unsigned int xid,
> @@ -915,13 +926,14 @@ int smb2_create_hardlink(const unsigned int xid,
>
> return smb2_set_path_attr(xid, tcon, from_name, to_name,
> cifs_sb, co, FILE_READ_ATTRIBUTES,
> - SMB2_OP_HARDLINK, NULL);
> + SMB2_OP_HARDLINK, NULL, NULL);
> }
>
> int
> smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_alloc)
> + struct cifs_sb_info *cifs_sb, bool set_alloc,
> + struct dentry *dentry)
> {
> struct cifsFileInfo *cfile;
> struct kvec in_iov;
> @@ -934,7 +946,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> FILE_WRITE_DATA, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_EOF}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, dentry);
> }
>
> int
> @@ -963,7 +975,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
> FILE_WRITE_ATTRIBUTES, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_INFO}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> cifs_put_tlink(tlink);
> return rc;
> }
> @@ -998,7 +1010,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> da, cd, co, ACL_NO_MODE, in_iov,
> - cmds, 2, cfile, NULL, NULL);
> + cmds, 2, cfile, NULL, NULL, NULL);
> if (!rc) {
> rc = smb311_posix_get_inode_info(&new, full_path,
> data, sb, xid);
> @@ -1008,7 +1020,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> da, cd, co, ACL_NO_MODE, in_iov,
> - cmds, 2, cfile, NULL, NULL);
> + cmds, 2, cfile, NULL, NULL, NULL);
> if (!rc) {
> rc = cifs_get_inode_info(&new, full_path,
> data, sb, xid, NULL);
> @@ -1036,7 +1048,7 @@ int smb2_query_reparse_point(const unsigned int xid,
> FILE_READ_ATTRIBUTES, FILE_OPEN,
> OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_GET_REPARSE}, 1,
> - cfile, NULL, NULL);
> + cfile, NULL, NULL, NULL);
> if (rc)
> goto out;
>
> diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
> index b3069911e9dd..221143788a1c 100644
> --- a/fs/smb/client/smb2proto.h
> +++ b/fs/smb/client/smb2proto.h
> @@ -75,7 +75,8 @@ int smb2_query_path_info(const unsigned int xid,
> struct cifs_open_info_data *data);
> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> - struct cifs_sb_info *cifs_sb, bool set_alloc);
> + struct cifs_sb_info *cifs_sb, bool set_alloc,
> + struct dentry *dentry);
> extern int smb2_set_file_info(struct inode *inode, const char *full_path,
> FILE_BASIC_INFO *buf, const unsigned int xid);
> extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
> @@ -91,7 +92,8 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
> extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
> const char *name, struct cifs_sb_info *cifs_sb);
> extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
> - const char *name, struct cifs_sb_info *cifs_sb);
> + const char *name, struct cifs_sb_info *cifs_sb,
> + struct dentry *dentry);
> int smb2_rename_path(const unsigned int xid,
> struct cifs_tcon *tcon,
> struct dentry *source_dentry,
> --
> 2.39.2
>
Patch 1 of this patch series
https://lore.kernel.org/all/[email protected]/
aims to fix a customer reported bug by reusing lease key in unlink,
rename and set_path_size compound operations on the smb client. The
bug, its implications and reproduction has been described in the
commit message of the patch. In short, unlink, rename and
set_path_size operations can cause the server to send lease breaks to
the same client, on the same connection which hurts performance.
The aim is to have a fix in place for this problem without regressing
existing behaviour. Also, the proposed changes should go in smaller
batches so that they can be backported with relative ease. Patch 1
regressed a few operations on hardlinks (eg., xfstests generic 002,
013). As per MS-SMB2, lease keys are associated with the file name.
Linux cifs client maintains lease keys with the inode. If the file
has hardlinks, it is possible that the lease for a file be wrongly
reused for an operation on the hardlink or vice versa. In these
cases, the mentioned compound operations fail with
STATUS_INVALID_PARAMETER.
A simple fix for the regressions would be to have a two-phased
approach and resend the compound op request again without the lease
key if STATUS_INVALID_PARAMETER is received. This would help patch 1
fix the original issue. Fix(es) for the hardlink-leasekey problem can
come in the next batch.
On Wed, Mar 6, 2024 at 9:14 AM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> There is a shortcoming in the current implementation of the file
> lease mechanism exposed when the lease keys were attempted to be
> reused for unlink, rename and set_path_size operations for a client. As
> per MS-SMB2, lease keys are associated with the file name. Linux smb
> client maintains lease keys with the inode. If the file has any hardlinks,
> it is possible that the lease for a file be wrongly reused for an
> operation on the hardlink or vice versa. In these cases, the mentioned
> compound operations fail with STATUS_INVALID_PARAMETER.
> This patch adds a fallback to the old mechanism of not sending any
> lease with these compound operations if the request with lease key fails
> with STATUS_INVALID_PARAMETER.
> Resending the same request without lease key should not hurt any
> functionality, but might impact performance especially in cases where
> the error is not because of the usage of wrong lease key and we might
> end up doing an extra roundtrip.
>
> Signed-off-by: Meetakshi Setiya <[email protected]>
> ---
> fs/smb/client/smb2inode.c | 41 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 429d83d31280..f697c14cd8c6 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -154,6 +154,17 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> }
>
> /* if there is an existing lease, reuse it */
> +
> + /*
> + * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2,
> + * lease keys are associated with the filepath. We are maintaining lease keys
> + * with the inode on the client. If the file has hardlinks, it is possible
> + * that the lease for a file be reused for an operation on its hardlink or
> + * vice versa.
> + * As a workaround, send request using an existing lease key and if the server
> + * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request
> + * again without the lease.
> + */
> if (dentry) {
> inode = d_inode(dentry);
> if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> @@ -874,11 +885,20 @@ int
> smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> - return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
> + int rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
> CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
> ACL_NO_MODE, NULL,
> &(int){SMB2_OP_DELETE}, 1,
> NULL, NULL, NULL, dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
> + CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
> + ACL_NO_MODE, NULL,
> + &(int){SMB2_OP_DELETE}, 1,
> + NULL, NULL, NULL, NULL);
> + }
> + return rc;
> }
>
> static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> @@ -919,8 +939,14 @@ int smb2_rename_path(const unsigned int xid,
> drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb);
> cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
>
> - return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> + int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> + co, DELETE, SMB2_OP_RENAME, cfile, NULL);
> + }
> + return rc;
> }
>
> int smb2_create_hardlink(const unsigned int xid,
> @@ -949,11 +975,20 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> in_iov.iov_base = &eof;
> in_iov.iov_len = sizeof(eof);
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> - return smb2_compound_op(xid, tcon, cifs_sb, full_path,
> + int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_WRITE_DATA, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_EOF}, 1,
> cfile, NULL, NULL, dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> + FILE_WRITE_DATA, FILE_OPEN,
> + 0, ACL_NO_MODE, &in_iov,
> + &(int){SMB2_OP_SET_EOF}, 1,
> + cfile, NULL, NULL, NULL);
> + }
> + return rc;
> }
>
> int
> --
> 2.39.2
>
Merged the three patches into cifs-2.6.git pending testing but I had
to rebase this one. I also had to rebase one of Paulo's patches "smb:
client: move most of reparse point handling code to common file"
If you see anything wrong with the minor rebase let me know.
On Tue, Mar 5, 2024 at 9:44 PM <[email protected]> wrote:
>
> From: Meetakshi Setiya <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 50f7e47c2229..a88c8328b29c 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1417,6 +1417,7 @@ struct cifsFileInfo {
> bool invalidHandle:1; /* file closed via session abend */
> bool swapfile:1;
> bool oplock_break_cancelled:1;
> + 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 ef98c840791f..1f46e0db6e92 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -296,6 +296,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 b75282c204da..46951f403d31 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -483,6 +483,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);
> @@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -893,6 +893,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;
> @@ -907,7 +910,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);
> @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> rc = cifs_fattr_to_inode(inode, &fattr);
> cgfi_exit:
> cifs_free_open_info(&data);
> + free_dentry_path(page);
> free_xid(xid);
> return rc;
> }
> @@ -1075,6 +1086,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) {
> @@ -1114,8 +1126,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);
> @@ -1170,6 +1186,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:
> @@ -1850,16 +1868,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 0748d7b757b9..9428a0db7718 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 69f3442c5b96..429d83d31280 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)
> --
> 2.39.2
>
>
--
Thanks,
Steve
Just checked, looks fine to me.
(apologies for the previous mail)
On Wed, Mar 6, 2024 at 12:39 PM Steve French <[email protected]> wrote:
>
> Merged the three patches into cifs-2.6.git pending testing but I had
> to rebase this one. I also had to rebase one of Paulo's patches "smb:
> client: move most of reparse point handling code to common file"
>
> If you see anything wrong with the minor rebase let me know.
>
> On Tue, Mar 5, 2024 at 9:44 PM <[email protected]> wrote:
> >
> > From: Meetakshi Setiya <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 50f7e47c2229..a88c8328b29c 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -1417,6 +1417,7 @@ struct cifsFileInfo {
> > bool invalidHandle:1; /* file closed via session abend */
> > bool swapfile:1;
> > bool oplock_break_cancelled:1;
> > + 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 ef98c840791f..1f46e0db6e92 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -296,6 +296,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 b75282c204da..46951f403d31 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -483,6 +483,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);
> > @@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -893,6 +893,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;
> > @@ -907,7 +910,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);
> > @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> > rc = cifs_fattr_to_inode(inode, &fattr);
> > cgfi_exit:
> > cifs_free_open_info(&data);
> > + free_dentry_path(page);
> > free_xid(xid);
> > return rc;
> > }
> > @@ -1075,6 +1086,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) {
> > @@ -1114,8 +1126,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);
> > @@ -1170,6 +1186,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:
> > @@ -1850,16 +1868,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 0748d7b757b9..9428a0db7718 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 69f3442c5b96..429d83d31280 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)
> > --
> > 2.39.2
> >
> >
>
>
> --
> Thanks,
>
> Steve
[email protected] writes:
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index b75282c204da..46951f403d31 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -483,6 +483,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;
This is unnecessary as @cfile is kzalloc()'d.
> cfile->invalidHandle = false;
> cfile->deferred_close_scheduled = false;
> cfile->tlink = cifs_get_tlink(tlink);
> @@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -893,6 +893,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;
You're leaking @page if above condition is true.
> @@ -907,7 +910,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);
Now you're leaking @data and @fid if above condition is true.
> + }
> 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);
> @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> rc = cifs_fattr_to_inode(inode, &fattr);
> cgfi_exit:
> cifs_free_open_info(&data);
> + free_dentry_path(page);
> free_xid(xid);
> return rc;
> }
> @@ -1075,6 +1086,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) {
> @@ -1114,8 +1126,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)
You shouldn't ignore if cifs_iget() failed. Return -ENOMEM instead.
Besides, calling cifs_iget() here looks wrong as @fattr is not fully
set, yet. Why can't you use @inode from cifs_get_fattr() or do above
check right after cifs_get_fattr() in cifs_get_inode_info{,_unix}()?
Hi
PFA the follow up patch after the above revision.
Thanks
Meetakshi
On Wed, Mar 6, 2024 at 8:01 PM Paulo Alcantara <[email protected]> wrote:
>
> [email protected] writes:
>
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index b75282c204da..46951f403d31 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -483,6 +483,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;
>
> This is unnecessary as @cfile is kzalloc()'d.
>
> > cfile->invalidHandle = false;
> > cfile->deferred_close_scheduled = false;
> > cfile->tlink = cifs_get_tlink(tlink);
> > @@ -1085,7 +1086,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 3073eac989ea..3242e3b74386 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -893,6 +893,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;
>
> You're leaking @page if above condition is true.
>
> > @@ -907,7 +910,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);
>
> Now you're leaking @data and @fid if above condition is true.
>
> > + }
> > 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);
> > @@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
> > rc = cifs_fattr_to_inode(inode, &fattr);
> > cgfi_exit:
> > cifs_free_open_info(&data);
> > + free_dentry_path(page);
> > free_xid(xid);
> > return rc;
> > }
> > @@ -1075,6 +1086,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) {
> > @@ -1114,8 +1126,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)
>
> You shouldn't ignore if cifs_iget() failed. Return -ENOMEM instead.
>
> Besides, calling cifs_iget() here looks wrong as @fattr is not fully
> set, yet. Why can't you use @inode from cifs_get_fattr() or do above
> check right after cifs_get_fattr() in cifs_get_inode_info{,_unix}()?